Bug 154749 – Remove the on demand executable allocator

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Oliver Hunt]

Remove the on demand executable allocator

[Oliver Hunt]

Created attachment 272366 [details]
Patch

[Geoffrey Garen]

Comment on attachment 272366 [details]
Patch

This patch (and its history) will be easier to read if you first make the changes necessary to remove the demand allocator, check that in, and then do any moves you want to do.

[Saam Barati]

Comment on attachment 272366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272366&action=review

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:105
> +                RELEASE_ASSERT_NOT_REACHED(); // In debug mode, this should be a hard failure.

This comment isn't true.
Maybe it should just be ASSERT_NOT_REACHED?
Or we could remove the break on the next line.

[Oliver Hunt]

Created attachment 272380 [details]
Patch

[Geoffrey Garen]

Comment on attachment 272380 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 272380 [details]
Patch

Clearing flags on attachment: 272380

Committed r197226: <http://trac.webkit.org/changeset/197226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Looks like this broke LLINT Cloop build: https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3832/steps/compile-webkit/logs/stdio

[Oliver Hunt]

(In reply to comment #8)
> Looks like this broke LLINT Cloop build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3832/steps/
> compile-webkit/logs/stdio

Ugh I'm a muppet - will fix in a few minutes

[Oliver Hunt]

Uuid fux
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/JavaScriptCore/ChangeLog
	M	Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp
Committed r197256

[Alexey Proskuryakov]

Test regressions too: https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/1607/steps/webkit-32bit-jsc-test/logs/stdio

[Carlos Alberto Lopez Perez]

Why was this removed? It caused OOM crashes: see bug 154822

[Csaba Osztrogonác]

(In reply to comment #12)
> Why was this removed? It caused OOM crashes: see bug 154822

Ping

[WebKit Commit Bot]

Re-opened since this is blocked by bug 154910

[Alexey Proskuryakov]

Rolled out in <http://trac.webkit.org/changeset/197441>.

[Csaba Osztrogonác]

(In reply to comment #12)
> Why was this removed?

Do you still plan to remove on demand executable allocator?
If yes, could you explain what is the reason? If no, let's close this bug.

[Oliver Hunt]

(In reply to comment #16)
> (In reply to comment #12)
> > Why was this removed?
> 
> Do you still plan to remove on demand executable allocator?
> If yes, could you explain what is the reason? If no, let's close this bug.

Because multiple allocators with variable behaviour are bad? Because it makes a number of other features impossible?

You never answered my question in the other bug: does your ARM port support going back and forth between the LLINT and the JIT?

[Csaba Osztrogonác]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #12)
> > > Why was this removed?
> > 
> > Do you still plan to remove on demand executable allocator?
> > If yes, could you explain what is the reason? If no, let's close this bug.
> 
> Because multiple allocators with variable behaviour are bad? Because it
> makes a number of other features impossible?
> 
> You never answered my question in the other bug: does your ARM port support
> going back and forth between the LLINT and the JIT?

If you point me how to d(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #12)
> > > Why was this removed?
> > 
> > Do you still plan to remove on demand executable allocator?
> > If yes, could you explain what is the reason? If no, let's close this bug.
> 
> Because multiple allocators with variable behaviour are bad? Because it
> makes a number of other features impossible?

In this case it would have been better to ask before removing.
 
> You never answered my question in the other bug: does your ARM port support
> going back and forth between the LLINT and the JIT?

https://bugs.webkit.org/show_bug.cgi?id=154822#c7

I haven't found any option to enable fallback on OOM. 
Could you point me where can we switch this on? 

Is it possible to enable this fallback if LLINT is disabled during 
stress test running? (Tests failed only in JSC_useLLINT=false case.)

[Oliver Hunt]

> > You never answered my question in the other bug: does your ARM port support
> > going back and forth between the LLINT and the JIT?
> 
> If you point me how to d(In reply to comment #17)

Do you have the actual llint enabled? Because that should just happen automatically if it can't allocate executable memory.

> > (In reply to comment #16)
> > > (In reply to comment #12)
> > > > Why was this removed?
> > > 
> > > Do you still plan to remove on demand executable allocator?
> > > If yes, could you explain what is the reason? If no, let's close this bug.
> > 
> > Because multiple allocators with variable behaviour are bad? Because it
> > makes a number of other features impossible?
> 
> In this case it would have been better to ask before removing.

Yeah, i talked about it internally but didn't comment in the bug as I assumed it was an obviously worth while change.

>  
> > You never answered my question in the other bug: does your ARM port support
> > going back and forth between the LLINT and the JIT?
> 
> https://bugs.webkit.org/show_bug.cgi?id=154822#c7
> 
> I haven't found any option to enable fallback on OOM. 
> Could you point me where can we switch this on? 
> 
> Is it possible to enable this fallback if LLINT is disabled during 
> stress test running? (Tests failed only in JSC_useLLINT=false case.)

Wait, are you deliberately disabling the llint during stress testing? If you are then your stress test is _correct_ to crash. The LLINT design is deliberately intended to allow for running out of executable memory. That part of the value it provides.

[Csaba Osztrogonác]

(In reply to comment #19)
> Wait, are you deliberately disabling the llint during stress testing? If you
> are then your stress test is _correct_ to crash. The LLINT design is
> deliberately intended to allow for running out of executable memory. That
> part of the value it provides.

To make it clear, I'm not the one - the bad guy - who deliberately disables
LLINT during testing, but jsc-stress-tests script - wrote by Filip Pizlo - 
which runs all tests in different configurations. It's same on all platforms.

As you can see in the description of the other bug, tests fail only in no-llint
configuration: https://bugs.webkit.org/show_bug.cgi?id=154822#c0 .
Not only on Linux, but your own Apple's ARM platform too. But you guys simply
skipped these tests previously. Just read this comment again if you haven't 
do it: https://bugs.webkit.org/show_bug.cgi?id=154822#c1

Back to the different allocators, we should make it clear, it wasn't my
decision which one we use. You at Apple implemented both of them, enabled 
the fixed allocator only on X86_64, IOS and ARM64, other platforms uses
the on demand allocator.

Platform.h
-----------
#if CPU(X86_64) || PLATFORM(IOS) || CPU(ARM64)
#define ENABLE_EXECUTABLE_ALLOCATOR_FIXED 1
#else
#define ENABLE_EXECUTABLE_ALLOCATOR_DEMAND 1
#endif

It means that the fixed allocator wasn't tested ever on other platforms.
In this case it isn't fair to switch to untested code path and remove
the current codepath without asking.

Now we know that the fixed allocator has at least three problems:
- The fixed sized 16Mb isn't enough to run all JSC stress tests.
  https://bugs.webkit.org/show_bug.cgi?id=154822
- Increasing the fixed size over 16Mb causes many crashes on the
  Apple owned Thumb2 ARMv7Assembler .
  https://bugs.webkit.org/show_bug.cgi?id=154822#c3
- There are crashed on Apple Mac x86 platforms too
  https://bugs.webkit.org/show_bug.cgi?id=154910
  https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/1607

I think it would be fair to fix the ARMv7Assembler issue and increase
the fixed size to 32 MB to make all stress tests pass _ before _ removing
the on demand allocator. But of course, it is just a suggestion. You can
do here whatever you want, because you are stronger.

[Csaba Osztrogonác]

*** This bug has been marked as a duplicate of bug 168754 ***