wasm2c: Optimize call indirect code footprint for better performance

[shravanrn]

Currently call indirects in Wasm2c add a lot of inline checks. This bloats code size impacting icache and has a visible impact on Wasm-sandboxed code. For example, in one of the Firefox code paths that is indirect call-heavy on a Wasm-sandboxed libexpat (for XML parsing), this code change improved code performance by 4.33% (i.e. eliminated 13.5% of wasm2c's overhead over native code in this benchmark)

The changes in this PR are

• Remove the unnecessary null check on the function pointer in CHECK_CALL_INDIRECT and null check on the local function type. This will generally be populated by Wasm2c runtime code, and will never populated to be null
• Move the slowpath for call_indirect type checking (the code paths used when invoking a funcref whose type was created in a different module) into a non-inline function to reduce the call_indirect inline footprint

This PR does not change any functionality; it only optimizes performance.

(Note this PR can be landed independently of other perf PRs I submit. I will rebase PRs depending on which land first)

[zherczeg]

I know very little about wasm2c, so I am not the best reviewer for this part. I have experience with code optimizations though.

[zherczeg]

If Garbage Collection will ever be part of wabt, do you plan to support it in wasm2c? Type checking will be affected by GC.

[shravanrn]

If Garbage Collection will ever be part of wabt, do you plan to support it in wasm2c? Type checking will be affected by GC.

This is not on my todo list for the forseeable future and I haven't heard anything from this other contributors about this. Maybe @keithw or @sbc100 know more.

After the performance improvements, my next two features are probably uvwasi support and threads support, although I don't really have a timeline on these as this will depend on when I am not too busy with other responsibilities.

[keithw]

(1) I think it's awesome that you're diving into these performance corners! I really liked your talk yesterday.

(2) I am a little surprised by these results given that (at the time I did this call_indirect optimization) I had it benchmarked down to individual instruction counts (#2120) and the penalty from cross-module call_indirect was basically zero (when the linker is able to merge the string constants, so it's just a pointer equality test) or 1-1.5 cycles (when the CPU has to do the actual 256-bit equality test).

See discussion on #2120 for more (I certainly had to refresh my memory).

Do you think what you're seeing now is consistent with that or has this bitrotted somehow? And, wrt Firefox, is it possible to see if the linker is merging the string constants (as I would have expected)? Or is the idea that it is merging the string constants, so in practice it is just a 64-bit equality test, but just "having the AVX2 256-bit equality test" inline is what's blowing out the icache in production?

[shravanrn]

but just "having the AVX2 256-bit equality test" inline is what's blowing out the icache in production?

@keithw Yup, it's the icache blowout. I don't think my tests exercise the memcmp part of the code at all and Firefox only uses a single Wasm module (so merges the strings). The exact benchmark for XML parsing involves calling back into the application through an indirect call each time an XML element or attribute is encountered during the parse. This makes the call indirect path extremely hot (millions of calls for a large XML document/SVG file). I don't believe this is bitrot or the original discussion was incorrect with respect to the compiler/linker/microbenchmark aspects, but I think what I learned was that the micro-architectural aspects in a large application/Wasm module (macrobenchmark) seem to have non trivial performance impact.

[shravanrn]

but just "having the AVX2 256-bit equality test" inline is what's blowing out the icache in production?

@keithw Caveat to the above. I suppose the slowdown could be a mix of other micro-architectural effect that I haven't checked as well beyond just the icache (e.g., speculative execution prefers the non branching path, runs AVX instructions followed by a rollback which could be extra slow 🤷 etc.). But, either way, the fix is the same --- move the AVX instructions more clearly into a slow/cold code path

[zherczeg]

There is something I don't understand. All function types should be known at compile time. Hence wasm_rt_func_type_t should simply be a type index (unsigned int). There should be no HASH comparison or anything. Functions could have type descriptors though, but they should not be needed for any type comparison.

[shravanrn]

There is something I don't understand. All function types should be known at compile time. Hence wasm_rt_func_type_t should simply be a type index (unsigned int). There should be no HASH comparison or anything. Functions could have type descriptors though, but they should not be needed for any type comparison.

@zherczeg I believe some of the logic was documented in #2120

The main issue is how you can assign a unique number for all the function types you will encounter, and there are a few options that almost work, but most of these end up having issues of performance or compatibility

1. Fresh incrementing integer --- Start a counter at 0. As you see a new type give this type a function type of the current value of counter. The problem is that the function types assigned are not synchronized outside of the current module, and each module has its own idea of what type id 1 ones. This would break the multi-module cases for wasm2c.

2. Each unique function type has a fixed numeric mapping --- E.g., (u32, u32) -> u32 is always 111 or (f32, f32) -> f32 is always 222 etc. The problem with this approach is that given that each function can have an arbitrary number of parameters, but the numeric we pick will have a limit ===> this places artificial cap on how general a function this can represent. Additionally, when looking at different mapping schemes with a numeric of uint64_t and accounting for all the different Wasm types along with things like multiple returns, the cap becomes rather small and the design is not very future proof.

3. Function types are assigned by a shared registry at runtime --- function indexes are never set at compile time, but the Wasm2c runtime layer maintains a runtime version of the counter in Option 1 that is shared across modules. This option has runtime costs because a call-indirect Wasm instruction now has to query the registry at runtime prior to executing the CALL_INDIRECT macro, and a hashmap lookup at each call indirect would dwarf any of the overheads we are trying to fix.

I think all this let @keithw converge on the design he implemented in wasm2c

4. Each unique function type has a variable size string mapping --- Effectively something like (u32, u32) -> u32 or (f32, f32) -> f32 would be a string we compare for type equality. However, doing an unbounded string equality has its own performance concerns, so hashing this with SHA256 to a fixed 32-byte array was chosen as the best approach. (The hash technically introduces a negligible chance for collision, but the odds of this actually happening are astronomical, so this is not a real concern). Finally the 32-byte array was preserved as a string so that the natural string de-duplication in C's compile and link, would allow us to implement a pointer fast path of direct pointer comparison.

[zherczeg]

The problem with collision is that if it ever happens, you cannot fix it.

What about option 1 with a header file, where all types are in a generated header file? The header is parsed/extended as necessary.

[shravanrn]

What about option 1 with a header file, where all types are in a generated header file? The header is parsed/extended as necessary.

This would require compilation of all modules to occur

• in a shared environment where this header file can be shared and extended amongst the modules
• to be sequentialized in some way and will likely mean anyone doing this has to modify their build system

The current design has the advantage that any Wasm module can be compiled independently and will continue to work.

Also, I do want to be cautious that we don't detour this PR into a full redesign of call-indirect. If anyone is interested in doing the work for that, I am happy to discuss designs for that in a proposed PR/bug for that. Right now, I just want to keep the changes minimal so we improve performance with the design we have in place

Note Cleaned up the design a bit more.

[zherczeg]

The code looks reasonable. LGTM