| Summary: | Use MADV_FREE_REUSABLE to return JIT memory to OS | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||
| Component: | JavaScriptCore | Assignee: | Pratik Solanki <psolanki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | barraclough, dglazkov, fpizlo, ggaren, oliver, psolanki, webkit.review.bot | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Pratik Solanki
2013-01-14 14:46:29 PST
This basically undoes <http://trac.webkit.org/changeset/116593> made in <http://bugs.webkit.org/show_bug.cgi?id=86047>. Created attachment 182632 [details]
Patch
Comment on attachment 182632 [details] Patch Attachment 182632 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15867662 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html Comment on attachment 182632 [details]
Patch
Thanks for tackling this.
This patch has a bug: notifyNeedPage() forgets to MADV_FREE_REUSE.
Let me suggest another approach, which also fixes the bug:
(a) Make a USE(MADV_FREE_FOR_JIT_MEMORY) Platform.h #define, with a comment about MADV_FREE_REUSABLE not working on older OS's.
(b) Set that to true on older OS's
(c) Change "#if OS(DARWIN)" to "#if USE(MADV_FREE_FOR_JIT_MEMORY)" in this file.
(In reply to comment #5) > (From update of attachment 182632 [details]) > Thanks for tackling this. > > This patch has a bug: notifyNeedPage() forgets to MADV_FREE_REUSE. Good catch. I'll fix this. > Let me suggest another approach, which also fixes the bug: > > (a) Make a USE(MADV_FREE_FOR_JIT_MEMORY) Platform.h #define, with a comment about MADV_FREE_REUSABLE not working on older OS's. > (b) Set that to true on older OS's > (c) Change "#if OS(DARWIN)" to "#if USE(MADV_FREE_FOR_JIT_MEMORY)" in this file. Generally, I hate adding more ifdefs to Platform.h since they cause full world builds. And in this case it feels even more painful since the ifdef will be used only in one source file. Do we have a precedent for using #if local to just one source file? Otherwise I can add it in Platform.h like you suggest. > Generally, I hate adding more ifdefs to Platform.h since they cause full world builds. And in this case it feels even more painful since the ifdef will be used only in one source file. Do we have a precedent for using #if local to just one source file?
I think putting the #if inside this file, instead of Platform.h, is probably fine in this case. There's a small precedent for that in FastMalloc.cpp. (Once things start to span multiple files, Platform.h is probably best, to avoid surprises.)
Also, let's USE(MADV_FREE_FOR_JIT_MEMORY) for PLATFORM(IOS) as well, as we discussed.
Created attachment 182859 [details]
Patch
Comment on attachment 182859 [details]
Patch
r=me
Committed r139812: <http://trac.webkit.org/changeset/139812> Comment on attachment 182859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182859&action=review > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:44 > +// MADV_FREE_REUSABLE does not work for JIT memory on older OSes so use MADV_FREE in that case. I don’t understand the mention of MADV_FREE_REUSABLE here. I don’t see any code in this file using MADV_FREE_REUSABLE! > I don’t understand the mention of MADV_FREE_REUSABLE here. I don’t see any code in this file using MADV_FREE_REUSABLE!
MADV_FREE_REUSABLE is the default implementation of PageReservation::commit() and PageReservation::decommit(). Maybe the #ifdef should have been about those functions instead.
(In reply to comment #12) > > I don’t understand the mention of MADV_FREE_REUSABLE here. I don’t see any code in this file using MADV_FREE_REUSABLE! > > MADV_FREE_REUSABLE is the default implementation of PageReservation::commit() and PageReservation::decommit(). Maybe the #ifdef should have been about those functions instead. Thats right. I guess the comment could have been clearer. Maybe something like "PageReservation::decommit() uses MADV_FREE_REUSABLE which does not work with JIT memory on older OSes. Use MADV_FREE in such cases." I can make that change if that sounds good. |