143376 2015-04-03 08:58:21 -0700 FTL JIT tests should fail if JSC is built without FTL JIT support 2016-01-07 07:36:28 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 143374 1 ossy ossy fpizlo ggaren mark.lam mcatanzaro msaboff oliver ossy oldest_to_newest 1082576 0 ossy 2015-04-03 08:58:21 -0700 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 1082577 1 250077 ossy 2015-04-03 08:59:59 -0700 Created attachment 250077 Patch 1084692 2 ossy 2015-04-12 14:05:59 -0700 ping? 1084701 3 fpizlo 2015-04-12 15:58:41 -0700 (In reply to comment #2) > ping? Why is this desirable behavior? We don't do this for any of the other JIT tiers. 1084739 4 ossy 2015-04-12 23:30:32 -0700 (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. 1084833 5 fpizlo 2015-04-13 09:47:16 -0700 (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? 1084834 6 fpizlo 2015-04-13 09:47:17 -0700 (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? 1084853 7 ossy 2015-04-13 10:19:12 -0700 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. 1084863 8 fpizlo 2015-04-13 10:30:48 -0700 (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. 1087795 9 ossy 2015-04-23 05:12:54 -0700 (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. 1153350 10 mcatanzaro 2016-01-06 20:36:04 -0800 (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? 1153410 11 ossy 2016-01-07 07:35:51 -0800 (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. 250077 2015-04-03 08:59:59 -0700 2016-01-07 07:36:28 -0800 Patch bug-143376-20150403085912.patch text/plain 1370 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTgyMzE3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAy MGJmODJhODQ0ZGFhZjNlNTc4MGYxYWUwOGRjMmJlNTdlNGM3ZTA3Li5mYmIwM2FhZmVlOTZiMGJh MDMzMjNkZjI0M2U3ZDU4YTgxMzlmODNmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxNS0wNC0wMyAgQ3NhYmEgT3N6dHJvZ29uw6FjICA8b3NzeUB3ZWJraXQu b3JnPgorCisgICAgICAgIEZUTCBKSVQgdGVzdHMgc2hvdWxkIGZhaWwgaWYgSlNDIGlzIGJ1aWx0 IHdpdGhvdXQgRlRMIEpJVCBzdXBwb3J0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNDMzNzYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIGpzYy5jcHA6CisgICAgICAgIChqc2NtYWluKToKKwogMjAxNS0w NC0wMyAgWmFuIERvYmVyc2VrICA8emRvYmVyc2VrQGlnYWxpYS5jb20+CiAKICAgICAgICAgRml4 IHRoZSBFRkwgYW5kIEdUSyBidWlsZCBhZnRlciByMTgyMjQzCmRpZmYgLS1naXQgYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvanNjLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qc2MuY3BwCmlu ZGV4IDY5MDVhOGI1MmUxMzBlMTJlNzNlZjY5NmNiMDg4NGFiNzJiZjBhN2QuLmU0OGFjNTUyY2Rk NGRmOWRkNjQxODAwMDBlYTlhM2VmMDZhNmIyOWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9qc2MuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qc2MuY3BwCkBAIC0x NDcyLDYgKzE0NzIsMTQgQEAgaW50IGpzY21haW4oaW50IGFyZ2MsIGNoYXIqKiBhcmd2KQogICAg IC8vIE5vdGUgdGhhdCB0aGUgb3B0aW9ucyBwYXJzaW5nIGNhbiBhZmZlY3QgVk0gY3JlYXRpb24s IGFuZCB0aHVzCiAgICAgLy8gY29tZXMgZmlyc3QuCiAgICAgQ29tbWFuZExpbmUgb3B0aW9ucyhh cmdjLCBhcmd2KTsKKworI2lmICFFTkFCTEUoRlRMX0pJVCkKKyAgICBpZiAoT3B0aW9uczo6ZnRs Q3Jhc2hlc0lmQ2FudEluaXRpYWxpemVMTFZNKCkpIHsKKyAgICAgICAgZGF0YUxvZygiQ2FuJ3Qg ZW5hYmxlIEZUTCBKSVQsIGJlY2F1c2UgSlNDIGlzIGJ1aWx0IHdpdGhvdXQgRlRMIEpJVCBzdXBw b3J0LlxuIik7CisgICAgICAgIENSQVNIKCk7CisgICAgfQorI2VuZGlmCisKICAgICBWTSogdm0g PSBWTTo6Y3JlYXRlKExhcmdlSGVhcCkubGVha1JlZigpOwogICAgIGludCByZXN1bHQ7CiAgICAg ewo=