-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[json] Use S.R.I vectors in JsonReaderHelper #81758
[json] Use S.R.I vectors in JsonReaderHelper #81758
Conversation
wasm performance improvement | measurement | new simd | old simd | nosimd | |-:|-:|-:|-:| | Json, non-ASCII text deserialize | 0.4234ms | 0.4280ms | 0.4234ms | | Json, small deserialize | 0.0376ms | 0.0402ms | 0.0397ms | | Json, large deserialize | 10.4231ms | 11.1614ms | 11.0021ms |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue Detailswasm performance improvement
|
Now to measure the CLR perf. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I think new private static readonly IndexOfAnyValues<byte> s_quoteOrAnyControlOrBackSlash = IndexOfAnyValues.Create(
"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F"u8 +
"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1A\x1B\x1C\x1D\x1E\x1F"u8 +
"\"\\"u8; And then replace the callsites of span.IndexOfAny(s_quoteOrAnyControlOrBackSlash); |
The failing build is unrelated and fails on main too https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-main-3fc9898d85424d66af/Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests/1/console.aa25b4ac.log?helixlogtype=result |
I think the
|
Were these results from WASM? If not, can you post the results for how much slower they were if you have them handy? For this set of values,
It's also worth noting that this |
If it's meaningfully faster, I'd be keen on moving the implementation into IndexOfAnyValues and then having Json just use IndexOfAnyValues. The nice thing about the IndexOfAnyValues.Create design is optimizing for this case doesn't increase the API surface area at all, and if it's a matter of shipping the custom vectorized routine in Json where it only benefits Json, or shipping the custom vectorized routine underneath IndexOfAnyValues where it then benefits both Json and anyone else who happens to have a similar pattern, I'd much rather get the larger benefit and keep the Json codebase smaller and custom-vectorization-free. If it's not meaningfully faster, then I'd be keen on deleting the implementation from Json and having it just use the existing algorithms underlying IndexOfAnyValues. Either way, it'd be nice to switch Json to using IndexOfAnyValues. |
These were measured on win/amd/x64. I am adding the detailed ouput below.
That would be fine too for wasm. The main motivation here was to use
If it uses platform specific SIMD API for arm/intel I can try to extend it to use S.R.I.Wasm.PackedSimd (which is not complete yet). Detailed results: summary:
|
The platform-specific bits of the implementation for ASCII bytes are here. The rest of the boilerplate can remain as-is. The code for ASCII chars also has some platform-specific optimizations around how the input can be packed from chars to bytes. |
Wow, it seems like indeed quite a few benchmarks improved - dotnet/perf-autofiling-issues#12941 nice! Also: |
@radekdoulik, could you share the change you measured here? I'm not able to reproduce the regressions you're seeing (at least not the ones I tried). |
wasm performance improvement