Bug 154749

Created attachment 272366 [details] Patch

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.

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.

Created attachment 272380 [details] Patch

Comment on attachment 272380 [details] Patch r=me

Comment on attachment 272380 [details] Patch Clearing flags on attachment: 272380 Committed r197226: <http://trac.webkit.org/changeset/197226>

All reviewed patches have been landed. Closing bug.

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

(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

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

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

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

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

Re-opened since this is blocked by bug 154910

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

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

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

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

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

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

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