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

C#: Optimize parsing of some primitive and wrapper types #6843

Merged
merged 3 commits into from
Nov 5, 2019
Merged

C#: Optimize parsing of some primitive and wrapper types #6843

merged 3 commits into from
Nov 5, 2019

Conversation

chrisdunelm
Copy link
Contributor

@chrisdunelm chrisdunelm commented Nov 4, 2019 •
edited
Loading

Benchmark on my Windows 10 machine.

Before:

|               Method |     Mean |    Error |    StdDev |   Median | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|--------------------- |---------:|---------:|----------:|---------:|------------:|------------:|------------:|--------------------:|
|   ParseWrapperFields | 965.8 ns | 61.36 ns | 180.91 ns | 884.7 ns |      0.8869 |           - |           - |             1.82 KB |
| ParsePrimitiveFields | 409.3 ns | 14.55 ns |  41.98 ns | 393.8 ns |      0.4873 |           - |           - |                1 KB |

After:

|               Method |     Mean |     Error |    StdDev |   Median | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|--------------------- |---------:|----------:|----------:|---------:|------------:|------------:|------------:|--------------------:|
|   ParseWrapperFields | 692.2 ns | 40.284 ns | 118.78 ns | 660.4 ns |      0.8879 |           - |           - |             1.82 KB |
| ParsePrimitiveFields | 325.9 ns |  6.636 ns |  12.63 ns | 324.8 ns |      0.4878 |           - |           - |                1 KB |

@jtattermusch jtattermusch self-assigned this Nov 4, 2019
@jtattermusch jtattermusch self-requested a review November 4, 2019 17:23
@jtattermusch
Copy link
Contributor

I'll review the C# side, I'd like @gerben-s to check the correctness of the wrapper parsing optimizations.

@chrisdunelm chrisdunelm force-pushed the csharp_wrapper_primitive_opts branch from 6bb4109 to 2cd2abe Compare November 4, 2019 18:21
gerben-s
gerben-s reviewed Nov 4, 2019
View reviewed changes
Copy link
Contributor

@gerben-s gerben-s left a comment

Choose a reason for hiding this comment

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

What about the other wrappers? fixed32, int32 etc...

@chrisdunelm
Copy link
Contributor Author

All primitive wrapper types implemented.

@chrisdunelm chrisdunelm force-pushed the csharp_wrapper_primitive_opts branch from 2cd2abe to 2ac8946 Compare November 4, 2019 21:37
@chrisdunelm
Copy link
Contributor Author

@gerben-s PTAL

gerben-s
gerben-s approved these changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@gerben-s gerben-s left a comment

Choose a reason for hiding this comment

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

Overall I think that the code contains redundancies and could be factored better in time. But it's an excellent change to improve speed.

csharp/src/Google.Protobuf/CodedInputStream.cs
int shift = 0;
ulong result = 0;
while (shift < 64)
if (bufferPos + 10 <= bufferSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ReadRawVarint32 needs a similar change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ReadRawVarint32 already appears optimized I think?

@chrisdunelm chrisdunelm force-pushed the csharp_wrapper_primitive_opts branch from 9093bd9 to d22eade Compare November 5, 2019 11:39
@chrisdunelm
Copy link
Contributor Author

chrisdunelm commented Nov 5, 2019 •
edited
Loading

The "TimerTest" benchmark shows the following improvement:
Benchmark on a Linux machine.

Before:

|            Method |    Mean |    Error |   StdDev |
|------------------ |--------:|---------:|---------:|
|      CsvBenchmark | 2.137 s | 0.0472 s | 0.0484 s |
| ProtobufBenchmark | 2.352 s | 0.0450 s | 0.0421 s |

After:

|            Method |    Mean |    Error |   StdDev |
|------------------ |--------:|---------:|---------:|
|      CsvBenchmark | 2.108 s | 0.0192 s | 0.0161 s |
| ProtobufBenchmark | 1.898 s | 0.0313 s | 0.0292 s |

jtattermusch
jtattermusch reviewed Nov 5, 2019
View reviewed changes
csharp/src/Google.Protobuf/CodedInputStream.cs
}
else
{
var bytes = new byte[8];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not that important because we don't really have users on big endian I think, but can we avoid the allocation?
Unfortunately stackalloc is not available because as currently the LangVersion is set to 6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed stackallow would be much better here.

Not that I'm suggesting changing this now, but why are you still on C# 6?

Copy link
Contributor

Choose a reason for hiding this comment

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

(complicated story, but basically we had an internal user that could only use C# 6 due to using Unity. I believe that's no longer a problem).

Copy link

Choose a reason for hiding this comment

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

If this dependency is no longer true, then can we start using stackalloc and Span and such niceties?

jtattermusch
jtattermusch approved these changes Nov 5, 2019
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 one minor nit.

@jtattermusch jtattermusch added the c# label Nov 5, 2019
@jtattermusch
Copy link
Contributor

Merging (and will backport to v3.10.x)

@jtattermusch jtattermusch merged commit 0150f7f into protocolbuffers:master Nov 5, 2019
@jtattermusch jtattermusch mentioned this pull request Nov 5, 2019
Merged
@chrisdunelm chrisdunelm deleted the csharp_wrapper_primitive_opts branch November 5, 2019 17:14
@jtattermusch jtattermusch mentioned this pull request Nov 6, 2019
Closed
@jtattermusch jtattermusch mentioned this pull request Nov 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnashOommen AnashOommen AnashOommen left review comments

@jtattermusch jtattermusch jtattermusch approved these changes

@gerben-s gerben-s gerben-s approved these changes

Assignees

@gerben-s gerben-s

@jtattermusch jtattermusch

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

Successfully merging this pull request may close these issues.
6 participants
@chrisdunelm @jtattermusch @AnashOommen @gerben-s @googlebot @protobuf-kokoro