Skip to content
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

Add MessagePackSerializer.MaxAsyncBuffer to speed up small async deserializations #159

Conversation

AArnott
Copy link
Owner

@AArnott AArnott commented Dec 11, 2024

Since synchronous deserialization is substantially faster than async deserialization, we prefer sync. But sync requires that all msgpack data be pre-buffered. That's a reasonable trade-off, assuming the msgpack data is reasonably small. When it is large, the slower async path may be preferable to avoid unbounded memory consumption.

Closes #155

Before

Method Mean Error StdDev Ratio RatioSD Gen0 Gen1 Allocated Alloc Ratio
Sync_Deserialize 111.4 us 19.72 us 1.08 us 1.00 0.01 11.4746 1.9531 70.34 KB 1.00
Async_Deserialize 327.0 us 6.20 us 0.34 us 2.94 0.02 11.2305 1.9531 70.49 KB 1.00

After

Method Categories Mean Error StdDev Ratio Gen0 Gen1 Allocated Alloc Ratio
Sync_Deserialize map-init,Deserialize 114.37 μs 10.251 μs 0.562 μs 1.00 11.4746 1.9531 72024 B 1.00
Async_Deserialize map-init,Deserialize 104.00 μs 2.420 μs 0.133 μs 0.91 11.4746 1.9531 72080 B 1.00

Not all benchmarks show 0 tax for async deserialization for small jobs. The above works because it's a reasonably large array but still under the threshold for synchronous deserialization. But very tiny deserializations still pay a tax due to buffering overhead (even if it's already in memory) that sync deserialization APIs can avoid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 6 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • src/Nerdbank.MessagePack/net8.0/PublicAPI.Unshipped.txt: Language not supported
  • src/Nerdbank.MessagePack/netstandard2.0/PublicAPI.Unshipped.txt: Language not supported
Comments skipped due to low confidence (4)

test/Nerdbank.MessagePack.Tests/AsyncSerializationTests.cs:48

  • The word 'sufficently' is misspelled. It should be 'sufficiently'.
// Verify that with a sufficently low async buffer, the async paths are taken.

test/Nerdbank.MessagePack.Tests/AsyncSerializationTests.cs:105

  • [nitpick] The property name 'AsyncDeserializationCounter' should be 'asyncDeserializationCounter' to follow naming conventions.
internal int AsyncDeserializationCounter { get; set; }

src/Nerdbank.MessagePack/MessagePackSerializer.cs:39

  • [nitpick] Consider using a constant or a more descriptive variable name instead of the magic number 1 * 1024 * 1024 for better readability.
private int maxAsyncBuffer = 1 * 1024 * 1024;

src/Nerdbank.MessagePack/MessagePackSerializer.cs:665

  • Ensure that the new behavior introduced with MaxAsyncBuffer is covered by tests to verify its functionality.
private async ValueTask<T?> DeserializeAsync<T>(PipeReader reader, ITypeShapeProvider provider, MessagePackConverter<T> converter, CancellationToken cancellationToken)
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.95%. Comparing base (445be10) to head (9996e32).

Files with missing lines Patch % Lines
...Nerdbank.MessagePack.Tests/FragmentedPipeReader.cs 84.21% 3 Missing ⚠️
...dbank.MessagePack.Tests/AsyncSerializationTests.cs 91.30% 2 Missing ⚠️
src/Nerdbank.MessagePack/MessagePackSerializer.cs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   81.42%   76.95%   -4.48%     
==========================================
  Files         146      145       -1     
  Lines       10907    10534     -373     
  Branches     1519     1466      -53     
==========================================
- Hits         8881     8106     -775     
- Misses       1957     1991      +34     
- Partials       69      437     +368     
Flag Coverage Δ
Linux 76.95% <92.20%> (+0.25%) ⬆️
Windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…serializations

Since synchronous deserialization is substantially faster than async deserialization, we prefer sync. But sync requires that all msgpack data be pre-buffered. That's a reasonable trade-off, assuming the msgpack data is reasonably small. When it is large, the slower async path may be preferable to avoid unbounded memory consumption.

Closes #155
@AArnott AArnott force-pushed the 155-buffer-at-least-some-threshold-of-bytes-before-proceeding-with-async-deserialization branch from c27975c to 38d6680 Compare December 11, 2024 17:33
@AArnott AArnott added the enhancement New feature or request label Dec 11, 2024
@AArnott
Copy link
Owner Author

AArnott commented Dec 11, 2024

CC: @eiriktsarpalis

@AArnott AArnott merged commit 8dcd403 into main Dec 11, 2024
2 checks passed
@AArnott AArnott deleted the 155-buffer-at-least-some-threshold-of-bytes-before-proceeding-with-async-deserialization branch December 11, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer at least some threshold of bytes before proceeding with async deserialization
1 participant