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

Reduce string encoding/decoding in MessagePackFormatter #594

Merged
merged 2 commits into from
Oct 29, 2020

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Oct 28, 2020

Several strings are very common in JSON-RPC. Rather than encode/decode them repeatedly, allocating new strings each time we decode, we instead use pre-encoded bytes and recognize the encoded bytes without decoding them.

The techniques used here come from MessagePack's own mpc-tool code output. So even though we're using MessagePack's "internal" namespace, it needs to be stable within a major version to keep mpc generated code working.

I recognize that this compromises readability, but perf traces indicate that the decoding time and allocation costs are significant, so I believe it is justified. I define constants and add code comments to help explain it.

Also note (because I only learned this recently) that in C# the syntax below produces very efficient code. It never allocates an array -- ever. Not even once. Read the IL yourself. This syntax leads the C# compiler to generate code that initializes the ReadOnlySpan<byte> to point directly into the binary of the loaded assembly itself to get at the bytes in the compiled binary.

static ReadOnlySpan<byte> VersionPropertyNameBytes => new byte[] { 167, 106, 115, 111, 110, 114, 112, 99 };

I did not optimize every single formatter in the MessagePackFormatter.cs file. Just the hot path ones. We can optimize the error formatter if that becomes important.

Several strings are *very* common in JSON-RPC. Rather than encode/decode them repeatedly, allocating new strings each time we decode, we instead use pre-encoded bytes and recognize the encoded bytes without decoding them.

The techniques used here come from MessagePack's own mpc-tool code output. So even though we're using MessagePack's "internal" namespace, it needs to be stable within a major version to keep mpc generated code working.
- [Sample deserialization decoder optimization](https://github.com/neuecc/MessagePack-CSharp/blob/896ed3c6bb3264bbda5c9a410a27d61b4cf1d93e/sandbox/Sandbox/Generated.cs#L1553-L1597)
- [Sample serialization encoder optimization](https://github.com/neuecc/MessagePack-CSharp/blob/896ed3c6bb3264bbda5c9a410a27d61b4cf1d93e/sandbox/Sandbox/Generated.cs#L1519-L1531)
@AArnott AArnott added this to the v2.7 milestone Oct 28, 2020
@AArnott AArnott self-assigned this Oct 28, 2020
@codecov-io
Copy link

codecov-io commented Oct 28, 2020

Codecov Report

Merging #594 into master will decrease coverage by 0.02%.
The diff coverage is 94.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
- Coverage   89.93%   89.91%   -0.03%     
==========================================
  Files          54       54              
  Lines        4492     4522      +30     
==========================================
+ Hits         4040     4066      +26     
- Misses        452      456       +4     
Impacted Files Coverage Δ
src/StreamJsonRpc/MessagePackFormatter.cs 91.56% <94.56%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 154a566...105f453. Read the comment docs.

src/StreamJsonRpc/MessagePackFormatter.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc/MessagePackFormatter.cs Show resolved Hide resolved
src/StreamJsonRpc/MessagePackFormatter.cs Outdated Show resolved Hide resolved
Copy link
Member

@BertanAygun BertanAygun left a comment

Choose a reason for hiding this comment

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

Approving based on comments

@AArnott AArnott merged commit ac64578 into microsoft:master Oct 29, 2020
@AArnott AArnott deleted the perf branch October 29, 2020 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants