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

Optimize writing small strings #8149

Merged

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented Dec 16, 2020

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

|                           Method | BytesToWrite | encodedSize |       Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------------- |------------- |------------ |-----------:|----------:|----------:|------:|------:|------:|----------:|
|         WriteString_WriteContext |        10080 |           1 | 149.235 us | 1.5862 us | 1.4061 us |     - |     - |     - |       2 B |
|         WriteString_WriteContext |        10080 |           4 |  48.668 us | 0.4787 us | 0.4478 us |     - |     - |     - |       1 B |
| WriteNonAsciiString_WriteContext |        10080 |           4 |  99.273 us | 1.4526 us | 1.2877 us |     - |     - |     - |       1 B |
|         WriteString_WriteContext |        10080 |          10 |  25.254 us | 0.2668 us | 0.2365 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |          10 |  61.336 us | 0.3819 us | 0.3573 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |         105 |  11.158 us | 0.1050 us | 0.0931 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |         105 |   9.676 us | 0.1001 us | 0.0937 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |       10080 |   8.997 us | 0.0433 us | 0.0405 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |       10080 |   4.459 us | 0.0446 us | 0.0418 us |     - |     - |     - |         - |

After

|                           Method | BytesToWrite | encodedSize |       Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------------- |------------- |------------ |-----------:|----------:|----------:|------:|------:|------:|----------:|
|         WriteString_WriteContext |        10080 |           1 | 173.719 us | 2.6596 us | 2.4878 us |     - |     - |     - |       2 B |
|         WriteString_WriteContext |        10080 |           4 |  44.960 us | 0.6183 us | 0.5481 us |     - |     - |     - |       1 B |
| WriteNonAsciiString_WriteContext |        10080 |           4 |  56.988 us | 0.2900 us | 0.2571 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |          10 |  19.008 us | 0.3009 us | 0.2349 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |          10 |  24.667 us | 0.2239 us | 0.2094 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |         105 |  11.563 us | 0.0841 us | 0.0745 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |         105 |   9.856 us | 0.0580 us | 0.0543 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |       10080 |   9.143 us | 0.0960 us | 0.0851 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |       10080 |   4.458 us | 0.0306 us | 0.0256 us |     - |     - |     - |         - |

@google-cla google-cla bot added the cla: yes label Dec 16, 2020
Copy link
Contributor

@jtattermusch jtattermusch left a 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.

csharp/src/Google.Protobuf/WritingPrimitives.cs Outdated Show resolved Hide resolved
@JamesNK
Copy link
Contributor Author

JamesNK commented Jan 26, 2021

@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.

@JamesNK
Copy link
Contributor Author

JamesNK commented Jan 26, 2021

After:

|                           Method | BytesToWrite | encodedSize |       Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------------- |------------- |------------ |-----------:|----------:|----------:|------:|------:|------:|----------:|
|         WriteString_WriteContext |        10080 |           1 | 197.933 us | 3.4468 us | 3.2242 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |           4 |  51.337 us | 0.2081 us | 0.1845 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |           4 |  63.139 us | 0.3039 us | 0.2843 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |          10 |  21.122 us | 0.2113 us | 0.1977 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |          10 |  27.255 us | 0.5020 us | 0.4450 us |     - |     - |     - |         - |

Before:

|                           Method | BytesToWrite | encodedSize |       Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------------- |------------- |------------ |-----------:|----------:|----------:|------:|------:|------:|----------:|
|         WriteString_WriteContext |        10080 |           1 | 147.735 us | 0.7897 us | 0.7001 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |           4 |  48.229 us | 0.7844 us | 0.6953 us |     - |     - |     - |       1 B |
| WriteNonAsciiString_WriteContext |        10080 |           4 | 117.600 us | 0.6227 us | 0.5520 us |     - |     - |     - |         - |
|         WriteString_WriteContext |        10080 |          10 |  25.140 us | 0.1824 us | 0.1706 us |     - |     - |     - |         - |
| WriteNonAsciiString_WriteContext |        10080 |          10 |  42.219 us | 0.6843 us | 0.6401 us |     - |     - |     - |         - |

Copy link
Contributor

@jtattermusch jtattermusch left a 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?

csharp/src/Google.Protobuf/WritingPrimitives.cs Outdated Show resolved Hide resolved
@JamesNK
Copy link
Contributor Author

JamesNK commented Jan 27, 2021

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)

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@jtattermusch
Copy link
Contributor

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.

@jtattermusch jtattermusch merged commit 4140735 into protocolbuffers:master Jan 28, 2021
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.

None yet

4 participants