Switch JsonReaderHelper.IndexOfQuoteOrAnyControlOrBackSlash to use IndexOfAnyValues #82789
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephentoub
added
NO-MERGE
|
/benchmark list |
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue Detailsnull
|
|
Crank Pull Request Bot
Benchmarks:
Profiles:
Components:
Arguments: any additional arguments to pass through to crank, e.g. |
|
/benchmark microbenchmarks aspnet-perf-win libs --variable filter="System.Text.Json.Tests*" |
|
Benchmark started for microbenchmarks on aspnet-perf-win with libs and arguments |
MihaZupan
reviewed
Feb 28, 2023
There was a problem hiding this comment.
For this set of values, we have fast implementations for x86 and ARM64 right now.
Do you know what the performance numbers look like on WASM? I would imagine this will regress @radekdoulik's improvements from #81758.
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.netstandard.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.net8.cs
Show resolved
Hide resolved
I don't know. But if this regresses wasm, presumably many of the other 50+ places we have that use IndexOfAnyValues are also degraded on wasm. What would it take to make this work well on wasm? |
|
Also, if a) this does regress wasm, and b) we for whatever reason can't extend this implementation in IndexOfAnyValues to optimize wasm in addition to x86/arm, I'd still want to make this change to System.Text.Json, and instead augment IndexOfAnyValues to have a specialized implementation for this pattern, effectively moving the implementation from json into corelib. If we're going to carry it around, we might as well get the benefits from it with anyone else who happens to need something similar. |
stephentoub
force-pushed
the
jsonindexofany
branch
from
549962d
to
236eea9
Compare
Assuming the browser ends up translating the WASM instructions into something reasonable, it shouldn't be all that difficult to extend It seems like a worthwhile investment given the many places that would benefit through |
|
The WASM issue aside (which we need to address), here are the crank results from running the benchmarks. I don't see anything that stands out as being problematic from switching to use IndexOfAnyValues:
|
|
(Obviously untested) this is about what it would take to make The only missing thing is exposing the This is all assuming the behavior for these intrinsics (swizzle, narrow) is as described in https://emscripten.org/docs/porting/simd.html, https://github.com/WebAssembly/simd/blob/main/proposals/simd/SIMD.md. |
MihaZupan
approved these changes
Mar 2, 2023
There was a problem hiding this comment.
I agree with #82789 (comment)
Even if we can't do #82789 (comment) for whatever reason, we can still move the "less than or 2 special" implementation into IndexOfAnyValues directly.
|
Running benchmarks again after #82866 with All the numbers
|
|
@radekdoulik, could we implement the necessary wasm primitives here to make this work? |
Yes. I will try to add it quickly. |
Excellent, thanks. That should help not only simplify and improve this case, but the other dozens of places IndexOfAnyValues is used and would then end up picking this ASCII-optimized searcher. |
radekdoulik
added a commit
to radekdoulik/runtime
that referenced
this pull request
Mar 7, 2023
Add them as internal as the approved API contains wrong methods for these. dotnet#53730 (comment) This will allow faster implementation of IndexOfAnyValues for wasm. dotnet#82789 (comment)
radekdoulik
added a commit
that referenced
this pull request
Mar 7, 2023
* [wasm] Add narrow methods to PackedSimd Add them as internal as the approved API contains wrong methods for these. #53730 (comment) This will allow faster implementation of IndexOfAnyValues for wasm. #82789 (comment) * Fix build
stephentoub
removed
the
NO-MERGE
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.