Bug 133136 – Latest emscripten life benchmark is 4x slower because the DFG doesn't realize that arithmetic on booleans is a thing

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alon Zakai]

The emscripten-asmjs benchmarks on arewefastyet.com have been updated (new builds with latest emscripten). JSC is faster on all of those but one (which is expected, as the new benchmarks used a newer LLVM, emscripten improvements, etc.), but one of them, the life benchmark, became 4x slower. Source of benchmark is at

https://github.com/haytjes/arewefastyet/blob/master/benchmarks/asmjs-ubench/life.js

This is a microbenchmark and perhaps not that interesting, and the new builds speed up JSC everywhere else, especially on the large benchmarks. Still, perhaps something is being hit here that might show up in other places.

Measurement was done by arewefastyet itself ( http://arewefastyet.com/#machine=12&view=breakdown&suite=asmjs-ubench ) and happens on both 64 and 32 bit OS X, which I think means with and without FTL.

[Filip Pizlo]

Oh, LMAO!

You're doing a clever thing where you add a boolean to a number, and we're being very dumb about it - we end up:

- Predicting that the result of boolean + number will be a string.  That's false.

- Deciding that the best way to execute boolean + number is with the deepest of addition slow paths.  That's dumb.

I'll fix it.

[Filip Pizlo]

Created attachment 231997 [details]
it begins

I'm going to make a comprehensive fix here.  In most places where we would speculate integer or number, we can accept booleans also.  We should of course be smart about it - like, if we have it on good authority that the input to a math operation is *only* integers then we shouldn't emit code to allow booleans.  I think that also we need to tweak the slow path counting in the baseline JIT - the baseline will think that booleans require taking slow path even though they basically don't.  It'll be interesting to figure out what to do about that.

[Filip Pizlo]

Created attachment 232027 [details]
moar!

Some key concepts:

- Except for the prediction propagation and type fixup phases, which are super early in the pipeline, nobody has to know about the fact that booleans may flow into numerical operations because there will just be a BooleanToNumber node that will take a value and, if that value is a boolean, will convert it to the equivalent numerical value.  It will have a BooleanUse mode where it will also speculate that the input is a boolean but it can also do UntypedUse in which case it will pass through any non-booleans.  This operation is very easy to model in all of the compiler tiers.

- No changes to the baseline JIT.  The Baseline JIT will still believe that boolean inputs require taking the slow path and it will still report that it took slow path for any such operations.  The DFG will now be smart enough to ignore baseline JIT slow path profiling on operations that were known to have had boolean inputs.  That's a little quirky, but it's probably easier than modifying the baseline JIT to track booleans correctly.

Most of this change is very mechanical and I'm not quite done yet.  Soon though!

[Filip Pizlo]

Created attachment 232042 [details]
almost done

It compiles.  Now I just need to get it to work.

[Filip Pizlo]

It works.  I will post a patch soon.  I think I need to think about some good tests.

[Filip Pizlo]

Created attachment 232049 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:83:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:83:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 77 files


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

[Filip Pizlo]

(In reply to comment #7)
> Attachment 232049 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:83:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]

Fixed.

> ERROR: Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:83:  Multi line control clauses should use braces.  [whitespace/braces] [4]

Nope.

> Total errors found: 2 in 77 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 232049 [details]
the patch

EFL EWS failure seems to be due to an additional warning that the compiler has:

JavaScriptCore/dfg/DFGFixupPhase.cpp:1588:15: error: variable 'currentValue' set but not used [-Werror=unused-but-set-variable]

I think it’s possible it detected a real problem; perhaps m_insertionSet.insertNode should be using currentValue instead of m_currentNode.

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 232049 [details])
> EFL EWS failure seems to be due to an additional warning that the compiler has:
> 
> JavaScriptCore/dfg/DFGFixupPhase.cpp:1588:15: error: variable 'currentValue' set but not used [-Werror=unused-but-set-variable]
> 
> I think it’s possible it detected a real problem; perhaps m_insertionSet.insertNode should be using currentValue instead of m_currentNode.

Ah!  It's actually the opposite - the variable shouldn't be there.  It's a vestige of an earlier version of the patch.  That branch in which it is assigned is also a vestige.

[Filip Pizlo]

Created attachment 232050 [details]
the patch

[Build Bot]

Comment on attachment 232050 [details]
the patch

Attachment 232050 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5333382259015680

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html

[Build Bot]

Created attachment 232056 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Filip Pizlo]

(In reply to comment #13)
> Created an attachment (id=232056) [details]
> Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

This failure doesn't look related to anything I did.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/169354

[Csaba Osztrogonác]

(In reply to comment #15)
> Landed in http://trac.webkit.org/changeset/169354

It broke 10 JSC stress tests on the Apple 32 bit buildbot:

** The following JSC stress test failures have been introduced:
	stress/tricky-array-bounds-checks.js.always-trigger-copy-phase
	stress/tricky-array-bounds-checks.js.default
	stress/tricky-array-bounds-checks.js.default-ftl
	stress/tricky-array-bounds-checks.js.dfg-eager
	stress/tricky-array-bounds-checks.js.dfg-eager-no-cjit-validate
	stress/tricky-array-bounds-checks.js.ftl-eager
	stress/tricky-array-bounds-checks.js.ftl-eager-no-cjit
	stress/tricky-array-bounds-checks.js.ftl-no-cjit-no-inline-validate
	stress/tricky-array-bounds-checks.js.ftl-no-cjit-validate
	stress/tricky-array-bounds-checks.js.no-cjit-validate-phases

Results for JSC stress tests:
    10 failures found.

[Geoffrey Garen]

<http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/1678>

[Geoffrey Garen]

<rdar://problem/17040598>

[Alon Zakai]

Great stuff! Massive speedup on arewefastyet due to this.

Just fyi, v8 has a similar issue, and has apparently decided not to optimize it (the results of which can also be seen on arewefastyet). We are therefore looking into workarounds in emscripten for v8, which will probably involve emitting |0 or such on boolean-generating expressions in the contexts where v8 requires it (which will increase code size, but hopefully not too badly). Please let me know if there are any concerns on your side regarding that, but I assume especially since the optimizations here, that |0 etc. on booleans would not be a problem for you?

[Filip Pizlo]

(In reply to comment #19)
> Great stuff! Massive speedup on arewefastyet due to this.
> 
> Just fyi, v8 has a similar issue, and has apparently decided not to optimize it (the results of which can also be seen on arewefastyet). We are therefore looking into workarounds in emscripten for v8, which will probably involve emitting |0 or such on boolean-generating expressions in the contexts where v8 requires it (which will increase code size, but hopefully not too badly). Please let me know if there are any concerns on your side regarding that, but I assume especially since the optimizations here, that |0 etc. on booleans would not be a problem for you?

Nope, it shouldn't be a problem.

The downside is that you're emitting more code, which is unfortunate.  It seems silly that V8 isn't optimizing a fairly sensible feature of JavaScript.  After all, in C++ I can do:

bool x = ...
int y = x + 1;

And it works just like in JS. ;-)