Skip to content

Optimize writing small strings#8149

Merged
jtattermusch merged 2 commits intoprotocolbuffers:masterfrom
JamesNK:jamesnk/writestring-small
Jan 28, 2021
Merged

Optimize writing small strings#8149
jtattermusch merged 2 commits intoprotocolbuffers:masterfrom
JamesNK:jamesnk/writestring-small

Conversation

@JamesNK
Copy link
Copy Markdown
Contributor

@JamesNK JamesNK commented Dec 16, 2020 •
edited
Loading

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
thomasvl
thomasvl reviewed Dec 17, 2020
View reviewed changes
Comment thread csharp/src/Google.Protobuf/WritingPrimitives.cs
@jtattermusch jtattermusch self-assigned this Jan 14, 2021
jtattermusch
jtattermusch reviewed Jan 26, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread csharp/src/Google.Protobuf/WritingPrimitives.cs Outdated
Comment thread csharp/src/Google.Protobuf/WritingPrimitives.cs
Comment thread csharp/src/Google.Protobuf/WritingPrimitives.cs
@JamesNK JamesNK force-pushed the jamesnk/writestring-small branch from 598e3b7 to 6b2595e Compare January 26, 2021 20:43
@JamesNK
Copy link
Copy Markdown
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 JamesNK force-pushed the jamesnk/writestring-small branch from de8d196 to 7a39739 Compare January 26, 2021 23:03
@JamesNK
Copy link
Copy Markdown
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 |     - |     - |     - |         - |

@JamesNK JamesNK force-pushed the jamesnk/writestring-small branch from b40b1f7 to cb246b8 Compare January 26, 2021 23:51
jtattermusch
jtattermusch reviewed Jan 27, 2021
View reviewed changes
Copy link
Copy Markdown
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?

Comment thread csharp/src/Google.Protobuf/WritingPrimitives.cs Outdated
@JamesNK
Copy link
Copy Markdown
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)

jtattermusch
jtattermusch approved these changes Jan 28, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

jtattermusch commented Jan 28, 2021

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

Reviewers

@thomasvl thomasvl thomasvl left review comments

+1 more reviewer

@jtattermusch jtattermusch jtattermusch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jtattermusch jtattermusch

Labels

c# cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JamesNK @jtattermusch @thomasvl @protobuf-kokoro