Automatically enable gzip compression #238

Closed
jeffkaufman opened this Issue Apr 7, 2013 · 22 comments

Comments

Projects
None yet
7 participants
Contributor

jeffkaufman commented Apr 7, 2013

mod_pagespeed turns on gzip. In ngx_pagespeed we should do this too.

So, in case like ours, where our web heads are running gzip as well, this would just be overkill right? our web heads shouldn't compress, and ngx_pagespeed does?

Owner

igrigorik commented Apr 8, 2013

@vidluther I think it makes sense to gzip as early as possible, since that also reduces transfer sizes within your own infrastructure. Hence, yeah, compress at the origin, or in this case, at ngx_pagespeed.

So, compress on the web server, and then compress at the nginx servers acting as a reverse proxy/load balancer via ngx_pagespeed as well?

So, in the words of Xzibit..

Yo dawg, I compressed your output inside a compressed output?

Owner

igrigorik commented Apr 8, 2013

Err.. may have misinterpreted your comment. PageSpeed will have to decompress any incoming data to rewrite it (obviously). Whether that actually makes sense to do - in terms of spending cycles on both ends - depends on the actual size of response, link speed between your reverse proxy and the application generating the markup, etc. There is not necessarily a "one size fits all" here. Having said that, if ngx_pagespeed is running on same server as app server, then yeah, no reason to enable compression in both places, as you'll be burning CPU three times over on compression/decompression.

Contributor

jeffkaufman commented Apr 8, 2013

(This should probably come with an option "pagespeed EnableGzip off", but gzip-on is a much better default.)

k0nsl commented Apr 8, 2013

I like this idea, Mr. Kaufman.

On Mon 08 Apr 2013 04:19:41 PM CEST, Jeff Kaufman wrote:

(This should probably come with an option "pagespeed EnableGzip off",
but gzip-on is a much better default.)


Reply to this email directly or view it on GitHub
#238 (comment).

Contributor

jeffkaufman commented May 13, 2013

What would it take to turn on gzip by default? We need to set gzip to be on, and set gzip types to include many content types.

ngx_http_gzip_conf_t is defined in ngx_http_gzip_filter_module.c, which means that if we want to reach into the nginx configuration we're going to need to duplicate the structure definition. This is risky, because if there are any changes to this structure in the future we amount to randomly setting bytes. (Yikes!)

The alternative to doing this would be to try to use ngx_http_gzip_filter_commands, which has a standard structure. It stores offsetof(ngx_http_gzip_conf_t, enable) and ngx_string("gzip_types") which is what we need. As a check we could verify that they had pointers to ngx_conf_set_flag_slot and ngx_http_types_slot as expected. Any errors here have to be detected runtime, which is unfortunate, but at least we can detect them, and we can bail out without changing anything.

I see how to go from the gzip module's ngx_module_t to the array of ngx_command_t, and then we can pick out the right one by string comparisons on command->name (looking for gzip and gzip_types). We verify by comparing the function pointer for command->set to the function pointers ngx_conf_set_flag_slot and ngx_http_types_slot. If this succeeds we run those functions on the top level (http-block/main) gzip conf, using a fake ngx_conf_t* to pass in the arguments.

Anyone see a better way?

Contributor

jeffkaufman commented May 13, 2013

Thinking more, we can't just enable gzip at the main level. If pagespeed is on for only one server block, we should only turn on gzip for that server block. So at the point where we're deciding what server blocks to apply to we also need to find and modify the gzip configuration for that server block.

Owner

oschaaf commented May 14, 2013

@jeffkaufman
Possibly useful: ps_init_child_process() [1] loops over all server contexts. To do something similar for this, we would probably need to add another ngx_module_t and make that run after gzip, so we can check both our and gzip's configuration (which should both be available at that point in time).

[1] https://github.com/pagespeed/ngx_pagespeed/blob/master/src/ngx_pagespeed.cc#L2757

Owner

oschaaf commented Apr 7, 2014

@keesspoelstra has explored pulling this off at https://github.com/We-Amp/ngx_pagespeed/tree/keesspoelstra-gzip-enable -- looking promising

@oschaaf and @keesspoelstra the pull at https://github.com/We-Amp/ngx_pagespeed/tree/keesspoelstra-gzip-enable looks good - but what about an option to disable for locations / servers where assets are already existing as statically gzip'd.

For example: "pagespeed EnableGzip off"

Less CPU waste and optional when required...

@oschaaf your recent updates to this via We-Amp/ngx_pagespeed@67737e3 make sense:

Via changes to src/ngx_gzip_setter.cc

+  // If these functions are called it means there is an explicit gzip
+  // configuration. The gzip configuration set by pagespeed is then rollbacked
+  // and pagespeed will stop enabling gzip automatically.

I assume this indicates we do have control over 'automatic gzip' on this pull?

Owner

oschaaf commented Apr 7, 2014

@peterbowey "I assume this indicates we do have control over 'automatic gzip' on this pull?"
@keesspoelstra's work on this makes ngx_pagespeed bail out as far as gzip configuration is concerned when explicit gzip configuration is detected. So yes - you have control over it. If you'd add gzip on or gzip off that would work as expected.

@oschaaf Just what I needed to know. Thanks...

Contributor

jeffkaufman commented Apr 9, 2014

Should I look at the automatic gzip code now, or wait for a pull request?

I'm excited about having gzip on by default with ngx_pagespeed.

Owner

oschaaf commented Apr 9, 2014

@jeffkaufman Please wait for a pull, hopefully later today.

@jeffkaufman @oschaaf The 'automatic gzip code' addition works fine for me. I tested this via ngx_pagespeed trunk-tracking and the code patches @ https://github.com/We-Amp/ngx_pagespeed/compare/keesspoelstra-gzip-enable.

The above code recognizes previous gzip configurations, in which case it indicates via the nginx error log an entry of: "Pagespeed: rollback gzip, explicit configuration".

However, I am not so sure this report feature should log to the nginx 'error logs'.

Contributor

keesspoelstra commented Apr 9, 2014

@peterbowey this message should does have level info, how are info level messages normally handled in nginx?
I will also add Pagespeed gzip on/off for location control.

A heads up, I am currently rebasing onto trunk-tracking.

@keesspoelstra Both such changes you outline above - are most welcome. At the moment, I am just too tired (@ 1:54 AM Australia) to recall the 'info' level handling for nginx - via the above.

Later, 'I can do', but I am sure you will solve both events, mean-while. Thanks for a great addition to ngx_pagespeed.

keesspoelstra added a commit to We-Amp/ngx_pagespeed that referenced this issue Apr 10, 2014

gzip: enabling gzip on "pagespeed on"
this adds the following configuration on "pagespeed on"
gzip  on;
gzip_proxied any;
gzip_vary on;
gzip_types application/ecmascript;
gzip_types application/javascript;
gzip_types application/json;
gzip_types application/pdf;
gzip_types application/postscript;
gzip_types application/x-javascript;
gzip_types image/svg+xml;
gzip_types text/css;
gzip_types text/csv;
gzip_types text/html;
gzip_types text/javascript;
gzip_types text/plain;
gzip_types text/xml;
gzip_http_version 1.0;

If an explicit configuration is detected the gzip configuration
set by pagespeed on is rollbacked

Fixes pagespeed#238

@keesspoelstra The update at We-Amp/ngx_pagespeed@a1ea5d9 works just fine.

However, after some testing, I decided to revert to manual gzip pagespeed + nginx configurations - (above this auto gzip method). Why? I could not see a direct benefit on existing given controls I encapsulate in any nginx block or server. If I had thought ngx_* may benefit - but it now seems unlikely.

I assume the given 'auto' gzip is a reasonable time saver 'short-cut' on nginx setups along with pagespeed? Did I miss any 'other' attributes of same?

Contributor

jeffkaufman commented Apr 14, 2014

@peterbowey

I assume the given 'auto' gzip is a reasonable time saver 'short-cut' on nginx setups along with pagespeed?

The goal is to change gzip from opt-in to opt-out. If someone installs pagespeed and knows enough to set a specific gzip configuration then yes, this change is just a time-saving shortcut. But many people installing pagespeed just want it to be a module that does everything it can to make their site load faster, and automatically turning on gzip is pretty helpful for that group.

keesspoelstra added a commit to We-Amp/ngx_pagespeed that referenced this issue Jun 4, 2014

gzip: enabling gzip on "pagespeed on"
this adds the following configuration on "pagespeed on"
gzip  on;
gzip_proxied any;
gzip_vary on;
gzip_types application/ecmascript;
gzip_types application/javascript;
gzip_types application/json;
gzip_types application/pdf;
gzip_types application/postscript;
gzip_types application/x-javascript;
gzip_types image/svg+xml;
gzip_types text/css;
gzip_types text/csv;
gzip_types text/html;
gzip_types text/javascript;
gzip_types text/plain;
gzip_types text/xml;
gzip_http_version 1.0;

If an explicit configuration is detected the gzip configuration
set by pagespeed on is rollbacked

Fixes pagespeed#238

keesspoelstra added a commit to We-Amp/ngx_pagespeed that referenced this issue Jun 11, 2014

this adds the following configuration on "pagespeed on"
gzip on;
gzip_proxied any;
gzip_vary on;
gzip_types application/ecmascript;
gzip_types application/javascript;
gzip_types application/json;
gzip_types application/pdf;
gzip_types application/postscript;
gzip_types application/x-javascript;
gzip_types image/svg+xml;
gzip_types text/css;
gzip_types text/csv;
gzip_types text/html;
gzip_types text/javascript;
gzip_types text/plain;
gzip_types text/xml;
gzip_http_version 1.0;

If any explicit gzip configuration is detected the gzip configuration
set by pagespeed is rolled back completely and the explicit gzip
configuration is used.

To enable/disable gzip with pagespeed the following commands can be used:

pagespeed gzip on;
pagespeed gzip off;

We test the nesting of the gzip configuration (pagespeed gzip on/off and
pagespeed on/off)
Ideally we need to test all the content types as well and the rollback in
case of an explicit gzip configuration, but to do this we need to be able
to map a custom directory into the nginx tests this and seperate nginx
config files.

Fixes pagespeed#238
Contributor

jeffkaufman commented Aug 4, 2014

Fixed by #665

@jeffkaufman jeffkaufman closed this Aug 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment