Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow transparent compression #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

vlovich
Copy link

@vlovich vlovich commented Jul 26, 2012

this is a fairly hard-coded version but works as a proof of concept; would love to have feedback on how to fix this up to be mainlined.

https://wifislam.wordpress.com/2012/07/26/apache-sendfile-and-static-files/ for the analysis behind this patch.

Vitali Lovich added 3 commits July 26, 2012 00:32
limitations:

list of supported extensions is hard-coded
cached compressed file lives where the source file lives (i.e. not a
separate cache directory).  this means that apache must be able to write
there
cleanup some logic + add back support for Content-Length
@nmaier
Copy link
Owner

nmaier commented Jul 26, 2012

I'm not interested in hacking gzip into mod_xsendfile.

What would be a viable option instead is using mod_deflate and making sure it works in tandem with mod_xsendfile. Right now that does not seem to be the case, as the XSENDFILE filter is inserted after all other filters.
However something along the lines of the following should be possible (pseudo code)

static void ap_xsendfile_insert_output_filter(request_rec *r) {
  ...
  ap_filter_t *deflate = ap_get_output_filter_handle("DEFLATE");
  if (deflate) {
     if (deflate in r->output_filters) {
       remove deflate from r->output_filters; /* to be re-inserted later */
     }
     else {
       deflate = NULL; /* should not be (re-)inserted later */
    }
  )

  /* current implementation */
  ap_add_output_filter(
    "XSENDFILE",
    NULL,
    r,
    r->connection
    );

  /* reinsert filter behind XSENDFILE again */
  if (deflate) {
    ap_add_output_filter_handle(
      deflate,
      NULL,
      r,
      r->connection
      );
  }
}

@vlovich
Copy link
Author

vlovich commented Jul 26, 2012

I might be misunderstanding what you're saying, but i don't think that would work because xsendfile passes the file directly to the OS which sends it to the socket. Meaning there's no chance for deflate to run properly; either deflate's output would be dropped on the floor or both deflate and xsendfile would send a response back to the user. In any case, you wouldn't want deflate to run since that means your not only compressing the same static content over and over again (wasting CPU cycles) but you're also copying the static files into user space to compress them in the first place, which defeats the behavior mod_xsendfile is trying to achieve in the first place, right?

On Jul 26, 2012, at 6:28 AM, Nils [email protected] wrote:

I'm not interested in hacking gzip into mod_xsendfile.

What would be a viable option instead is using mod_deflate and making sure it works in tandem with mod_xsendfile. Right now that does not seem to be the case, as the XSENDFILE filter is inserted after all other filters.
However something along the lines of the following should be possible (pseudo code)

static void ap_xsendfile_insert_output_filter(request_rec *r) {
 ...
 ap_filter_t *deflate = ap_get_output_filter_handle("DEFLATE");
 if (deflate) {
    if (deflate in r->output_filters) {
      remove deflate from r->output_filters; /* to be re-inserted later */
    }
    else {
      deflate = NULL; /* should not be (re-)inserted later */
   }
 )

 /* current implementation */
 ap_add_output_filter(
   "XSENDFILE",
   NULL,
   r,
   r->connection
   );

 /* reinsert filter behind XSENDFILE again */
 if (deflate) {
   ap_add_output_filter_handle(
     deflate,
     NULL,
     r,
     r->connection
     );
 }
}

Reply to this email directly or view it on GitHub:
#19 (comment)

@nmaier
Copy link
Owner

nmaier commented Jul 26, 2012

tl;dr
Hooking into mod_deflate should be possible and is something I'm willing to pull or implement, pre-compressing stuff in mod_xsendfile is out of the module scope, but I would gladly pull or implement hooks into a dedicated module that does just that.

but i don't think that would work because xsendfile passes the file directly to the OS which sends it to the socket. Meaning there's no chance for deflate to run properly; either deflate's output would be dropped on the floor or both deflate and xsendfile would send a response back to the user.

Actually, mod_xsendfile only prepares the data structure that the server might use to pass over data to the socket or might instead pass on to the next filter in line (although, the current code kinda forces XSENDFILE to be the last filter ATM).
The code that does the preparation is more or less a direct copy from the httpd core code handling static files, which may later passed through mod_deflate, so that should work.

I'm pretty sure mod_xsendfile can be made working in tadem with mod_deflate.

In any case, you wouldn't want deflate to run since that means your not only compressing the same static content over and over again (wasting CPU cycles) but you're also copying the static files into user space to compress them in the first place, which defeats the behavior mod_xsendfile is trying to achieve in the first place, right?

Yep, that assessment is mostly right.

  • Having additional buffers and having to pass the data through user land is always a problem if you have to transform/modify the data from the file into something else. E.g. mod_deflate transparent compression or user land SSL engines.
  • mod_deflate indeed "wastes" additional CPU circles repeating a task again and again, at least when it comes to long-lived (static) content. However, some people find this is a good enough trade-off compared to the additional complexity and storage requirements and (remote) file system performance of storing pre-compressed files.

Having that said, mod_xsendfile + mod_deflate would still perform better with less buffers involved than having php or whatever read the file into a VM buffer, transforming it, copying it over to an apache buffer (or network buffer, if run outside of the apache process e.g. via fastcgi/wsgi) and having apache serve that.

The code from your pull request might actually be a lot better for your use case, however I wouldn't merge it because:

  • Most importantly, compression is out of the context of mod_xsendfile. It is my opinion that such pre-compression should be performed by the app that sends the code in the first place, or by another dedicated module (I would accept code that hooks into such a module).
  • The code is very posix'y. This is a huge problem because the code has to work on Windows and should use apr.
  • The code forks and execv (a hardcoded path).
  • The gzip cache needs to be better tunable and should allow to limit the space it may use.

The first point is a show-stopper... Hence, I'd consider the later point my input for designing a dedicated module.

@vlovich
Copy link
Author

vlovich commented Jul 27, 2012

Maybe I wasn't clear, but I definitely wasn't expecting this to be taken as is (as you point out & the blog post mentions, there's all sorts of problems with the code as is). It was more of a here's one approach I took, what do you think the approach should be kind of pull request.

@nmaier
Copy link
Owner

nmaier commented Jul 27, 2012

As I stated, I'd be willing to pull any integration with mod_deflate or any other compression module[1], but I'm not willing to integrate actual gzipping of any kind into mod_xsendfile, as this would be out of scope of this module.
Also integrating with mod_negotiation, which is typically used to conditionally serve plain or .gz files, would be a pull I'd accept.

@jmwebservices
Copy link

I just wanted to link to a few solutions I recommended for this module's compression problem. #36 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants