search

apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 365 forks source link

potential memory leak #120

Closed jeffkaufman closed 11 years ago

jeffkaufman commented 11 years ago

Running ngx_pagespeed on jefftk.com (a tiny VPS) for several weeks, I noticed yesterday that it was using a lot (~200MB) of memory, enough that the system was swapping. I need to figure out whether we're leaking memory or whether I just have it set to use more memory than is available on this server.

oschaaf commented 11 years ago

I think I trapped a leak in the custom options path.

It is reproducible on my machine with ab -n 100000 -c 10 -k 127.0.0.1/mod_pagespeed_example/?ModPagespeedFilters=trim_urls

Do you see the same behaviour? This seems to leak quite fast, and might be it?

jeffkaufman commented 11 years ago

I see it too; looking into it.

jeffkaufman commented 11 years ago

I'm not sure the problem is limited to custom options. I also see leaking with ab -n 100000 -c 10 -k 'http://localhost:8050/mod_pagespeed_example/

jeffkaufman commented 11 years ago

@oschaaf https://github.com/pagespeed/ngx_pagespeed/pull/121

jeffkaufman commented 11 years ago

@oschaaf I just tried instrumenting the ctor and dtor for NgxRewriteOptions for a quick run of ab -n 100000 -c 10 -k 'http://localhost:8050/mod_pagespeed_example/?ModPagespeedFilters=trim_urls. Before I interrupted it, it had made 374 ctor calls and 190 dtor calls. 374 is close enough to being twice 190 that I suspect each request is making us create two NgxRewriteOptions instances, and we're only destroying one of them properly. Still looking into it.

oschaaf commented 11 years ago

I have not proved this yet, but I had half a look last night: I think we should delete the request_options returned from ps_determine_request_options after merging them

jeffkaufman commented 11 years ago

@oschaaf "I think we should delete the request_options returned from ps_determine_request_options after merging them"

Yes: https://github.com/pagespeed/ngx_pagespeed/pull/123

jeffkaufman commented 11 years ago

@oschaaf: https://github.com/pagespeed/ngx_pagespeed/pull/124

jeffkaufman commented 11 years ago

We've fixed two leaks, and valgrind output looks good (no invalid memory operations or definitely lost bytes). I'm going to close this bug, and if we see more memory problems we can make a new bug.