Discussion:
[Mason-devel] [PATCH] Refactor and split out buffer stack
Alex Vandiver
2008-10-30 19:39:16 UTC
Permalink
Heya,
As some of you may know, Jifty uses both HTML::Mason and
Template::Declare as templating systems. Unfortunately, they both have
differing buffer stacks, which means that intercalling between the two
of them is difficult and error-prone. By factoring out Mason's buffer
stack code, they can be made able to intercall, more or less seamlessly.
It does, however, introduce a new dependency on the new
String::BufferStack.
Additionally, this resolves several outstanding bugs with flushing
buffers, filters, and clearing buffers -- specifically, [cpan.org
#38924], and [cpan.org #23535], as well as another related bug using
scomp which I have not reported to bugs.cpan.org yet.
These changes come at a slight performance hit -- 7.46 CPU-seconds
versus 7.39 CPU-seconds over the course of the test suite[1]. I believe
this performance hit comes mostly from the use of method calls, which
are relatively slow in perl. I briefly experimented with reimplementing
String::BufferStack in XS, but this proved to have no measurable
performance improvement, and provided an obviously increased maintenance
hassle.
The only part of the refactoring which whas me uneasy is the use of
the buffer stack to store information about the Mason component stack.
In the interest of keeping code more similar, I left the two intertwined
-- however, this makes relatively little sense if the purpose of the
module is to increase interoperability between templating systems, as
other templating systems will not be able to use the data stack, and
will make no sense of Mason's use of it.
The patch against SVN trunk is attached; code for String::BufferStack
can be found in Best Practical's SVN repository at
http://code.bestpractical.com/bps-public/String-BufferStack/

Comments, thoughts, and feedback?
- Alex

[1] Averaged over the course of 10 test runs each; std. dev. on those
was 0.01563 and 0.03035, respectively.
Alex Vandiver
2008-11-05 17:37:36 UTC
Permalink
Post by Alex Vandiver
The patch against SVN trunk is attached; code for String::BufferStack
can be found in Best Practical's SVN repository at
http://code.bestpractical.com/bps-public/String-BufferStack/
Any feedback or thoughts on this?
- Alex
John Williams
2008-11-06 21:43:08 UTC
Permalink
I, for one, would feel a lot better about it if String::BufferStack were
on CPAN. Adding a dependancy on a non-cpan module is a definite no vote
from me, without even considering any other merits.

~ John Williams
Post by Alex Vandiver
Post by Alex Vandiver
The patch against SVN trunk is attached; code for String::BufferStack
can be found in Best Practical's SVN repository at
http://code.bestpractical.com/bps-public/String-BufferStack/
Any feedback or thoughts on this?
- Alex
Dave Rolsky
2008-11-07 03:13:38 UTC
Permalink
Post by John Williams
I, for one, would feel a lot better about it if String::BufferStack were
on CPAN. Adding a dependancy on a non-cpan module is a definite no vote
from me, without even considering any other merits.
I'm sure Alex would release this to CPAN if it were going to be part of
Mason, but for now it's just an experiment to see if he can unify the
Mason & TD buffers.


-dave

/*============================================================
http://VegGuide.org http://blog.urth.org
Your guide to all that's veg House Absolute(ly Pointless)
============================================================*/
Alex Vandiver
2008-11-07 03:38:54 UTC
Permalink
Post by Dave Rolsky
Post by John Williams
I, for one, would feel a lot better about it if String::BufferStack were
on CPAN. Adding a dependancy on a non-cpan module is a definite no vote
from me, without even considering any other merits.
I'm sure Alex would release this to CPAN if it were going to be part of
Mason, but for now it's just an experiment to see if he can unify the
Mason & TD buffers.
Oh, absolutely -- it's on its way there now, in case that was causing
people to hesitate looking at this. I wasn't going to push it to CPAN
unless people thought the experiment was worth looking at.

To be clear, the experiment is a success, from my point of view. The
diffstat to Jifty removes a bunch of crufty code, and allows
inter-calling between the templating systems that wasn't possible
before. For instance, you can $m->scomp a Template::Declare template,
and it Just Works. Hence why I'm interested in getting this patch
applied.

At this point, I want to know what the chances of inclusion into the
mason core are. If this patch were possible to do with subclassing the
request object instead, I'd do it and not trouble trunk with the changes
-- the difficulty is that the buffer code is more or less everywhere, so
there's no clean way to subclass the request object without copying more
or less the whole file.
- Alex
Jonathan Swartz
2008-11-08 14:37:20 UTC
Permalink
Post by Alex Vandiver
Post by Dave Rolsky
Post by John Williams
I, for one, would feel a lot better about it if
String::BufferStack were
on CPAN. Adding a dependancy on a non-cpan module is a definite no vote
from me, without even considering any other merits.
I'm sure Alex would release this to CPAN if it were going to be part of
Mason, but for now it's just an experiment to see if he can unify the
Mason & TD buffers.
Oh, absolutely -- it's on its way there now, in case that was causing
people to hesitate looking at this. I wasn't going to push it to CPAN
unless people thought the experiment was worth looking at.
To be clear, the experiment is a success, from my point of view. The
diffstat to Jifty removes a bunch of crufty code, and allows
inter-calling between the templating systems that wasn't possible
before. For instance, you can $m->scomp a Template::Declare template,
and it Just Works. Hence why I'm interested in getting this patch
applied.
At this point, I want to know what the chances of inclusion into the
mason core are. If this patch were possible to do with subclassing the
request object instead, I'd do it and not trouble trunk with the changes
-- the difficulty is that the buffer code is more or less
everywhere, so
there's no clean way to subclass the request object without copying more
or less the whole file.
- Alex
One of us (Dave or myself) has to take a look at it, but in general,
I'd be supportive of including this in the core. Cleaning up the
buffer code and making Mason more interoperable with other templating
systems is good!

Jon
Jonathan Swartz
2008-12-15 23:24:38 UTC
Permalink
Hi Alex,

Sorry for the long delay in looking at your patch. I finally looked at
it this weekend. I can't recommend accepting this patch, but I'd like
to come up with a way for you (and others) to integrate it with
subclassing, with a minimum of cut-and-paste.

The main problem with the patch is the performance hit, which you did
candidly mention. In particular, the patch rolls back a key
optimization made in 1.3 - that each string output from a component
requires just a concatenation, rather than a method call. Sadly, we
don't have a good benchmark suite in Mason to test the typical
performance impact of changes (it's hard to know what a "typical" use
of Mason is), and running the test suite may unfortunately be a poor
substitute because the vast majority of test pages and components are
pretty simple. But at Amazon, where many of the 1.3 optimizations were
implemented, the concatenation optimzation did make a small-but-
noticeable improvement in total page cost. I can't see hitting the
vast majority of users who will never use this new feature with even a
small performance penalty.

As far as the flush_buffer related bug fixes. I am inclined to handle
those in a different way, but that's a large topic that I'll put in a
separate email.

As far as integrating String::BufferStack with subclassing - if you
set the enable_auto_flush property, Mason will call '$m->print'
whenever anything is output in a component. You can then override
print() to do what you want. Is this sufficient, or does it at least
make things easier? If not, I'd like to work with you further on it.

Thanks
Jon
Post by Alex Vandiver
Post by Dave Rolsky
Post by John Williams
I, for one, would feel a lot better about it if
String::BufferStack were
on CPAN. Adding a dependancy on a non-cpan module is a definite no vote
from me, without even considering any other merits.
I'm sure Alex would release this to CPAN if it were going to be part of
Mason, but for now it's just an experiment to see if he can unify the
Mason & TD buffers.
Oh, absolutely -- it's on its way there now, in case that was causing
people to hesitate looking at this. I wasn't going to push it to CPAN
unless people thought the experiment was worth looking at.
To be clear, the experiment is a success, from my point of view. The
diffstat to Jifty removes a bunch of crufty code, and allows
inter-calling between the templating systems that wasn't possible
before. For instance, you can $m->scomp a Template::Declare template,
and it Just Works. Hence why I'm interested in getting this patch
applied.
At this point, I want to know what the chances of inclusion into the
mason core are. If this patch were possible to do with subclassing the
request object instead, I'd do it and not trouble trunk with the changes
-- the difficulty is that the buffer code is more or less
everywhere, so
there's no clean way to subclass the request object without copying more
or less the whole file.
- Alex
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's
challenge
Build the coolest Linux based applications with Moblin SDK & win
great prizes
Grand prize is a trip for two to an Open Source event anywhere in
the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Mason-devel mailing list
https://lists.sourceforge.net/lists/listinfo/mason-devel
Loading...