Bug 158892 – Switching on symbols should be fast

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 281595 [details]
something like this

[Filip Pizlo]

Created attachment 281620 [details]
maybe the patch

This does the trick.

What's sad about this is that this optimization really belongs in lower tiers, but currently it's not feasible to do it in DFG IR.  You really need B3 IR to do it.  But without the optimization, the code runs so incredibly slow that it takes a long time to get to FTL.

It's starting to become quite clear that we need tier-up checks in straight-line code.

[Filip Pizlo]

Created attachment 281622 [details]
better patch

[Filip Pizlo]

Looks like this has a bug:

stress/float64-array-nan-inlined.js.default: test_script_12303: line 2: 55586 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true float64-array-nan-inlined.js )
stress/float64-array-nan-inlined.js.default: ERROR: Unexpected exit code: 139

Maybe it's latent so I'm still testing without the patch.

[Filip Pizlo]

(In reply to comment #4)
> Looks like this has a bug:
> 
> stress/float64-array-nan-inlined.js.default: test_script_12303: line 2:
> 55586 Segmentation fault: 11  ( "$@"
> ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false
> --useFunctionDotArguments\=true float64-array-nan-inlined.js )
> stress/float64-array-nan-inlined.js.default: ERROR: Unexpected exit code: 139
> 
> Maybe it's latent so I'm still testing without the patch.

Wait, it has to be latent!  It's a non-FTL test and this patch only changes FTL.

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > Looks like this has a bug:
> > 
> > stress/float64-array-nan-inlined.js.default: test_script_12303: line 2:
> > 55586 Segmentation fault: 11  ( "$@"
> > ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false
> > --useFunctionDotArguments\=true float64-array-nan-inlined.js )
> > stress/float64-array-nan-inlined.js.default: ERROR: Unexpected exit code: 139
> > 
> > Maybe it's latent so I'm still testing without the patch.
> 
> Wait, it has to be latent!  It's a non-FTL test and this patch only changes
> FTL.

Yeah, it was an unrelated bug, and I fixed it.

[Filip Pizlo]

Created attachment 282086 [details]
awesome patch

[Filip Pizlo]

Created attachment 282087 [details]
patch

[Filip Pizlo]

Created attachment 283979 [details]
the patch

[Filip Pizlo]

Created attachment 283980 [details]
the patch

[Filip Pizlo]

Comment on attachment 283980 [details]
the patch

Oh man, this doesn't quite work!  Fixing...

[Filip Pizlo]

Created attachment 283981 [details]
the patch

[Saam Barati]

Comment on attachment 283981 [details]
the patch

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

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:439
> +            // put them. We're not actually smart enough things to move things around at random.

"smart enough things" => "smart enough"

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:466
> +            if (m_blockInsertionSet.execute() | m_changedCFG) {

Should be "||"

[Filip Pizlo]

(In reply to comment #13)
> Comment on attachment 283981 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283981&action=review
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:439
> > +            // put them. We're not actually smart enough things to move things around at random.
> 
> "smart enough things" => "smart enough"

Fixed.

> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:466
> > +            if (m_blockInsertionSet.execute() | m_changedCFG) {
> 
> Should be "||"

That was sort of deliberate.  I wanted to emphasize that every subexpression should execute for side-effects.  I changed it to:

m_changedCFG |= m_blockInsertionSet.execute();
if (m_changedCFG) {

[Filip Pizlo]

Created attachment 284037 [details]
the patch

[Filip Pizlo]

Created attachment 284059 [details]
performance

[Keith Miller]

Comment on attachment 284037 [details]
the patch

r=me. Out of curiosity does this optimization kick in if the user doesn't write a switch explicitly? If so, maybe it would be worth adding a test. For example:

if (a === sym1)
    ...
if (a === sym2)
    ...
...

[Filip Pizlo]

(In reply to comment #17)
> Comment on attachment 284037 [details]
> the patch
> 
> r=me. Out of curiosity does this optimization kick in if the user doesn't
> write a switch explicitly? If so, maybe it would be worth adding a test. For
> example:
> 
> if (a === sym1)
>     ...
> if (a === sym2)
>     ...
> ...

It does kick in for this!  Good idea, I will add a test for it.

[Filip Pizlo]

(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 284037 [details]
> > the patch
> > 
> > r=me. Out of curiosity does this optimization kick in if the user doesn't
> > write a switch explicitly? If so, maybe it would be worth adding a test. For
> > example:
> > 
> > if (a === sym1)
> >     ...
> > if (a === sym2)
> >     ...
> > ...
> 
> It does kick in for this!  Good idea, I will add a test for it.

Yeah, I wrote a test like this - basically a scripted rewrite of bigswitch-indirect-symbol to use if - and it gets a speed-up from this patch.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/203491