Bug 106830 – Use MADV_FREE_REUSABLE to return JIT memory to OS

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Pratik Solanki]

On OS X versions that support it, use MADV_FREE_REUSABLE to return JIT memory back to OS.

[Pratik Solanki]

<rdar://problem/11437701>

[Pratik Solanki]

This basically undoes <http://trac.webkit.org/changeset/116593> made in <http://bugs.webkit.org/show_bug.cgi?id=86047>.

[Pratik Solanki]

Created attachment 182632 [details]
Patch

[WebKit Review Bot]

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

[Geoffrey Garen]

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.

[Pratik Solanki]

(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.

[Geoffrey Garen]

> 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.

[Pratik Solanki]

Created attachment 182859 [details]
Patch

[Geoffrey Garen]

Comment on attachment 182859 [details]
Patch

r=me

[Pratik Solanki]

Committed r139812: <http://trac.webkit.org/changeset/139812>

[Darin Adler]

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!

[Geoffrey Garen]

> 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.

[Pratik Solanki]

(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.