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

Several bug fixes for serialization or nested/repeated messages #387

Merged
merged 7 commits into from
Oct 16, 2024

Conversation

jasonmreding
Copy link
Collaborator

@jasonmreding jasonmreding commented Sep 30, 2024

Description

This PR fixes several issues with message serialization when EfficientMessageCopy = TRUE.
#378
#384
#392

Implementation

  • Fixed several places where the array being allocated was larger than required. In effect, we were requesting an allocation for the total byte size when NumericArrayResize takes the element size and not the byte size.
  • Fixed a few places where arrays were always allocating 8 byte pointers rather than 4 bytes for a pointer on 32 bit platforms.
  • Cleaned up some state management for better consistency with how we handle repeated string and message fields.
  • Fixed issues where the buffer used to copy data for repeated nested messages was always copying previous elements in the array and copying the address for the same buffer multiple times in the message structure. This leads to DWARNs in the memory manager since we attempt to deallocate the same block of memory multiple times. I essentially fixed this by allocating a new message on the stack for each element of the repeated field rather than reusing the same message object for all elements of the repeated field. I ultimately found this to be easier to read and rationalize about even though it probably requires more memory allocations. The alternative would be to clear the maps / repeated fields we use to hold the data as we parse the data. This would allow us to reuse those objects at their existing capacity which more than likely enough to hold the next element as well. However, the readability of the code is worse. If anyone feels like that performance gain is worth it, I can look into making that change.
  • Fixed issues where the tag size calculation for repeated string/message fields was incorrect and would only work if the tag could be encoded in a single byte.

Testing

@dixonjoel
Copy link
Collaborator

dixonjoel commented Oct 3, 2024

I noticed these warnings in the PR build. Probably pre-dates your changes, but I wonder if they're indicating problems or can be ignored.

D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb/table_internal.h(200,28): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb/msg_internal.h(469,1): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb/msg_internal.h(741,34): warning C4090: 'function': different 'const' qualifiers [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb\decode.c(190,61): warning C4646: function declared with 'noreturn' has non-void return type [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb\decode.c(412,31): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
  gzclose.c
  absl_random_internal_platform.vcxproj -> D:\a\grpc-labview\grpc-labview\build\grpc\third_party\abseil-cpp\absl\random\Release\absl_random_internal_platform.lib
  def.c

Looking at them more closely, probably harmless since it's 32-bit converted to 64-bit implicitly.

@bkeryan
Copy link
Collaborator

bkeryan commented Oct 3, 2024

I noticed these warnings in the PR build. Probably pre-dates your changes, but I wonder if they're indicating problems or can be ignored.

D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb/table_internal.h(200,28): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb/msg_internal.h(469,1): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb/msg_internal.h(741,34): warning C4090: 'function': different 'const' qualifiers [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb\decode.c(190,61): warning C4646: function declared with 'noreturn' has non-void return type [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
D:\a\grpc-labview\grpc-labview\third_party\grpc\third_party\upb\upb\decode.c(412,31): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [D:\a\grpc-labview\grpc-labview\build\grpc\upb.vcxproj]
  gzclose.c
  absl_random_internal_platform.vcxproj -> D:\a\grpc-labview\grpc-labview\build\grpc\third_party\abseil-cpp\absl\random\Release\absl_random_internal_platform.lib
  def.c

Looking at them more closely, probably harmless since it's 32-bit converted to 64-bit implicitly.

These are all coming from grpc source, which is 2 years out of date. They may have been fixed upstream since then.

It looks like @ni-sujain tried updating to grpc 1.62 in 04435f7 then reverted it in 0f954d4 .

@dixonjoel dixonjoel requested a review from bkeryan October 4, 2024 15:18
@dixonjoel
Copy link
Collaborator

@bkeryan @jasonmreding I don't think we can afford to wait much longer for a review. Can Brad just complete his review and then we can just move forward?

* master:
  VIPB version bump
  Bump vipb version
  Disabling EfficientMessageCopy feature until bugs can be fixed and more testing is performed. (#391)
  Add boolean array support to GetUnpackedField.
- Fixing serialization issues related to repeated string and message fields where the tag/field number requires more than one byte
- Fix repeated string/message serialization where data on the wire isn't sent in a contiguous block.
src/cluster_copier.cc Show resolved Hide resolved
src/lv_interop.cc Outdated Show resolved Hide resolved
src/lv_message.cc Outdated Show resolved Hide resolved
src/lv_message.cc Outdated Show resolved Hide resolved
src/lv_message_efficient.cc Outdated Show resolved Hide resolved
@jasonmreding jasonmreding requested a review from bkeryan October 15, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants