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

Improve performace of repeated packed fixedSize fields #19667

Conversation

YarinOmesi
Copy link
Contributor

@YarinOmesi YarinOmesi commented Dec 14, 2024

Changes

Improve performance of packed RepeatedField that has fixed size, by coping LEN bytes from serialzied buffer to RepeatedField array (like c memcpy).

Relevant Information

As the docs here notes, fixed size value is memcpy of the equivalent C types (u?int64_t, double)

You can see benchmark code and results here

Summarized result:

Method Job Size Mean Ratio Allocated
Parse_RepeatedFloats 3.29.0 100000 416,582.7 ns baseline 400297 B
Parse_RepeatedFloats improved-fixed-size-repeated 100000 134,671.7 ns -68% 400776 B
Write_RepeatedFloats 3.29.0 100000 340.549 us baseline 391.57 KB
Write_RepeatedFloats improved-fixed-size-repeated 100000 106.577 us -69% 391.5 KB

I am pretty new to this repo to tell me what to you think about this.

@YarinOmesi YarinOmesi requested a review from a team as a code owner December 14, 2024 19:48
@YarinOmesi YarinOmesi requested review from jskeet and removed request for a team December 14, 2024 19:48
Copy link

google-cla bot commented Dec 14, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@YarinOmesi YarinOmesi changed the title Improve performace of repeated packed fixedSize fields DRAFT: Improve performace of repeated packed fixedSize fields Dec 14, 2024
@YarinOmesi YarinOmesi marked this pull request as draft December 14, 2024 19:53
@YarinOmesi YarinOmesi force-pushed the improve-performance-repeated-packed-fixed-field branch from f30160d to 8f87b3d Compare December 14, 2024 20:33
@YarinOmesi YarinOmesi marked this pull request as ready for review December 14, 2024 20:46
@YarinOmesi YarinOmesi changed the title DRAFT: Improve performace of repeated packed fixedSize fields Improve performace of repeated packed fixedSize fields Dec 14, 2024
@zhangskz zhangskz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 16, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 16, 2024
@zhangskz zhangskz added the c# label Dec 18, 2024
@YarinOmesi YarinOmesi force-pushed the improve-performance-repeated-packed-fixed-field branch 2 times, most recently from 2823100 to e5c3514 Compare December 21, 2024 16:50
@YarinOmesi YarinOmesi marked this pull request as draft December 21, 2024 17:52
@YarinOmesi YarinOmesi marked this pull request as ready for review December 21, 2024 21:39
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 3, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 3, 2025
@JasonLunn
Copy link
Contributor

@YarinOmesi - can you rebase this PR to see if that clears up the test failures?

@YarinOmesi YarinOmesi force-pushed the improve-performance-repeated-packed-fixed-field branch from 5fd703d to 7eda30c Compare February 7, 2025 17:44
@JasonLunn JasonLunn added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed wait for user action labels Feb 7, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 7, 2025
@JasonLunn
Copy link
Contributor

Can you take a look at the failing C# conformance test?

Unhandled exception. System.InvalidOperationException: Type registry has no descriptor for type name ''
   at Google.Protobuf.JsonParser.MergeAny(IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.<>c.<.cctor>b__42_6(JsonParser parser, IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.Merge(IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.TryParseSingleValue(FieldDescriptor field, JsonTokenizer tokenizer, Object& value)
   at Google.Protobuf.JsonParser.MergeField(IMessage message, FieldDescriptor field, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.Merge(IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.Merge(IMessage message, TextReader jsonReader)
   at Google.Protobuf.JsonParser.Parse[T](TextReader jsonReader)
   at Google.Protobuf.JsonParser.Parse[T](String json)

@YarinOmesi
Copy link
Contributor Author

Can you take a look at the failing C# conformance test?

Unhandled exception. System.InvalidOperationException: Type registry has no descriptor for type name ''
   at Google.Protobuf.JsonParser.MergeAny(IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.<>c.<.cctor>b__42_6(JsonParser parser, IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.Merge(IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.TryParseSingleValue(FieldDescriptor field, JsonTokenizer tokenizer, Object& value)
   at Google.Protobuf.JsonParser.MergeField(IMessage message, FieldDescriptor field, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.Merge(IMessage message, JsonTokenizer tokenizer)
   at Google.Protobuf.JsonParser.Merge(IMessage message, TextReader jsonReader)
   at Google.Protobuf.JsonParser.Parse[T](TextReader jsonReader)
   at Google.Protobuf.JsonParser.Parse[T](String json)

There was a problem with reading bools with my approch because of .net marshaling stuff, so i added validation that comapre .net size and codec.FixedSize

@JasonLunn JasonLunn added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed wait for user action labels Feb 7, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 7, 2025
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.

3 participants