Skip to content

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

Merged
jtattermusch merged 3 commits intoprotocolbuffers:masterfrom
chrisdunelm:csharp_wrapper_primitive_opts
Nov 5, 2019
Merged

C#: Optimize parsing of some primitive and wrapper types#6843
jtattermusch merged 3 commits intoprotocolbuffers:masterfrom
chrisdunelm:csharp_wrapper_primitive_opts

Conversation

@chrisdunelm
Copy link
Copy Markdown
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
Copy Markdown
Contributor

jtattermusch commented Nov 4, 2019

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
Copy Markdown
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...

Comment thread csharp/src/Google.Protobuf/CodedInputStream.cs Outdated
@chrisdunelm
Copy link
Copy Markdown
Contributor Author

chrisdunelm commented Nov 4, 2019

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
Copy Markdown
Contributor Author

chrisdunelm commented Nov 4, 2019

@gerben-s PTAL

gerben-s
gerben-s approved these changes Nov 4, 2019
View reviewed changes
Copy link
Copy Markdown
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.

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

@gerben-s gerben-s Nov 4, 2019

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
Copy Markdown
Contributor Author

@chrisdunelm chrisdunelm Nov 5, 2019

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
Copy Markdown
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
Comment thread csharp/src/Google.Protobuf/CodedInputStream.cs
}
else
{
var bytes = new byte[8];
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Nov 5, 2019

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
Copy Markdown
Contributor Author

@chrisdunelm chrisdunelm Nov 5, 2019

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
Copy Markdown
Contributor

@jtattermusch jtattermusch Nov 5, 2019

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
Copy Markdown

@AnashOommen AnashOommen Nov 15, 2019

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
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 with one minor nit.

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

jtattermusch commented Nov 5, 2019

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

3 more reviewers

@AnashOommen AnashOommen AnashOommen left review comments

@jtattermusch jtattermusch jtattermusch approved these changes

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

Reviewers whose approvals may not affect merge requirements

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