-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove string allocations in VersionConverter
in System.Text.Json
#55179
Comments
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsDescriptionCurrent implementation of ConfigurationSee below in Benchmark results section Benchmark comparing current and not-allocating implementation of
|
Method | unparsedVersion | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|
CurrentConverterSerialize | 1.0 | 441.3 ns | 0.22 ns | 0.20 ns | 0.1526 | - | - | 240 B |
ProposedConverterSerialize | 1.0 | 430.0 ns | 0.44 ns | 0.41 ns | 0.1326 | - | - | 208 B |
CurrentConverterDeserialize | 1.0 | 487.3 ns | 0.13 ns | 0.11 ns | 0.0553 | - | - | 88 B |
ProposedConverterDeserialize | 1.0 | 434.7 ns | 0.27 ns | 0.24 ns | 0.0353 | - | - | 56 B |
CurrentConverterSerialize | 1.0.0 | 450.4 ns | 0.36 ns | 0.32 ns | 0.1578 | - | - | 248 B |
ProposedConverterSerialize | 1.0.0 | 458.7 ns | 0.21 ns | 0.18 ns | 0.1373 | - | - | 216 B |
CurrentConverterDeserialize | 1.0.0 | 520.6 ns | 1.54 ns | 1.20 ns | 0.0553 | - | - | 88 B |
ProposedConverterDeserialize | 1.0.0 | 465.3 ns | 0.17 ns | 0.14 ns | 0.0353 | - | - | 56 B |
CurrentConverterSerialize | 1.0.0.0 | 467.4 ns | 0.51 ns | 0.48 ns | 0.1631 | - | - | 256 B |
ProposedConverterSerialize | 1.0.0.0 | 460.8 ns | 0.30 ns | 0.27 ns | 0.1373 | - | - | 216 B |
CurrentConverterDeserialize | 1.0.0.0 | 557.7 ns | 0.33 ns | 0.31 ns | 0.0610 | - | - | 96 B |
ProposedConverterDeserialize | 1.0.0.0 | 470.9 ns | 0.09 ns | 0.08 ns | 0.0353 | - | - | 56 B |
CurrentConverterSerialize | 21474(...)83647 [21] | 499.3 ns | 0.58 ns | 0.54 ns | 0.1984 | - | - | 312 B |
ProposedConverterSerialize | 21474(...)83647 [21] | 491.3 ns | 0.49 ns | 0.41 ns | 0.1574 | - | - | 248 B |
CurrentConverterDeserialize | 21474(...)83647 [21] | 566.1 ns | 0.55 ns | 0.49 ns | 0.0763 | - | - | 120 B |
ProposedConverterDeserialize | 21474(...)83647 [21] | 522.3 ns | 0.21 ns | 0.19 ns | 0.0353 | - | - | 56 B |
CurrentConverterSerialize | 21474(...)83647 [32] | 532.0 ns | 0.91 ns | 0.81 ns | 0.2289 | - | - | 360 B |
ProposedConverterSerialize | 21474(...)83647 [32] | 507.1 ns | 0.40 ns | 0.36 ns | 0.1726 | - | - | 272 B |
CurrentConverterDeserialize | 21474(...)83647 [32] | 627.8 ns | 0.30 ns | 0.28 ns | 0.0916 | - | - | 144 B |
ProposedConverterDeserialize | 21474(...)83647 [32] | 534.8 ns | 0.29 ns | 0.28 ns | 0.0353 | - | - | 56 B |
CurrentConverterSerialize | 21474(...)83647 [43] | 583.9 ns | 0.40 ns | 0.38 ns | 0.2546 | - | - | 400 B |
ProposedConverterSerialize | 21474(...)83647 [43] | 558.4 ns | 0.46 ns | 0.41 ns | 0.1831 | - | - | 288 B |
CurrentConverterDeserialize | 21474(...)83647 [43] | 692.8 ns | 0.49 ns | 0.44 ns | 0.1068 | - | - | 168 B |
ProposedConverterDeserialize | 21474(...)83647 [43] | 584.8 ns | 0.18 ns | 0.14 ns | 0.0353 | - | - | 56 B |
Analysis
Since memory allocated on deserialization is constant i assume it's cost of simply calling JsonSerializer.Deserialize
and allocating Version
and VersionWrapper
objects.
However, memory allocated on serialization isn't constant, even if it's reduced by a bit, which is caused by needing to allocate string to handle result serialization.
Note
In case if this will be approved i would like to create PR to implement this.
Author: | N0D4N |
---|---|
Assignees: | - |
Labels: |
|
Milestone: | - |
Since this is not an API proposal there's no approval process as such other than reviewing the pull request. @layomia do you think we could accept such a PR in time for .NET 6 or should we move this to the 7.0.0 milestone? |
👍 yeah we could accept a PR optimizing the implementation in .NET 6.0, esp if it comes soon-ish. |
Marking as 7.0.0 for now until a PR comes in. |
Thanks, I'll start working on it as soon as i can. |
Description
Current implementation of
System.Text.Json.Serialization.Converters.VersionConverter
allocates string on reading and on writing. However, we can remove this allocations and don't allocate insideVersionConverter
at all.In case of writing we cat take advantage of
Version.TryFormat
that acceptsSpan<char>
, and since we know format/max string length ofVersion
object instance, we canstackalloc char[]
such span.In case of reading we can separate raw bytes by
'.'
, parse everything between them asInt32
, and pass thoseint
's toVersion
constructor.So to prove this i created draft, custom, no-allocating implementation of
VersionConverter
, that works faster and reduces allocated memory. All code of benchmark, benchmark results and tests that confirm that current and custom implementation ofVersionConverter
give the same output are available in this repoConfiguration
See below in Benchmark results section
Benchmark comparing current and not-allocating implementation of
VersionConverter
Click to see code
Benchmark results
Click to see benchmark results
Note
OS=neon 20.04
means KDE Neon which is based on Ubuntu 20.04 LTSAnalysis
Since memory allocated on deserialization is constant i assume it's cost of simply calling
JsonSerializer.Deserialize
and allocatingVersion
andVersionWrapper
objects.However, memory allocated on serialization isn't constant, even if it's reduced by a bit, which is caused by needing to allocate string to handle result serialization.
Note
In case if this will be approved i would like to create PR to implement this.
The text was updated successfully, but these errors were encountered: