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.

Sign up for GitHub

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

Jump to bottom

Improve performance of parsing repeated fixed sized types #7412

Merged
merged 1 commit into from Jun 3, 2020
Merged

Improve performance of parsing repeated fixed sized types #7412

merged 1 commit into from Jun 3, 2020

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented Apr 23, 2020 •
edited

Based on #7351. Will rebase on master once it is merged.

Actual changes: JamesNK@d0d576e

@jtattermusch

Before:

|                                                          Method | messageCount |        Mean |      Error |     StdDev | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|---------------------------------------------------------------- |------------- |------------:|-----------:|-----------:|------------:|------------:|------------:|--------------------:|
|                         RepeatedFieldMessage_ParseFromByteArray |            ? |    13.44 us |  0.1718 us |  0.1523 us |     21.4996 |           - |           - |            16.54 KB |
|                  RepeatedFieldMessage_ParseFromReadOnlySequence |            ? |    13.11 us |  0.0688 us |  0.0610 us |     21.2708 |           - |           - |            16.36 KB |
|        RepeatedFieldMessage_ParseDelimitedMessagesFromByteArray |           10 |   133.31 us |  1.8114 us |  1.6944 us |    212.6465 |           - |           - |           163.77 KB |
| RepeatedFieldMessage_ParseDelimitedMessagesFromReadOnlySequence |           10 |   126.78 us |  0.8992 us |  0.7971 us |    212.6465 |           - |           - |           163.59 KB |
|        RepeatedFieldMessage_ParseDelimitedMessagesFromByteArray |          100 | 1,332.55 us | 28.6365 us | 25.3855 us |   2126.9531 |           - |           - |          1636.12 KB |
| RepeatedFieldMessage_ParseDelimitedMessagesFromReadOnlySequence |          100 | 1,273.07 us | 13.5838 us | 10.6054 us |   2126.9531 |           - |           - |          1635.94 KB |

After:

|                                                          Method | messageCount |       Mean |      Error |     StdDev | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|---------------------------------------------------------------- |------------- |-----------:|-----------:|-----------:|------------:|------------:|------------:|--------------------:|
|                         RepeatedFieldMessage_ParseFromByteArray |            ? |   6.127 us |  0.0373 us |  0.0330 us |     10.6354 |           - |           - |             8.25 KB |
|                  RepeatedFieldMessage_ParseFromReadOnlySequence |            ? |   6.095 us |  0.0695 us |  0.0581 us |     10.4141 |           - |           - |             8.07 KB |
|        RepeatedFieldMessage_ParseDelimitedMessagesFromByteArray |           10 |  60.599 us |  0.5330 us |  0.4725 us |    105.2246 |           - |           - |            80.88 KB |
| RepeatedFieldMessage_ParseDelimitedMessagesFromReadOnlySequence |           10 |  59.789 us |  0.2847 us |  0.2377 us |    104.1260 |           - |           - |             80.7 KB |
|        RepeatedFieldMessage_ParseDelimitedMessagesFromByteArray |          100 | 622.160 us | 12.3938 us | 20.7072 us |   1041.0156 |           - |           - |           807.21 KB |
| RepeatedFieldMessage_ParseDelimitedMessagesFromReadOnlySequence |          100 | 597.545 us |  5.5646 us |  4.9328 us |   1041.0156 |           - |           - |           807.03 KB |

@jtattermusch
Copy link
Contributor

#7351 was merged, please rebase.

@JamesNK JamesNK force-pushed the jamesnk/repeated-fixed-parsing branch from d0d576e to 578aa1d Compare May 5, 2020 21:37
JamesNK
JamesNK commented May 5, 2020
View reviewed changes
csharp/src/Google.Protobuf.Test/ReadOnlySequenceFactory.cs
@@ -50,9 +50,9 @@ public static ReadOnlySequence<byte> CreateWithContent(byte[] data, int segmentS
while (currentIndex < data.Length)
{
var segment = new List<byte>();
for (; currentIndex < Math.Min(currentIndex + segmentSize, data.Length); currentIndex++)
while (segment.Count < segmentSize && currentIndex < data.Length)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI there was a bug here that created a sequence that started and ended with empty segments, but the actual content was a single segment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix!

jtattermusch
jtattermusch reviewed May 11, 2020
View reviewed changes
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.

Overall this is looking good, but left a few comments.
Thanks for the contribution!

csharp/src/Google.Protobuf/Collections/RepeatedField.cs Show resolved Hide resolved
csharp/src/Google.Protobuf/Collections/RepeatedField.cs Show resolved Hide resolved
csharp/src/Google.Protobuf/Collections/RepeatedField.cs Outdated Show resolved Hide resolved
csharp/src/Google.Protobuf.Benchmarks/ParseMessagesBenchmark.cs Outdated Show resolved Hide resolved
@jtattermusch jtattermusch self-assigned this May 11, 2020
jtattermusch
jtattermusch approved these changes May 11, 2020
View reviewed changes
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, thanks!

@JamesNK
Copy link
Contributor Author

JamesNK commented Jun 2, 2020

Mergable now that #7490 is resolved? @jtattermusch

jtattermusch
jtattermusch approved these changes Jun 2, 2020
View reviewed changes
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 with a very minor nit in the tests.

csharp/src/Google.Protobuf.Test/Collections/RepeatedFieldTest.cs Show resolved Hide resolved
csharp/src/Google.Protobuf.Test/ReadOnlySequenceFactory.cs
@@ -50,9 +50,9 @@ public static ReadOnlySequence<byte> CreateWithContent(byte[] data, int segmentS
while (currentIndex < data.Length)
{
var segment = new List<byte>();
for (; currentIndex < Math.Min(currentIndex + segmentSize, data.Length); currentIndex++)
while (segment.Count < segmentSize && currentIndex < data.Length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix!

@JamesNK JamesNK force-pushed the jamesnk/repeated-fixed-parsing branch from ba2bc4e to acc49aa Compare June 2, 2020 19:01
@JamesNK JamesNK force-pushed the jamesnk/repeated-fixed-parsing branch from e5de912 to 921bdaa Compare June 2, 2020 19:06
@jtattermusch jtattermusch merged commit efbadb6 into protocolbuffers:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtattermusch jtattermusch jtattermusch approved these changes

Assignees

@jtattermusch jtattermusch

Labels
c# cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JamesNK @jtattermusch @googlebot @protobuf-kokoro