Bug 53352

Summary: Heavy external fragmentation in FixedVMPoolAllocator can lead to a CRASH().
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, barraclough, ddkilzer, eric, ggaren, ossy, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 53401    
Bug Blocks:    

Description Gavin Barraclough 2011-01-28 16:29:33 PST
The FixedVMPoolAllocator currently uses a best fix policy - switch to first fit, this is less prone to external fragmentation.
Comment 1 Gavin Barraclough 2011-01-28 16:35:30 PST
fixed in r77025
Comment 2 Gavin Barraclough 2011-01-28 16:58:41 PST
Hey Xan,

We've had to re-write the FixedVMPoolAllocator, to fix fragmentation problems.  In the new implementation the type of the internal structure needs to change to support different sizes, which is going to increase the complexity of dynamically changing behavior depending whether overcommit is available.  Since I'm not testing on Linux I've decided it best just remove this for now, and cap Linux to 32MB of JIT buffers on all platforms.

Please feel free to look at reintroducing the dynamic check if you wish, I think it'll probably mean selecting at runtime between two sets of page tables - I'll leave this in your hands.

cheers,
G.
Comment 3 WebKit Review Bot 2011-01-28 21:14:26 PST
http://trac.webkit.org/changeset/77025 might have broken Leopard Intel Debug (Tests)
Comment 4 Csaba Osztrogon√°c 2011-01-30 11:57:47 PST
r77025 made js1_5/Regress/regress-159334.js fail on 64 bit Linux (GTK and Qt).

It was rolled out by http://trac.webkit.org/changeset/77090 .
Comment 5 David Kilzer (:ddkilzer) 2011-01-30 12:18:36 PST
<rdar://problem/8935574>
Comment 6 Geoffrey Garen 2011-01-30 18:28:52 PST
js1_5/Regress/regress-159334.js tests calling a giant function.
Comment 7 Gavin Barraclough 2011-01-30 23:39:15 PST
I think the fix here will be to disable these tests on Linux.  The problem is that Linux does not support VM overcommit, since on x86-64 we need to allocate all VM in a single allocation, we have two choices on x86-64 linux:
(1) commit a huge amount of physical RAM / swap up front
(2) opt not to be able to run crazily huge functions.
I think we have to pick the latter.  Will look at this tomorrow.
Comment 8 Gavin Barraclough 2011-01-31 11:19:07 PST
Actually, there are valid options, so I should leave this decision to the Linux folks.  I'm going to roll us back to the state prior to 75709 (see bug #42756), which is to say that on x86-64 we require 1GB of VM to be allocated up front, so we will not necessarily be able to run on !swap !overcommit configurations.

Someone with some skin in the game on Linux can choose between:
(1) Requiring 1GB VM.  This will lead to poor support for !swap !overcommit configurations.
(2) Dropping to ~32MB VM for all Linux/x86-64 configurations - this will mean we are unable to run crazily large test cases.
(3) Provide a compile time option to select between the above.
(4) Provide a runtime option to select between the above.

Relanded in r77145.
Comment 9 Geoffrey Garen 2011-01-31 11:44:47 PST
I'd suggest doing (2), since it's easier to keep a single, predictable configuration working and tested -- and the more complicated the Linux setup is, the less likely it is to stay working. In order to do (2), you'll also need to configure the Linux bot to skip js1_5/Regress/regress-159334.js.
Comment 10 Xan Lopez 2011-01-31 11:56:57 PST
(In reply to comment #9)
> I'd suggest doing (2), since it's easier to keep a single, predictable configuration working and tested -- and the more complicated the Linux setup is, the less likely it is to stay working. In order to do (2), you'll also need to configure the Linux bot to skip js1_5/Regress/regress-159334.js.

On the other hand it is sad that we are doing this in the setup theoretically more likely to have lots of memory available. It's up to figure out some sane alternative though, so it's OK to do that for the time being as far as I'm concerned.