[PATCH] Refactor and split out buffer stack
Alex Vandiver <[email protected]> Thu, 30 Oct 2008 15:39:16 -0400
| Newsgroups | gmane.comp.web.mason.devel |
|---|---|
| Organization | Best Practical, LLC |
| Message-ID | <1225395556.28351.30.camel@localhost> |
--=-Kq3Lf/TGO4zifOrQ+dnP
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
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.=20
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 =EF=BB=BF0.01563 and =EF=BB=BF0.03035, respectively.
--=-Kq3Lf/TGO4zifOrQ+dnP
Content-Disposition: attachment; filename=string-bufferstack.patch
Content-Type: text/x-patch; name=string-bufferstack.patch; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Index: t/05-request.t
===================================================================
--- t/05-request.t (revision 3897)
+++ t/05-request.t (working copy)
@@ -935,5 +935,54 @@
#------------------------------------------------------------
+ $group->add_test( name => 'flush_and_store',
+ description => 'Test that $m->flush_buffer is ignored in a store\'d component',
+ interp_params => { autoflush => 1 },
+ component => <<'EOF',
+<%def .world>\
+World\
+</%def>
+
+% my $world;
+% $m->comp( { store => \$world }, '.world');
+Hello, <% $world %>!
+
+% $world = $m->scomp('.world');
+Hello, <% $world %>!
+EOF
+ expect => <<'EOF',
+
+Hello, World!
+
+Hello, World!
+EOF
+ );
+
+#------------------------------------------------------------
+
+ $group->add_test( name => 'flush_and_scomp_recursive',
+ description => 'Test that $m->flush_buffer is ignored in a recursive scomp() call',
+ interp_params => { autoflush => 1 },
+ component => <<'EOF',
+<%def .orld>\
+orld\
+</%def>
+
+<%def .world>\
+W<& .orld &>\
+</%def>
+
+% my $world = $m->scomp('.world');
+Hello, <% $world %>!
+EOF
+ expect => <<'EOF',
+
+
+Hello, World!
+EOF
+ );
+
+#------------------------------------------------------------
+
return $group;
}
Index: lib/HTML/Mason/Compiler.pm
===================================================================
--- lib/HTML/Mason/Compiler.pm (revision 3897)
+++ lib/HTML/Mason/Compiler.pm (working copy)
@@ -333,7 +333,7 @@
if ($self->enable_autoflush) {
$self->_add_body_code("\$m->print( '", $$tref, "' );\n");
} else {
- $self->_add_body_code("\$\$_outbuf .= '", $$tref, "';\n");
+ $self->_add_body_code("\$m->{stack}->append('", $$tref, "');\n");
}
$self->{current_compile}{last_body_code_type} = 'text';
@@ -506,7 +506,7 @@
# output defined bits, which is what $m->print does internally
# as well. use 'if defined' for maximum efficiency; grep
# creates a list.
- $code = "for ( $text ) { \$\$_outbuf .= \$_ if defined }\n";
+ $code = "\$m->{stack}->append( $text );\n";
}
eval { $self->postprocess_perl->(\$code) } if $self->postprocess_perl;
@@ -550,8 +550,6 @@
push @{ $c->{comp_with_content_stack} }, $call;
my $code = "\$m->comp( { content => sub {\n";
- $code .= $self->_set_buffer();
-
eval { $self->postprocess_perl->(\$code) } if $self->postprocess_perl;
compiler_error $@ if $@;
Index: lib/HTML/Mason/Request.pm
===================================================================
--- lib/HTML/Mason/Request.pm (revision 3897)
+++ lib/HTML/Mason/Request.pm (working copy)
@@ -41,18 +41,16 @@
use HTML::Mason::Tools qw(can_weaken read_file compress_path load_pkg pkg_loaded absolute_comp_path);
use HTML::Mason::Utils;
use Class::Container;
+use String::BufferStack;
use base qw(Class::Container);
# Stack frame constants
use constant STACK_COMP => 0;
use constant STACK_ARGS => 1;
-use constant STACK_BUFFER => 2;
-use constant STACK_MODS => 3;
-use constant STACK_PATH => 4;
-use constant STACK_BASE_COMP => 5;
-use constant STACK_IN_CALL_SELF => 6;
-use constant STACK_BUFFER_IS_FLUSHABLE => 7;
-use constant STACK_HIDDEN_BUFFER => 8;
+use constant STACK_MODS => 2;
+use constant STACK_PATH => 3;
+use constant STACK_BASE_COMP => 4;
+use constant STACK_IN_CALL_SELF => 5;
# HTML::Mason::Exceptions always exports rethrow_exception() and isa_mason_exception()
use HTML::Mason::Exceptions( abbr => [qw(error param_error syntax_error
@@ -126,6 +124,9 @@
default => sub { print STDOUT $_[0] },
descr => "A subroutine or scalar reference through which all output will pass" },
+ stack =>
+ { isa => 'String::BufferStack', descr => 'The buffer stack for string appends', optional => 1},
+
# Only used when creating subrequests
parent_request =>
{ isa => __PACKAGE__,
@@ -189,8 +190,6 @@
dhandler_arg => undef,
execd => 0,
initialized => 0,
- stack => [],
- top_stack => undef,
wrapper_chain => undef,
wrapper_index => undef,
notes => {},
@@ -408,19 +407,15 @@
#
local $HTML::Mason::Commands::m = $self;
- # Dynamically scoped global pointing at the top of the request stack.
- #
- $self->{top_stack} = undef;
-
# Save context of subroutine for use inside eval.
my $wantarray = wantarray;
my @result;
- # Initialize output buffer to interpreter's preallocated buffer
- # before clearing, to reduce memory reallocations.
- #
- $self->{request_buffer} = $self->interp->preallocated_output_buffer;
- $self->{request_buffer} = '';
+ # Stackmaker, stackmaker, make me a stack
+ $self->{stack} ||= String::BufferStack->new(
+ prealloc => $self->{interp}{buffer_preallocate_size},
+ out_method => $self->out_method,
+ );
eval {
# Build wrapper chain and index.
@@ -450,7 +445,7 @@
tie *SELECTED, 'Tie::Handle::Mason';
my $old = select SELECTED;
- my $mods = {base_comp => $request_comp, store => \($self->{request_buffer})};
+ my $mods = {base_comp => $request_comp};
if ($self->{has_plugins}) {
my $context = bless
@@ -480,7 +475,7 @@
if ($self->{has_plugins}) {
# plugins called in reverse order when exiting.
my $context = bless
- [$self, $request_args, \$self->{request_buffer}, $wantarray, \@result, \$error],
+ [$self, $request_args, $self->{stack}->output_buffer_ref, $wantarray, \@result, \$error],
'HTML::Mason::Plugin::Context::EndRequest';
eval {
foreach my $plugin_instance (@{$self->{plugin_instances_reverse}}) {
@@ -508,12 +503,7 @@
return;
}
- # If there's anything in the output buffer, send it to out_method.
- # Otherwise skip out_method call to avoid triggering side effects
- # (e.g. HTTP header sending).
- if (length($self->{request_buffer}) > 0) {
- $self->out_method->($self->{request_buffer});
- }
+ $self->{stack}->flush_output;
# Return aborted value or result.
@result = ($err->aborted_value) if $self->aborted($err);
@@ -796,7 +786,7 @@
sub cache_self {
my ($self, %options) = @_;
- return if $self->{top_stack}->[STACK_IN_CALL_SELF]->{'CACHE_SELF'};
+ return if $self->{stack}->data->[STACK_IN_CALL_SELF]->{'CACHE_SELF'};
my (%store_options, %retrieve_options);
my ($expires_in, $key, $cache);
@@ -875,7 +865,7 @@
# by $m->cache_self and <%filter> sections respectively.
#
$tag ||= 'DEFAULT';
- my $top_stack = $self->{top_stack};
+ my $top_stack = $self->{stack}->data;
$top_stack->[STACK_IN_CALL_SELF] ||= {};
return if $top_stack->[STACK_IN_CALL_SELF]->{$tag};
local $top_stack->[STACK_IN_CALL_SELF]->{$tag} = 1;
@@ -895,7 +885,8 @@
$retval ||= \$dummy;
# Temporarily put $output in place of the current top buffer.
- local $top_stack->[STACK_BUFFER] = $output;
+
+ $self->{stack}->push(buffer => $output);
# Call the component again, capturing output, return value and
# error. Don't catch errors unless the error reference was specified.
@@ -912,6 +903,8 @@
$comp->run(@$args);
}
};
+ $self->{stack}->pop;
+
if ($@) {
if ($error) {
$$error = $@;
@@ -1008,7 +1001,7 @@
#
sub depth
{
- return scalar @{ $_[0]->{stack} };
+ return $_[0]->{stack}->data_depth;
}
#
@@ -1024,7 +1017,7 @@
my ($self, $path, $current_comp, $error, $exists_only) = @_;
return undef unless defined($path);
- $current_comp ||= $self->{top_stack}->[STACK_COMP];
+ $current_comp ||= $self->{stack}->data->[STACK_COMP];
return $self->_fetch_comp($path, $current_comp, $error)
unless $self->{use_internal_component_caches};
@@ -1184,19 +1177,8 @@
{
my $self = shift;
- # $self->{top_stack} is always defined _except_ in the case of a
- # call to print inside a start-/end-request plugin.
- my $bufref =
- ( defined $self->{top_stack}
- ? $self->{top_stack}->[STACK_BUFFER]
- : \$self->{request_buffer}
- );
+ $self->{stack}->append(@_);
- # use 'if defined' for maximum efficiency; grep creates a list.
- for ( @_ ) {
- $$bufref .= $_ if defined;
- }
-
$self->flush_buffer if $self->{autoflush};
}
@@ -1227,7 +1209,7 @@
or error($error || "could not find component for path '$path'\n");
}
- # Increment depth and check for maximum recursion. Depth starts at 1.
+ # Check for maximum recursion. Depth starts at 1.
#
my $depth = $self->depth;
error "$depth levels deep in component stack (infinite recursive call?)\n"
@@ -1236,23 +1218,21 @@
# Keep the same output buffer unless store modifier was passed. If we have
# a filter, put the filter buffer on the stack instead of the regular buffer.
#
- my $filter_buffer = '';
- my $top_buffer = defined($mods{store}) ? $mods{store} : $self->{top_stack}->[STACK_BUFFER];
- my $stack_buffer = $comp->{has_filter} ? \$filter_buffer : $top_buffer;
- my $flushable = exists $mods{flushable} ? $mods{flushable} : 1;
+ my @args;
+ push @args, buffer => $mods{store} if $mods{store};
+ push @args, filter => sub {$comp->filter ? $comp->filter->(@_) : ""} if $comp->has_filter;
- # Add new stack frame and point dynamically scoped $self->{top_stack} at it.
- push @{ $self->{stack} },
- [ $comp, # STACK_COMP
- \@_, # STACK_ARGS
- $stack_buffer, # STACK_BUFFER
- \%mods, # STACK_MODS
- $path, # STACK_PATH
- undef, # STACK_BASE_COMP
- undef, # STACK_IN_CALL_SELF
- $flushable, # STACK_BUFFER_IS_FLUSHABLE
- ];
- local $self->{top_stack} = $self->{stack}->[-1];
+ # Add new stack frame
+ $self->{stack}->push(
+ @args,
+ data => [
+ $comp, # STACK_COMP
+ \@_, # STACK_ARGS
+ \%mods, # STACK_MODS
+ $path, # STACK_PATH
+ undef, # STACK_BASE_COMP
+ ],
+ );
# Run start_component hooks for each plugin.
#
@@ -1287,23 +1267,10 @@
};
my $error = $@;
- # Run component's filter if there is one, and restore true top buffer
- # (e.g. in case a plugin prints something).
- #
- if ($comp->{has_filter}) {
- # We have to check $comp->filter because abort or error may
- # occur before filter gets defined in component. In such cases
- # there should be no output, but should look into this more.
- #
- if (defined($comp->filter)) {
- $$top_buffer .= $comp->filter->($filter_buffer);
- }
- $self->{top_stack}->[STACK_BUFFER] = $top_buffer;
- }
-
# Run end_component hooks for each plugin, in reverse order.
#
if ($self->{has_plugins}) {
+ $self->{stack}->set_filter(undef);
my $context = bless
[$self, $comp, \@_, $wantarray, \@result, \$error],
'HTML::Mason::Plugin::Context::EndComponent';
@@ -1316,7 +1283,7 @@
# This is very important in order to avoid memory leaks, since we
# stick the arguments on the stack. If we don't pop the stack,
# they don't get cleaned up until the component exits.
- pop @{ $self->{stack} };
+ $self->{stack}->pop;
# Repropagate error if one occurred, otherwise return result.
rethrow_exception $error if $error;
@@ -1329,37 +1296,33 @@
sub scomp {
my $self = shift;
my $buf;
- $self->comp({store => \$buf, flushable => 0},@_);
+ $self->comp({store => \$buf},@_);
return $buf;
}
sub has_content {
my $self = shift;
- return defined($self->{top_stack}->[STACK_MODS]->{content});
+ return defined($self->{stack}->data->[STACK_MODS]->{content});
}
sub content {
my $self = shift;
- my $content = $self->{top_stack}->[STACK_MODS]->{content};
+ my $content = $self->{stack}->data->[STACK_MODS]->{content};
return undef unless defined($content);
- # Run the content routine with the previous stack frame active and
- # with output going to a new buffer.
- #
my $err;
- my $buffer;
- my $save_frame = pop @{ $self->{stack} };
- {
- local $self->{top_stack} = $self->{stack}[-1];
- local $self->{top_stack}->[STACK_BUFFER] = \$buffer;
- local $self->{top_stack}->[STACK_BUFFER_IS_FLUSHABLE] = 0;
- local $self->{top_stack}->[STACK_HIDDEN_BUFFER] = $save_frame->[STACK_BUFFER];
- eval { $content->(); };
- $err = $@;
- }
- push @{ $self->{stack} }, $save_frame;
+ # We don't push a data section, so this is transparent to the data stack
+ $self->{stack}->push( private => 1 );
+ my $save = pop(@{$self->{stack}->data_ref});
+ eval { $content->(); };
+
+ $err = $@;
+
+ push(@{$self->{stack}->data_ref}, $save);
+ my $buffer = $self->{stack}->pop;
+
rethrow_exception $err;
# Return the output from the content routine.
@@ -1380,39 +1343,14 @@
sub clear_buffer
{
my $self = shift;
-
- foreach my $frame (@{$self->{stack}}) {
- my $bufref = $frame->[STACK_BUFFER];
- $$bufref = '';
- $bufref = $frame->[STACK_HIDDEN_BUFFER];
- $$bufref = '' if $bufref;
- }
+ $self->{stack}->clear;
}
sub flush_buffer
{
my $self = shift;
- $self->out_method->($self->{request_buffer})
- if length $self->{request_buffer};
- $self->{request_buffer} = '';
-
- if ( $self->{top_stack}->[STACK_BUFFER_IS_FLUSHABLE]
- && $self->{top_stack}->[STACK_BUFFER] )
- {
- my $comp = $self->{top_stack}->[STACK_COMP];
- if ( $comp->has_filter()
- && defined $comp->filter() )
- {
- $self->out_method->
- ( $comp->filter->( ${ $self->{top_stack}->[STACK_BUFFER] } ) );
- }
- else
- {
- $self->out_method->( ${ $self->{top_stack}->[STACK_BUFFER] } );
- }
- ${$self->{top_stack}->[STACK_BUFFER]} = '';
- }
+ $self->{stack}->flush_output;
}
sub request_args
@@ -1465,7 +1403,7 @@
$index = $depth-1 - $levels;
}
return if $index < 0 or $index >= $depth;
- return $self->{stack}->[$index];
+ return $self->{stack}->data($index);
}
# Return all stack frames, in order from the top of the stack to the
@@ -1474,24 +1412,25 @@
my ($self) = @_;
my $depth = $self->depth;
- return reverse map { $self->{stack}->[$_] } (0..$depth-1);
+ return reverse map { $self->{stack}->data($_) } (0..$depth-1);
}
#
# Accessor methods for top of stack elements.
#
-sub current_comp { return $_[0]->{top_stack}->[STACK_COMP] }
-sub current_args { return $_[0]->{top_stack}->[STACK_ARGS] }
+sub current_comp { return $_[0]->{stack}->data->[STACK_COMP] }
+sub current_args { return $_[0]->{stack}->data->[STACK_ARGS] }
sub base_comp {
my ($self) = @_;
- return unless $self->{top_stack};
+ my $data = $self->{stack}->data;
+ return unless $data;
- unless ( defined $self->{top_stack}->[STACK_BASE_COMP] ) {
+ unless ( defined $data->[STACK_BASE_COMP] ) {
$self->_compute_base_comp_for_frame( $self->depth - 1 );
}
- return $self->{top_stack}->[STACK_BASE_COMP];
+ return $data->[STACK_BASE_COMP];
}
#
@@ -1502,7 +1441,7 @@
my ($self, $frame_num) = @_;
die "Invalid frame number: $frame_num" if $frame_num < 0;
- my $frame = $self->{stack}->[$frame_num];
+ my $frame = $self->{stack}->data($frame_num);
unless (defined($frame->[STACK_BASE_COMP])) {
my $mods = $frame->[STACK_MODS];
@@ -1517,7 +1456,7 @@
($comp->is_subcomp && !$comp->is_method)) {
$base_comp = $self->_compute_base_comp_for_frame($frame_num-1);
} elsif ($path =~ m/(.*):/) {
- my $calling_comp = $self->{stack}->[$frame_num-1]->[STACK_COMP];
+ my $calling_comp = $self->{stack}->data($frame_num-1)->[STACK_COMP];
$base_comp = $self->fetch_comp($1, $calling_comp);
} else {
$base_comp = $comp;
Index: lib/HTML/Mason/Compiler/ToObject.pm
===================================================================
--- lib/HTML/Mason/Compiler/ToObject.pm (revision 3897)
+++ lib/HTML/Mason/Compiler/ToObject.pm (working copy)
@@ -390,7 +390,6 @@
return join '', ( $self->preamble,
$self->_set_request,
- $self->_set_buffer,
$self->_arg_declarations,
$self->_filter,
"\$m->debug_hook( \$m->current_comp->path ) if ( HTML::Mason::Compiler::IN_PERL_DB() );\n\n",
@@ -420,17 +419,6 @@
return 'local $' . $self->in_package . '::m = $HTML::Mason::Commands::m;' . "\n";
}
-sub _set_buffer
-{
- my $self = shift;
-
- if ($self->enable_autoflush) {
- return '';
- } else {
- return 'my $_outbuf = $m->{top_stack}->[HTML::Mason::Request::STACK_BUFFER];' . "\n";
- }
-}
-
my %coercion_funcs = ( '@' => 'HTML::Mason::Tools::coerce_to_array',
'%' => 'HTML::Mason::Tools::coerce_to_hash',
);
Index: lib/HTML/Mason/Interp.pm
===================================================================
--- lib/HTML/Mason/Interp.pm (revision 3897)
+++ lib/HTML/Mason/Interp.pm (working copy)
@@ -118,7 +118,6 @@
data_dir
dynamic_comp_root
object_file_extension
- preallocated_output_buffer
preloads
resolver
source_cache
@@ -169,11 +168,6 @@
$self->_create_data_subdirs();
$self->_initialize_escapes();
- #
- # Create preallocated buffer for requests.
- #
- $self->{preallocated_output_buffer} = ' ' x $self->buffer_preallocate_size;
-
$self->_set_code_cache_attributes();
#
Index: Build.PL
===================================================================
--- Build.PL (revision 3897)
+++ Build.PL (working copy)
@@ -8,13 +8,14 @@
use File::Spec;
-my %prereq = ( 'Cache::Cache' => 1.00,
- 'Class::Container' => 0.07,
- 'CGI' => 2.46,
- 'Exception::Class' => 1.15,
- 'File::Spec' => 0.8,
- 'Params::Validate' => 0.70,
- 'Scalar::Util' => 1.01,
+my %prereq = ( 'Cache::Cache' => 1.00,
+ 'Class::Container' => 0.07,
+ 'CGI' => 2.46,
+ 'Exception::Class' => 1.15,
+ 'File::Spec' => 0.8,
+ 'Params::Validate' => 0.70,
+ 'Scalar::Util' => 1.01,
+ 'String::BufferStack' => 1.00,
);
my $is_dist = grep { /dist=1/ } @ARGV;
--=-Kq3Lf/TGO4zifOrQ+dnP
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
-------------------------------------------------------------------------
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=/
--=-Kq3Lf/TGO4zifOrQ+dnP
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
Mason-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mason-devel
--=-Kq3Lf/TGO4zifOrQ+dnP--