-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
Optimize writing small strings #8149
Optimize writing small strings #8149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea for the optimization is neat and the savings for some string lengths seem significant.
On the other hand, I don't like that the complexity of the methods has increased (several cases to handle + some ifdefs) and that we now have 2 copies of basically the same code under the unsafe block - isn't there a way to refactor so that we end have just one copy of that snippet? (reorganize the method, introduce a static method, ...).
Alternative idea (not sure if its good or not): the method could be modified so that you simply skip Utf8Encoding.GetByteCount(value);
and WriteLength
if you know the length is going to fit into a single byte - this might result in a smaller method without code duplication and might be both faster and easier to read.
598e3b7
to
6b2595e
Compare
@jtattermusch Updated. Code is a lot smaller. There is some cleverness going on with position but I put a comment on it to explain what is going on. |
de8d196
to
7a39739
Compare
After:
Before:
|
b40b1f7
to
cb246b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good with one minor comment.
Is the new version (with the new WriteStringToBuffer method) faster, slower or the same as the previous version of this PR?
The same - #8149 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
the windows C# build error seems unrelated: https://fusion2.corp.google.com/invocations/fa716574-5e3f-4a95-a360-2ae376505e58 (look like an infrastructure problem with the windows workers). Other test failures are also unrelated. |
If a string is 42 characters or less then its length will always fit in 1 byte. If there is space available in the buffer, write directly to the buffer, then write the length afterwards based on the number of bytes written. Avoids having to calculate the size.
Slightly slower for 1 byte ASCII strings, faster for all other strings 42 chars or less.
@jtattermusch
Before
After