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
Created attachment 250077 [details] Patch
ping?
(In reply to comment #2) > ping? Why is this desirable behavior? We don't do this for any of the other JIT tiers.
(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.
(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?
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.
(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.
(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.
(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?
(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.