Remove unnecessary checks in ConsumeNumber (#33294) #34500
Conversation
There is still more work pending related to that issue, correct? |
src/System.Text.Json/src/System/Text/Json/Utf8JsonReader.MultiSegment.cs
Outdated
Show resolved
Hide resolved
|
@dotnet-bot test code coverage please |
In #33294 I made some other suggestions that may improve perf. They are more niche and should be thoroughly benchmarked first. I suggest to investigate them seperately and merge this PR as is (once the issue regarding |
Of course. I am fine with this being merged in, but I was just pointing out that we can't yet close the issue. |
|
If CI is happy as well this is ready to merge |
|
@dotnet-bot test Linux x64 Release Build |
…net/corefx#34500) * Remove unnecessary checks in ConsumeNumber (dotnet/corefx#33294) * Remove unnecessary _isNotPrimitive check Commit migrated from dotnet/corefx@d2b28cc
Summary
Removes redundant checks in
ConsumeNumber/ConsumeNumberMultiSegmentwhich are ensured as postconditions of the wrapped methodTryGetNumber/TryGetNumberMultiSegment. This adresses the most obvious optimization of #33294 (and fixes two typos in associated comments).ConsumeNumber
TryGetNumberreturnstrueif and only if one of the following expressions is true:_consumed < _buffer.Length && JsonConstants.Delimiters.IndexOf(_buffer[_consumed]) >= 0_consumed >= _buffer.Length && IsLastSpanThe first expression represents the standard case; there is data left, and the parsed number is terminated with a delimiter. The second expression is only valid if a "primitive" (payload consisting of only a number) is being parsed.
The checks in
ConsumeNumberhave been reduced to the required minimum to ensure correctness. Especially the not inexpensive checkJsonConstants.Delimiters.IndexOf(_buffer[_consumed]) >= 0could be removed (replaced with an assertion). I figured the introduced assertions make sense to ensure the postconditions ofTryGetNumberdon't change unnoticed.ConsumeNumberMultiSegment
A similar situation presents itself with
TryGetNumberMultiSegment. It returnstrueif and only if one of the following expressions is true:_consumed < _buffer.Length && JsonConstants.Delimiters.IndexOf(_buffer[_consumed]) >= 0_consumed >= _buffer.Length && IsLastSpan && !GetNextSpan()_consumed >= _buffer.Length && IsLastSpanThe first expression represents the standard case; there is data left, and the parsed number is terminated with a delimiter. The second and third expression is only valid if a "primitive" (payload consisting of only a number) is being parsed.
The checks in
ConsumeNumberMultiSegmenthave been reduced to the required minimum to ensure correctness. Especially the not inexpensive checkJsonConstants.Delimiters.IndexOf(_buffer[_consumed]) >= 0could be removed (replaced with an assertion). I figured the introduced assertions make sense to ensure the postconditions ofTryGetNumberMultiSegmentdon't change unnoticed./cc @ahsonkhan