Bug 143376 – FTL JIT tests should fail if JSC is built without FTL JIT support

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Csaba Osztrogonác]

Now we can build JSC without FTL JIT and can execute run-javascriptcore-tests with --ftl-jit 
accidentally and all tests pass. The problem is that useFTLJIT options is silently set to false:
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/Options.cpp#L233

[Csaba Osztrogonác]

Created attachment 250077 [details]
Patch

[Csaba Osztrogonác]

ping?

[Filip Pizlo]

(In reply to comment #2)
> ping?

Why is this desirable behavior?

We don't do this for any of the other JIT tiers.

[Csaba Osztrogonác]

(In reply to comment #3)
> (In reply to comment #2)
> > ping?
> 
> Why is this desirable behavior?
> 
> We don't do this for any of the other JIT tiers.

Because FTL JIT is the only one tier which is disabled by default on Linux
platforms and the only one tier which has to be enabled with explicit command
line option of build-jsc / build-webkit / run-javascriptcore-tests.

It's so easy to miss --ftl-jit accidentally during development. I'd like
to add this extra check to avoid getting false positive FTL JIT test passes
when FTL JIT isn't built at all. ftlCrashesIfCantInitializeLLVM option
is true only if you call JSC from run-javascriptcore-tests and FTL JIT
build is disabled only on non Apple Mac platforms, so this check wouldn't
affect Apple Mac port at all.

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > ping?
> > 
> > Why is this desirable behavior?
> > 
> > We don't do this for any of the other JIT tiers.
> 
> Because FTL JIT is the only one tier which is disabled by default on Linux
> platforms and the only one tier which has to be enabled with explicit command
> line option of build-jsc / build-webkit / run-javascriptcore-tests.
> 
> It's so easy to miss --ftl-jit accidentally during development. I'd like
> to add this extra check to avoid getting false positive FTL JIT test passes
> when FTL JIT isn't built at all. ftlCrashesIfCantInitializeLLVM option
> is true only if you call JSC from run-javascriptcore-tests and FTL JIT
> build is disabled only on non Apple Mac platforms, so this check wouldn't
> affect Apple Mac port at all.

Wouldn't it be cleaner and simpler to just remove the --ftl-jit option?

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > ping?
> > 
> > Why is this desirable behavior?
> > 
> > We don't do this for any of the other JIT tiers.
> 
> Because FTL JIT is the only one tier which is disabled by default on Linux
> platforms and the only one tier which has to be enabled with explicit command
> line option of build-jsc / build-webkit / run-javascriptcore-tests.
> 
> It's so easy to miss --ftl-jit accidentally during development. I'd like
> to add this extra check to avoid getting false positive FTL JIT test passes
> when FTL JIT isn't built at all. ftlCrashesIfCantInitializeLLVM option
> is true only if you call JSC from run-javascriptcore-tests and FTL JIT
> build is disabled only on non Apple Mac platforms, so this check wouldn't
> affect Apple Mac port at all.

Wouldn't it be cleaner and simpler to just remove the --ftl-jit option?

[Csaba Osztrogonác]

Removing the --ftl-jit option of what? build-jsc and build-webkit has
this option to enable building FTL JIT if it isn't built by default.
run-javascriptcore-tests has this option to enable FTL JIT tests too,
because run-jsc-stress-tests doesn't know how did you build JSC.

Removing --useFTLJIT command line argument of JSC wouldn't work,
because run-jsc-stress-tests still doesn't how did you build JSC
and it passes --useFTLJIT=false as base options and passes
--useFTLJIT=true as FTL options.

[Filip Pizlo]

(In reply to comment #7)
> Removing the --ftl-jit option of what? build-jsc and build-webkit has
> this option to enable building FTL JIT if it isn't built by default.
> run-javascriptcore-tests has this option to enable FTL JIT tests too,
> because run-jsc-stress-tests doesn't know how did you build JSC.

Remove all of them.  On x86_64 Linux, and on all 64-bit Darwins, we just build it.

I get what scenario you're worried about - you want to test FTL, but you didn't build it, and so you get surprised.  We found that we didn't need it.

We had an option in the tests that was exactly like what you proposed - but it caused strange issues.  For example, someone would intentionally tweak something in a way that disabled FTL even though --ftl-jit was provided, and they'd get test failures and spend some time scratching their heads.  Eventually, we found that forcing test failures when the FTL JIT was disabled wasn't worth it once we just had FTL enabled across the board.  When we do want to validate that our performance features are enabled, we just rely on performance tests.  The difference in execution time of things like Octane is pretty obvious.  But, we haven't had this issue in a long time.

I suspect that the Linux ports are no different.  Once it's enabled, then that's that.  We don't have to worry about accidentally disabling it.

Is there some scenario that you hit often that involves the FTL JIT accidentally being disabled, and the only way to validate this is to force test failures?

> 
> Removing --useFTLJIT command line argument of JSC wouldn't work,
> because run-jsc-stress-tests still doesn't how did you build JSC
> and it passes --useFTLJIT=false as base options and passes
> --useFTLJIT=true as FTL options.

I don't want to remove that option.  I'm only talking about the --ftl-jit options to the build scripts.

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > Removing the --ftl-jit option of what? build-jsc and build-webkit has
> > this option to enable building FTL JIT if it isn't built by default.
> > run-javascriptcore-tests has this option to enable FTL JIT tests too,
> > because run-jsc-stress-tests doesn't know how did you build JSC.
> 
> Remove all of them.  On x86_64 Linux, and on all 64-bit Darwins, we just
> build it.

To make it clear FTL JIT is _disabled_ by default 
on all Linux platforms because of many reasons:
- The necessary pathces for X86_64 can be found only in LLVM 3.6 which
isn't shipped yet by Ubuntu. Ubuntu 15.04 will ship LLVM 3.6 , the planned
release date is today, but isn't released yet. (Of course it can be
workaroundable with jhbuild - we do it now)
- There are many crashes with FTL JIT on Linux (X86_64 and AArch64 too):
bug143087 and bug143089
- performance regressions on SunSpider and V8: bug143822

I don't think if anybody wants to enable a feature which 
slows down the runtime and makes JSC crash sometimes. These 
bugs should be fixed before enabling FTL JIT by default.
 
> I get what scenario you're worried about - you want to test FTL, but you
> didn't build it, and so you get surprised.  We found that we didn't need it.
> 
> We had an option in the tests that was exactly like what you proposed - but
> it caused strange issues.  For example, someone would intentionally tweak
> something in a way that disabled FTL even though --ftl-jit was provided, and
> they'd get test failures and spend some time scratching their heads. 

I don't understand this problem, the proposed change would provide simple
and clear error message if you build JSC with disabled FTL JIT, but try
to run tests with run-javascriptcore-tests with --ftl-jit (which passes
--useFTLJIT=true and --ftlCrashesIfCantInitializeLLVM=true)

There is _no_ reason to run tests twice, first time with useFTLJIT=false
and then with useFTLJIT=true, because useFTLJIT will be set to false
if FTL JIT was disabled in build time. It is wasting of time, nothing else. For example the Apple Windows bots ran tests twice due to this bug
until I disabled it. (https://trac.webkit.org/changeset/179190)

The root of the problem is that run-jsc-stress-tests doesn't 
know if JSC was built with enabled or disabled FTL JIT.

> Eventually, we found that forcing test failures when the FTL JIT was
> disabled wasn't worth it once we just had FTL enabled across the board. 
> When we do want to validate that our performance features are enabled, we
> just rely on performance tests.  The difference in execution time of things
> like Octane is pretty obvious.  But, we haven't had this issue in a long
> time.
> 
> I suspect that the Linux ports are no different.  Once it's enabled, then
> that's that.  We don't have to worry about accidentally disabling it.

But the problem is that FTL JIT isn't enabled by default,
we have to enable explicitly now. I don't think if it will
be disabled accidentally by a change ever.
 
> Is there some scenario that you hit often that involves the FTL JIT
> accidentally being disabled, and the only way to validate this is to force
> test failures?

Not so often, but more then 1-2 times I missed to pass --ftl-jit to
build-jsc and noticed this user error hours later when stress/performance
tests finished. With this patch I could have caught it at the beginning.
 
> > Removing --useFTLJIT command line argument of JSC wouldn't work,
> > because run-jsc-stress-tests still doesn't how did you build JSC
> > and it passes --useFTLJIT=false as base options and passes
> > --useFTLJIT=true as FTL options.
> 
> I don't want to remove that option.  I'm only talking about the --ftl-jit
> options to the build scripts.

I have strong objection against removing --ftl-jit option
until it is enabled by default on X86_64 and AArch64 Linux.
We should be able to enable FTL JIT with command line options,
not only with modifying the build system.

[Michael Catanzaro]

(In reply to comment #9)
> I have strong objection against removing --ftl-jit option
> until it is enabled by default on X86_64 and AArch64 Linux.
> We should be able to enable FTL JIT with command line options,
> not only with modifying the build system.

Hi from 2016. Do we still want this patch?

[Csaba Osztrogonác]

(In reply to comment #10)
> Hi from 2016. Do we still want this patch?

Filip doesn't want this change and it isn't so important for me.