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

Intern strings when using the MessagePackFormatter #907

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Apr 13, 2023

This feature is available in MessagePack but not on by default because it has a (small) perf cost when deserializing. But we want it on for RPC by default.

String interning means that we reuse string objects that are equal by value rather than creating new ones. We don't use string.Intern here because that keeps them in memory forever, and because that requires allocating a string before switching it for the long-lived instance. The way MessagePack (and this PR) does it requires no allocations at all if the string has been interned before. And the interning collection (which comes from MSBuild) is a weak intern, meaning string will not be held in memory for the life of the process.

I also use my CommonString type in a few more places. This has the effect of never deserializing even the first string, nor taking the perf hit of an interned string lookup. It memorizes the encoding and decoding between chars and bytes and reuses them each time.

@AArnott AArnott added this to the v2.16 milestone Apr 13, 2023
BertanAygun
BertanAygun previously approved these changes Apr 13, 2023
@BertanAygun
Copy link
Member

Probably should have asked before approving but wouldn't this considered a protocol breaking change?

@BertanAygun BertanAygun self-requested a review April 13, 2023 19:01
@BertanAygun BertanAygun dismissed their stale review April 13, 2023 19:01

Trying to undo my own approval.

@AArnott
Copy link
Member Author

AArnott commented Apr 13, 2023

No. This doesn't impact the wire protocol at all. It's strictly an implementation detail on how we read (and write) the strings.

@AArnott AArnott merged commit 42a95c0 into microsoft:main Apr 13, 2023
@AArnott AArnott deleted the internStrings branch April 13, 2023 22:01
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