Bug 153478 – [GTK][EFL] Switch FTL to B3

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Mac already did that in r195562. At the moment B3 doesn't even build, but since we have enabled FTL with LLVM for the first this cycle, we should probably witch to B3 before the next stable release, or disable FTL for now again to avoid introducing a dependency that we will remove soon.

[Csaba Osztrogonác]

It should build fine, see bug153456 for details.

But unfortunately there are zillion crashes, see bug153422 for details.
Before this serious regression there were only 6 crashes. (bug152248)

[Csaba Osztrogonác]

Now there is only one JSC test failure with B3 based FTL JIT: bug153644
Once this bug is fixed, we can enable it by default and start removing
LLVM dependency.

[Csaba Osztrogonác]

Created attachment 270193 [details]
Patch

[Michael Catanzaro]

Since LLVM will now only be used by the iOS, I think we should get rid of FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files as well.

[Michael Catanzaro]

Certainly we need the build to not fail when LLVM is not installed.

[Michael Catanzaro]

(In reply to comment #4)
> Since LLVM will now only be used by the iOS, I think we should get rid of
> FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files
> as well.

I didn't realize that EFL supported FTL on AArch64. I don't know why EFL supports this but not GTK.

Anyway, I guess we need to leave the LLVM CMake goo around for that case, unless you want to drop support for FTL or AArch64, but we should stop using it at least when it's not needed.

[Michael Catanzaro]

*** Bug 153720 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Created attachment 270333 [details]
Patch

[Michael Catanzaro]

This patch seems to work in both Epiphany and MiniBrowser, but I couldn't immediately figure out how to get JSC tests working, so....

[Michael Catanzaro]

Created attachment 270334 [details]
Patch

[Michael Catanzaro]

Created attachment 270337 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #4)
> > Since LLVM will now only be used by the iOS, I think we should get rid of
> > FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files
> > as well.
> 
> I didn't realize that EFL supported FTL on AArch64. I don't know why EFL
> supports this but not GTK.

IIRC, LLVM needed some patches for AArch64 that were not in LLVM 3.7. EFL enabled it adding those patches in jhbuild. But that doesn't work for us in production, so we were waiting for those patches to land in LLVM and require the new version for AArch64.

> Anyway, I guess we need to leave the LLVM CMake goo around for that case,
> unless you want to drop support for FTL or AArch64, but we should stop using
> it at least when it's not needed.

I think it's indeed simpler to just disable FTL on AArch64 for now, but maybe EFL guys have a good reason to keep using LLVM.

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Since LLVM will now only be used by the iOS, I think we should get rid of
> > > FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files
> > > as well.
> > 
> > I didn't realize that EFL supported FTL on AArch64. I don't know why EFL
> > supports this but not GTK.
> 
> IIRC, LLVM needed some patches for AArch64 that were not in LLVM 3.7. EFL
> enabled it adding those patches in jhbuild. But that doesn't work for us in
> production, so we were waiting for those patches to land in LLVM and require
> the new version for AArch64.

FTL JIT worked more or less with the patched LLVM 3.5 on AArch64,
but it wasn't enabled by default ever, because there were a few
JSC stress test crashes. Bumping to LLVM 3.6 wasn't possible,
because LLVM stucked in an infinite loop during compiling.
(didn't investigate what was the problem)

First and last, it isn't showstopper to remove the LLVM dependency
and enable B3-FTL by default on X86_64 Linux platforms. And we can
enable it on AArch64 once it is ready.

[Csaba Osztrogonác]

Comment on attachment 270337 [details]
Patch

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

In general it looks good to me with some changes.

> Source/JavaScriptCore/CMakeLists.txt:-1074
> -        llvm/InitializeLLVM.cpp
> -        llvm/InitializeLLVMLinux.cpp
> -        llvm/InitializeLLVMPOSIX.cpp

I think we will need these files for LLVMDisassembler, but I can fix it later.

> Source/JavaScriptCore/ftl/FTLState.cpp:52
> +#if !FTL_USES_B3
>      , context(llvm->ContextCreate())
> +#endif

In this case context member would be uninitialized and unused.
Let's add guard for it in the header file too.

> Source/WTF/wtf/Platform.h:794
>  

We can remove these lines from Platform.h:

#if PLATFORM(GTK) && HAVE(LLVM) && ENABLE(JIT) && !defined(ENABLE_FTL_JIT) && CPU(X86_64)
#define ENABLE_FTL_JIT 1
#endif

> Source/cmake/FindLLVM.cmake:-1
> -#

Please don't remove Source/cmake/FindLLVM.cmake, because LLVMDisassembler still needs LLVM.

> Source/cmake/OptionsEfl.cmake:280
> +if (ENABLE_FTL_JIT AND NOT WTF_CPU_X86_64)
> +    message(FATAL_ERROR "FTL JIT is only available on X86_64 architecture.")

It would be great if we can enable FTL JIT explicitly with command line option on AArch64 too for testing purposes.

[Michael Catanzaro]

(In reply to comment #14)
> I think we will need these files for LLVMDisassembler, but I can fix it
> later.

I didn't know about LLVMDisassembler. OK.

> In this case context member would be uninitialized and unused.
> Let's add guard for it in the header file too.

OK.

> We can remove these lines from Platform.h:
> 
> #if PLATFORM(GTK) && HAVE(LLVM) && ENABLE(JIT) && !defined(ENABLE_FTL_JIT)
> && CPU(X86_64)
> #define ENABLE_FTL_JIT 1
> #endif

Yeah, ENABLE_FTL_JIT is guaranteed to be defined for a long time now, that so condition will never be hit.

> > Source/cmake/FindLLVM.cmake:-1
> > -#
> 
> Please don't remove Source/cmake/FindLLVM.cmake, because LLVMDisassembler
> still needs LLVM.

OK.

> It would be great if we can enable FTL JIT explicitly with command line
> option on AArch64 too for testing purposes.

OK, let's allow it when DEVELOPER_MODE is on.

[Csaba Osztrogonác]

(In reply to comment #15)
> > It would be great if we can enable FTL JIT explicitly with command line
> > option on AArch64 too for testing purposes.
> 
> OK, let's allow it when DEVELOPER_MODE is on.

I didn't mean enabling by default, because it isn't stable at all.
Without FTL JIT we have 20-40 crashes long time ago on AArch64: bug151486,
and ~120 crashes with enabled B3 FTL JIT.

94    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${WTF_CPU_X86_64})
This line enables FTL JIT on X86_64 by default, it's correct. But we didn't
want to enable it on AArch64 yet, because JSC in itself it is unstable
on AArch64 Linux a half year ago. We don't want to have more problem
until somebody fixes bug151486.

279    if (ENABLE_FTL_JIT AND NOT WTF_CPU_X86_64)
280        message(FATAL_ERROR "FTL JIT is only available on X86_64 architecture.")

I simply meant that we shouldn't trigger fatal error if a developer would 
like to enable FTL JIT on AArch64 with --ftl-jit command line option.

[Michael Catanzaro]

Right, I understood. I am planning to do this:

if (ENABLE_FTL_JIT AND NOT WTF_CPU_X86_64 AND NOT DEVELOPER_MODE)
    message(FATAL_ERROR "FTL JIT is only available on X86_64 architecture.")
endif ()

So if you pass -DENABLE_FTL_JIT and -DDEVELOPER_MODE (which should be passed by build-webkit) you can get FTL on AArch64, but otherwise you'll get stopped.

[Michael Catanzaro]

(In reply to comment #14) 
> In this case context member would be uninitialized and unused.
> Let's add guard for it in the header file too.

It gets used from various places... let's initialize it to nullptr and see if that works....

[Michael Catanzaro]

(In reply to comment #9)
> This patch seems to work in both Epiphany and MiniBrowser, but I couldn't
> immediately figure out how to get JSC tests working, so....

I ran 'run-javascriptcore-tests --debug --ftl' and got 30787/30787 passes. Some tests were disabled due to "missing Ruby features" but seems good enough to me.

[Michael Catanzaro]

Created attachment 270550 [details]
Patch

[Carlos Garcia Campos]

Created attachment 270560 [details]
Rebased patch

Just made it apply on current trunk

[Carlos Garcia Campos]

Created attachment 270562 [details]
Try again

Same patch again now that build fix landed in r196057

[Carlos Garcia Campos]

I've just tried the patch but got a couple of failures:

modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules: Segmentation fault
modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules: ERROR: Unexpected exit code: 139
modules.yaml/modules/namespace.js.ftl-eager-modules: Segmentation fault
modules.yaml/modules/namespace.js.ftl-eager-modules: ERROR: Unexpected exit code: 139
30901/30901 (failed 2)         

** The following JSC stress test failures have been introduced:
	modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules
	modules.yaml/modules/namespace.js.ftl-eager-modules

Results for JSC stress tests:
    2 failures found.

[Carlos Garcia Campos]

I've run Kraken, Octane, JetStream and Speedometer without problems, though, and getting significant better results with B3 in all cases expect Speedometer that I got the very same score with LLVM. I'm currently running my browser with B3 too, without problems so far.

[Csaba Osztrogonác]

Results on EFL port:

** The following JSC stress test failures have been introduced:
	modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules

Results for JSC stress tests:
    1 failure found.


Maybe it is a flakiness or a new regression.

[Csaba Osztrogonác]

We will need to add the following lines to Options<Efl|Gtk>.cmake :

+if (USE_LLVM_DISASSEMBLER)
+    find_package(LLVM REQUIRED)
+    SET_AND_EXPOSE_TO_BUILD(HAVE_LLVM TRUE)
+endif()

[Csaba Osztrogonác]

Comment on attachment 270562 [details]
Try again

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

> Source/JavaScriptCore/CMakeLists.txt:1076
> +    list(APPEND JavaScriptCore_SOURCES
> +        disassembler/LLVMDisassembler.cpp
> +        disassembler/X86Disassembler.cpp

We can remove these lines, these sources are added to JavaScriptCore_SOURCES unconditionally.

> Source/JavaScriptCore/dfg/DFGPlan.cpp:458
> +#if !FTL_USES_B3 || USE(LLVM_DISASSEMBLER)

How is this file related to LLVM_DISASSEMBLER?

> Source/JavaScriptCore/dfg/DFGPlan.cpp:464
> +#if !FTL_USES_B3 || USE(LLVM_DISASSEMBLER)

ditto

> Source/JavaScriptCore/dfg/DFGPlan.cpp:471
> +#if !FTL_USES_B3 || USE(LLVM_DISASSEMBLER)

ditto

> Source/JavaScriptCore/ftl/FTLState.cpp:41
> +#if !FTL_USES_B3
> +#include <llvm/InitializeLLVM.h>
> +#endif

llvm/InitializeLLVM.h is guarded properly, we don't need conditional include.

[Carlos Garcia Campos]

We also need to change if (ENABLE_FTL_JIT) with if (USE_LLVM_DISASSEMBLER) in PlatformGTK.cmake

[Carlos Garcia Campos]

(In reply to comment #25)
> Results on EFL port:
> 
> ** The following JSC stress test failures have been introduced:
> 	modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules
> 
> Results for JSC stress tests:
>     1 failure found.
> 
> 
> Maybe it is a flakiness or a new regression.

Yes, maybe, I've just run it again and I got this one only as well. I'm using it in my browser without problems, so I think we can land the patch and fix tests failing afterwards.

[Yusuke Suzuki]

(In reply to comment #25)
> Results on EFL port:
> 
> ** The following JSC stress test failures have been introduced:
> 	modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules
> 
> Results for JSC stress tests:
>     1 failure found.
> 
> 
> Maybe it is a flakiness or a new regression.

Is it caused before enabling B3?

[Csaba Osztrogonác]

(In reply to comment #30)
> (In reply to comment #25)
> > Results on EFL port:
> > 
> > ** The following JSC stress test failures have been introduced:
> > 	modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules
> > 
> > Results for JSC stress tests:
> >     1 failure found.
> > 
> > 
> > Maybe it is a flakiness or a new regression.
> 
> Is it caused before enabling B3?

Linux platforms still use LLVM-FTL without any failures, but 
we got this crash with the proposed patch which tries to enable B3.

But previously (near a week before) I haven't seen any failures
with enabled B3, that's why I think it can be a regression.

[Carlos Garcia Campos]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #25)
> > > Results on EFL port:
> > > 
> > > ** The following JSC stress test failures have been introduced:
> > > 	modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules
> > > 
> > > Results for JSC stress tests:
> > >     1 failure found.
> > > 
> > > 
> > > Maybe it is a flakiness or a new regression.
> > 
> > Is it caused before enabling B3?
> 
> Linux platforms still use LLVM-FTL without any failures, but 
> we got this crash with the proposed patch which tries to enable B3.
> 
> But previously (near a week before) I haven't seen any failures
> with enabled B3, that's why I think it can be a regression.

And that's why I think we should land this in trunk asap, while the patch is hanging here new regressions in linux might be introduced without nobody noticing it :-)

[Yusuke Suzuki]

(In reply to comment #32)
> And that's why I think we should land this in trunk asap, while the patch is
> hanging here new regressions in linux might be introduced without nobody
> noticing it :-)

Yeah, landing this patch is very nice to me too ;)
I've just applied this patch, and built JSC and execute it infinitely by "test.rb"
loop {
    system "WebKitBuild/Release/bin/jsc -m Source/JavaScriptCore/tests/modules/execution-order-self.js"
}

But I didn't see any segv yet. Once it is caused, is it possible to paste its stack trace? (A separated issue is nice).

[Carlos Garcia Campos]

Created attachment 270579 [details]
Addressed review comments

[WebKit Commit Bot]

Attachment 270579 [details] did not pass style-queue:


ERROR: Source/cmake/OptionsEfl.cmake:282:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 270580 [details]
Fix "coding" style in EFL cmake file

[Csaba Osztrogonác]

Comment on attachment 270580 [details]
Fix "coding" style in EFL cmake file

LGTM, let's do the B3!

[WebKit Commit Bot]

Comment on attachment 270580 [details]
Fix "coding" style in EFL cmake file

Clearing flags on attachment: 270580

Committed r196077: <http://trac.webkit.org/changeset/196077>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 270580 [details]
Fix "coding" style in EFL cmake file

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

> Source/cmake/OptionsEfl.cmake:94
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${ENABLE_FTL_DEFAULT})
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${WTF_CPU_X86_64})

It didn't work on non X86_64, because WTF_CPU_X86_64 is undefined, not OFF.
Reverted this part of the patch in https://trac.webkit.org/changeset/196113