Two patches that may break things...

[Matt Sergeant <ma...@sergeant.org>]

I'm looking for committer votes on the two patches below.

I wanted to test whether:

   $r->send_fd($cache_fh);
   return OK;

would be faster than what we currently use, which is:

   $r->filename($cache_filename);
   return DECLINED;

While debugging this I discovered that we try and send the cache twice.  
That is, $Cache->deliver gets called twice in a cached request. At  
least I think it does - I've changed things so many times now I can't  
quite recall :-)

The upshot is that instead of dropping through deep into  
deliver_to_browser, we really should be skipping deliver_to_browser for  
cached delivery.

So I added the following to deliver_to_browser (it made for cleaner  
code to add it there):

diff -u -r1.53 AxKit.pm
--- lib/AxKit.pm        18 Sep 2003 22:12:33 -0000      1.53
+++ lib/AxKit.pm        18 May 2004 18:45:20 -0000
@@ -917,6 +917,9 @@

      AxKit::Debug(4, "delivering to browser");

+    return $result_code if $result_code == DECLINED;
+    return OK if $result_code == DONE;
+
      if (length($r->pnotes('xml_string'))) {
          # ok, data is in xml_string
          AxKit::Debug(4, "Delivering xml_string");

Now I'm slightly worried that this will break something. However only  
slightly as it all made sense to me.

That was patch #1. Can I get votes on applying this please?

======================================================================== 
====

Now I could make send_fd work (it didn't work before for various  
reasons).

So here's the second patch to do that:

diff -u -b -r1.12 Cache.pm
--- lib/Apache/AxKit/Cache.pm   3 Oct 2003 18:38:38 -0000       1.12
+++ lib/Apache/AxKit/Cache.pm   18 May 2004 19:57:54 -0000
@@ -4,7 +4,7 @@
  use strict;

  use Apache;
-use Apache::Constants qw(OK DECLINED SERVER_ERROR);
+use Apache::Constants qw(OK DECLINED SERVER_ERROR DONE);
  use Apache::AxKit::Exception;
  use Digest::MD5 ();
  use Compress::Zlib qw(gzopen);
@@ -156,10 +156,12 @@
  }

  sub get_fh {
-    my $self = shift;
+    my ($self, $gzip) = @_;
      return if $self->{no_cache};
      my $fh = Apache->gensym();
-    if (AxKit::sysopen($fh, $self->{file}, O_RDONLY, 'raw')) {
+    my $file = $self->{file} . ($gzip ? '.gz' : '');
+    AxKit::Debug(3, "Cache: get_fh opening: $file\n");
+    if (AxKit::sysopen($fh, $file, O_RDONLY, 'raw')) {
          flock($fh, LOCK_SH);
          return $fh;
      }
@@ -223,7 +225,6 @@
          $r->content_type($AxKit::OrigType);
      }

-
      my ($transformer, $doit) = AxKit::get_output_transformer();

      if ($doit) {
@@ -240,13 +241,17 @@

          if ($self->{gzip} && $AxKit::Cfg->DoGzip) {
              AxKit::Debug(4, 'Cache: Delivering gzipped output');
-            $r->filename($self->{file}.'.gz');
+            my $fh = $self->get_fh(1);
+            $r->send_fd($fh);
          }
          else {
-            $r->filename($self->{file});
+            my $fh = $self->get_fh();
+            AxKit::Debug(3, "Cache: send_fd($fh)\n");
+            my $bytes_sent = $r->send_fd($fh);
+            AxKit::Debug(3, "Cache: sent $bytes_sent bytes\n");
          }

-        return DECLINED;
+        return DONE;
      }

  }

I have to return DONE instead of DECLINED so that deliver_to_browser  
translates that into OK. This is the only really messy bit of this  
patch set - how do you bypass deliver_to_browser but still get OK sent  
back to apache? So I had to use this special return code. This is kinda  
ugly, and if anyone has a better idea I'd be glad to see it.

The upside of this patch is it seems to be faster than dropping back to  
apache and letting the next stage deliver the cached file. These are  
the simple benchmark results I got (delivering a very small cached  
transformation):

   Current CVS: 95 req/s
   Patch #1   : 98 req/s
   Patch 1+2 : 113 req/s

Clearly this is faster. Good. So what's the downside?

Well I can imagine this method doesn't set all the right headers. I  
need to look into this more - am I now missing things like Etags and  
other stuff I might like to have in there? Then again, maybe we could  
still get a speedup by creating those headers myself.

Can anyone else see other downsides?

Vote now!

Re: Two patches that may break things...

[Matt Sergeant <ma...@sergeant.org>]

On Tue, 18 May 2004, Matt Sergeant wrote:

> So I added the following to deliver_to_browser (it made for cleaner
> code to add it there):
>
> diff -u -r1.53 AxKit.pm
> --- lib/AxKit.pm        18 Sep 2003 22:12:33 -0000      1.53
> +++ lib/AxKit.pm        18 May 2004 18:45:20 -0000
> @@ -917,6 +917,9 @@
>
>       AxKit::Debug(4, "delivering to browser");
>
> +    return $result_code if $result_code == DECLINED;
> +    return OK if $result_code == DONE;
> +
>       if (length($r->pnotes('xml_string'))) {
>           # ok, data is in xml_string
>           AxKit::Debug(4, "Delivering xml_string");
>
> Now I'm slightly worried that this will break something. However only
> slightly as it all made sense to me.

I've removed the DONE line above from my patch as it turns out that the
cache change works out slower once you manually send the right headers.

Though this still leaves me asking whether this will cause problems.

The potential problem comes if the developer returns DECLINED from a
transformation (XSP or XPathScript only I guess), but expects his output
to actually go to the browser. Can anyone think of when or why that might
happen?

> That was patch #1. Can I get votes on applying this please?

I'm not going to wait for feedback or votes any more. So if nobody talks
I'm just going to apply.

Matt.