Bug 212511 – br_table is slow or it doesn't scale

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

[Vlad Brezae]

Consider this C file https://gist.github.com/BrzVlad/1b5a6bdb20205db1c970edae709d337e. Compile it to wasm using an emsdk (emsdk/upstream/emscripten/emcc -O2 huge-interp.c). Run it once as is and once with SWITCH_CASE defined to nothing. While on v8 you can see that the performance is more or less the same, on jsc the test case becomes 10x slower, which shouldn't happen. Looking into the generated native code, it shows that br_table is implemented by doing a lot of sequential comparisons rather than dispatching through a table.

This is significantly impacting the performance of the mono runtime (when it is running in interpreter mode)

[Radar WebKit Bug Importer]

<rdar://problem/63813245>

[Tadeu Zagallo]

Created attachment 400861 [details]
Patch

[Mark Lam]

Comment on attachment 400861 [details]
Patch

r=me if you've verified that JetStream2 and Speedometer2 are not impacted.  Can you confirm that perf is not impacted?

[Saam Barati]

Comment on attachment 400861 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        already have LICM in DFG, it should be ok to disable it for now.

but B3's LICM will probably hoist things DFG LICM can't.

Why not just disable it for now in Wasm code?

[Tadeu Zagallo]

Comment on attachment 400861 [details]
Patch

Ran perf tests and the results were neutral on JS2 and Speedometer2. Talked with Saam offline and he agreed to landing this patch and working on improving the phase in the future.

[EWS]

Committed r262523: <https://trac.webkit.org/changeset/262523>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400861 [details].

[Yusuke Suzuki]

testb3 is failing after this change, can you take a look?

[Filip Pizlo]

(In reply to Saam Barati from comment #4)
> Comment on attachment 400861 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400861&action=review
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        already have LICM in DFG, it should be ok to disable it for now.
> 
> but B3's LICM will probably hoist things DFG LICM can't.
> 
> Why not just disable it for now in Wasm code?

I added B3’s LICM for fun and completeness. Not sad to see it disabled if it causes any issues at all whatsoever since its upside is believed to be something like zero. LICM is a cheesy optimization anyway (huge win for some benchmarks but zero effect on the overwhelming majority of loops) and the DFG’s is really good. So you ha e very few LICM opportunities and DFG finds almost all of them. The ones left to B3 are not that interesting. 

And in wasm I would assume that licm rarely finds something that is both profitable and legal to hoist. B3’s LICM can’t hoist checking opcodes so that immediately means no hoisting of memory accesses. Memory access hoisting is the most profitable, so that means it’s just not going to be a big perf winner.