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

Fixing client serialization bugs #402

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Fixing client serialization bugs #402

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c010f49
Fixing client serialization issues around sparse enums and oneof fiel…
jasonmreding Jan 8, 2025
28410b0
Merge branch 'master' into users/jreding/ClientSerializationFixes
jasonmreding Jan 9, 2025
4c3859d
Implementing PR feedback.
jasonmreding Jan 14, 2025
9217e97
Attempting to fix Linux build by reverting runner image back to ubunt…
jasonmreding Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 36 additions & 44 deletions src/cluster_copier.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,14 @@ namespace grpc_labview {
// cluster: Pointer to the cluster created by LabVIEW
void ClusterDataCopier::CopyToCluster(const LVMessage& message, int8_t* cluster)
{
std::map<std::string, int> oneof_containerToSelectedIndexMap;
for (auto val : message._metadata->_mappedElements)
for (auto indexAndValue : message._values)
{
auto fieldMetadata = val.second;
auto start = cluster + fieldMetadata->clusterOffset;
std::shared_ptr<LVMessageValue> value;
for (auto v : message._values)
auto& value = indexAndValue.second;
auto it = message._metadata->_mappedElements.find(value->_protobufId);
if (it != message._metadata->_mappedElements.end())
{
if (v.second->_protobufId == fieldMetadata->protobufIndex)
{
value = v.second;
break;
}
}
if (value != nullptr)
{
if (fieldMetadata->isInOneof)
{
// set the map of the selected index for the "oneofContainer" to this protobuf Index
assert(oneof_containerToSelectedIndexMap.find(fieldMetadata->oneofContainerName) == oneof_containerToSelectedIndexMap.end());
oneof_containerToSelectedIndexMap.insert(std::pair<std::string, int>(fieldMetadata->oneofContainerName, fieldMetadata->protobufIndex));
}
auto& fieldMetadata = it->second;
auto start = cluster + fieldMetadata->clusterOffset;;
switch (fieldMetadata->type)
{
case LVMessageMetadataType::StringValue:
Expand Down Expand Up @@ -95,18 +81,22 @@ namespace grpc_labview {
}
}

// second pass to fill the oneof selected_index. We can do this in one pass when we push the selected_field to the end of the oneof cluster!
// TODO: Skip the entire loop if the message has no oneof. It's a bool in the metadata.
for (auto val : message._metadata->_mappedElements)
// Second pass to fill the oneof selected_index. We can do this in one pass when we push the selected_field to the end of the oneof cluster!
if (message._oneofContainerToSelectedIndexMap.size() > 0)
{
auto fieldMetadata = val.second;
if (fieldMetadata->isInOneof && fieldMetadata->protobufIndex < 0)
// Must iterate over _elements and not _mappedElements since all oneof selected_index fields use -1 for the field number
// and there can be multiple oneof fields in a message.
for (auto& fieldMetadata : message._metadata->_elements)
{
// This field is the selected_index field of a oneof
if (oneof_containerToSelectedIndexMap.find(fieldMetadata->oneofContainerName) != oneof_containerToSelectedIndexMap.end())
if (fieldMetadata->isInOneof && fieldMetadata->protobufIndex < 0)
{
auto start = cluster + fieldMetadata->clusterOffset;
*(int*)start = oneof_containerToSelectedIndexMap[fieldMetadata->oneofContainerName];
// This field is the selected_index field of a oneof
auto it = message._oneofContainerToSelectedIndexMap.find(fieldMetadata->oneofContainerName);
if (it != message._oneofContainerToSelectedIndexMap.end())
{
auto selectedIndexPtr = reinterpret_cast<int*>(cluster + fieldMetadata->clusterOffset);
*selectedIndexPtr = it->second;
}
}
}
}
Expand All @@ -116,31 +106,33 @@ namespace grpc_labview {
//---------------------------------------------------------------------
void ClusterDataCopier::CopyFromCluster(LVMessage& message, int8_t* cluster)
{
message._values.clear();
std::map<std::string, int> oneof_containerToSelectedIndexMap; // Needed to serialize only the field related to the selected_index
for (auto val : message._metadata->_mappedElements)
message.Clear();
for (auto& fieldMetadata : message._metadata->_elements)
{
auto fieldMetadata = val.second;
if (fieldMetadata->isInOneof)
if (fieldMetadata->isInOneof && fieldMetadata->protobufIndex < 0)
{
if (fieldMetadata->protobufIndex < 0)
{
// set the map of the selected index for the "oneofContainer" to this protobuf Index
assert(oneof_containerToSelectedIndexMap.find(fieldMetadata->oneofContainerName) == oneof_containerToSelectedIndexMap.end());
auto selected_index = *(int*)(cluster + fieldMetadata->clusterOffset);
oneof_containerToSelectedIndexMap.insert(std::pair<std::string, int>(fieldMetadata->oneofContainerName, selected_index));
}
// set the map of the selected index for the "oneofContainer" to this protobuf Index
assert(message._oneofContainerToSelectedIndexMap.find(fieldMetadata->oneofContainerName) == message._oneofContainerToSelectedIndexMap.end());
auto selected_index = *(int*)(cluster + fieldMetadata->clusterOffset);
message._oneofContainerToSelectedIndexMap.insert(std::pair<std::string, int>(fieldMetadata->oneofContainerName, selected_index));
}
}

for (auto val : message._metadata->_mappedElements)
{
auto fieldMetadata = val.second;
if (fieldMetadata->isInOneof)
{
if (fieldMetadata->protobufIndex >= 0)
if (fieldMetadata->protobufIndex < 0)
{
// Do not serialize the selected_index field of a oneof. This is used for internal book keeping
// and is not really a part of the message data that should go across the wire.
continue;
}
else
{
auto it = oneof_containerToSelectedIndexMap.find(fieldMetadata->oneofContainerName);
assert(it != oneof_containerToSelectedIndexMap.end());
auto it = message._oneofContainerToSelectedIndexMap.find(fieldMetadata->oneofContainerName);
assert(it != message._oneofContainerToSelectedIndexMap.end());
auto selected_index = it->second;
if (selected_index != fieldMetadata->protobufIndex)
{
Expand Down
6 changes: 2 additions & 4 deletions src/grpc_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,8 @@ LIBRARY_EXPORT int32_t ClientUnaryCall2(

if (clientCall->_useLVEfficientMessage)
{
clientCall->_request = std::make_shared<grpc_labview::LVMessageEfficient>(requestMetadata);
clientCall->_response = std::make_shared<grpc_labview::LVMessageEfficient>(responseMetadata);
clientCall->_request->SetLVClusterHandle(reinterpret_cast<const char*>(requestCluster));
clientCall->_response->SetLVClusterHandle(reinterpret_cast<const char*>(responseCluster));
clientCall->_request = std::make_shared<grpc_labview::LVMessageEfficient>(requestMetadata, requestCluster);
clientCall->_response = std::make_shared<grpc_labview::LVMessageEfficient>(responseMetadata, responseCluster);
}
else
{
Expand Down
11 changes: 10 additions & 1 deletion src/lv_message.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ namespace grpc_labview
void LVMessage::Clear()
{
_values.clear();
_oneofContainerToSelectedIndexMap.clear();
}

//---------------------------------------------------------------------
Expand All @@ -105,8 +106,16 @@ namespace grpc_labview
auto fieldIt = _metadata->_mappedElements.find(index);
if (fieldIt != _metadata->_mappedElements.end())
{
auto fieldInfo = (*fieldIt).second;
auto& fieldInfo = (*fieldIt).second;
LVMessageMetadataType dataType = fieldInfo->type;

if (fieldInfo->isInOneof)
{
// set the map of the selected index for the "oneofContainer" to this protobuf Index
assert(_oneofContainerToSelectedIndexMap.find(fieldInfo->oneofContainerName) == _oneofContainerToSelectedIndexMap.end());
_oneofContainerToSelectedIndexMap.insert(std::pair<std::string, int>(fieldInfo->oneofContainerName, fieldInfo->protobufIndex));
}

switch (dataType)
{
case LVMessageMetadataType::Int32Value:
Expand Down
10 changes: 1 addition & 9 deletions src/lv_message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,13 @@ namespace grpc_labview
bool ParseFromByteBuffer(const grpc::ByteBuffer& buffer);
std::unique_ptr<grpc::ByteBuffer> SerializeToByteBuffer();

void SetLVClusterHandle(const char* lvClusterHandle) {
_LVClusterHandle = lvClusterHandle;
};

const char* GetLVClusterHandleSharedPtr() {
return _LVClusterHandle;
};

std::map<int, std::shared_ptr<LVMessageValue>> _values;
std::shared_ptr<MessageMetadata> _metadata;
std::map<std::string, int> _oneofContainerToSelectedIndexMap;

protected:
mutable google::protobuf::internal::CachedSize _cached_size_;
google::protobuf::UnknownFieldSet _unknownFields;
const char* _LVClusterHandle;

virtual const char *ParseBoolean(const MessageElementMetadata& fieldInfo, uint32_t index, const char *ptr, google::protobuf::internal::ParseContext *ctx);
virtual const char *ParseInt32(const MessageElementMetadata& fieldInfo, uint32_t index, const char *ptr, google::protobuf::internal::ParseContext *ctx);
Expand Down
89 changes: 72 additions & 17 deletions src/lv_message_efficient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ namespace grpc_labview
DEFINE_PARSE_FUNCTION(uint64_t, UInt64, UINT64, UInt64)
DEFINE_PARSE_FUNCTION(float, Float, FLOAT, Float)
DEFINE_PARSE_FUNCTION(double, Double, DOUBLE, Double)
DEFINE_PARSE_FUNCTION_SPECIAL(int32_t, Enum, ENUM, Enum)
DEFINE_PARSE_FUNCTION_SPECIAL(int32_t, SInt32, SINT32, SInt32)
DEFINE_PARSE_FUNCTION_SPECIAL(int64_t, SInt64, SINT64, SInt64)
DEFINE_PARSE_FUNCTION_SPECIAL(uint32_t, Fixed32, FIXED32, Fixed32)
Expand All @@ -67,10 +66,46 @@ namespace grpc_labview

//---------------------------------------------------------------------
//---------------------------------------------------------------------
const char* LVMessageEfficient::ParseString(google::protobuf::uint32 tag, const MessageElementMetadata& fieldInfo, uint32_t index, const char* protobuf_ptr, ParseContext* ctx)
const char* LVMessageEfficient::ParseEnum(const MessageElementMetadata& fieldInfo, uint32_t index, const char* ptr, ParseContext* ctx)
{
const char* lv_ptr = (this->GetLVClusterHandleSharedPtr()) + fieldInfo.clusterOffset;
std::shared_ptr<EnumMetadata> enumMetadata = fieldInfo._owner->FindEnumMetadata(fieldInfo.embeddedMessageName);
auto lv_ptr = _LVClusterHandle + fieldInfo.clusterOffset;

if (fieldInfo.isRepeated)
{
auto repeatedEnum = std::make_shared<LVRepeatedEnumMessageValue>(index);
ptr = PackedEnumParser(&(repeatedEnum->_value), ptr, ctx);

int count = repeatedEnum->_value.size();
for (size_t i = 0; i < count; i++)
{
auto enumValueFromPrtobuf = repeatedEnum->_value[i];
repeatedEnum->_value[i] = enumMetadata->GetLVEnumValueFromProtoValue(enumValueFromPrtobuf);
}

if (count != 0)
{
auto messageTypeSize = sizeof(int32_t);
NumericArrayResize(GetTypeCodeForSize(messageTypeSize), 1, reinterpret_cast<void*>(lv_ptr), count);
auto array = *(LV1DArrayHandle*)lv_ptr;
(*array)->cnt = count;
auto byteCount = count * sizeof(int32_t);
memcpy((*array)->bytes<int32_t>(), repeatedEnum->_value.data(), byteCount);
}
}
else
{
int32_t enumValueFromPrtobuf;
ptr = ReadENUM(ptr, &enumValueFromPrtobuf);
*(int32_t*)lv_ptr = enumMetadata->GetLVEnumValueFromProtoValue(enumValueFromPrtobuf);
}
return ptr;
}

//---------------------------------------------------------------------
//---------------------------------------------------------------------
const char* LVMessageEfficient::ParseString(google::protobuf::uint32 tag, const MessageElementMetadata& fieldInfo, uint32_t index, const char* protobuf_ptr, ParseContext* ctx)
{
if (fieldInfo.isRepeated)
{
auto repeatedStringValuesIt = _repeatedStringValuesMap.find(fieldInfo.fieldName);
Expand All @@ -97,6 +132,7 @@ namespace grpc_labview
{
auto str = std::string();
protobuf_ptr = InlineGreedyStringParser(&str, protobuf_ptr, ctx);
auto lv_ptr = _LVClusterHandle + fieldInfo.clusterOffset;
SetLVString((LStrHandle*)lv_ptr, str);
}
return protobuf_ptr;
Expand Down Expand Up @@ -146,7 +182,6 @@ namespace grpc_labview
do
{
protobuf_ptr += tagSize;
LVMessageEfficient nestedMessage(metadata);

// Resize the vector if we need more memory
if (elementIndex >= numElements - 1)
Expand All @@ -156,9 +191,9 @@ namespace grpc_labview
repeatedMessageValuesIt->second.get()->_buffer.Resize(arraySize, _fillData);
}

auto vectorPtr = repeatedMessageValuesIt->second.get()->_buffer.data();
vectorPtr = vectorPtr + (elementIndex * clusterSize);
nestedMessage.SetLVClusterHandle(vectorPtr);
auto nestedMessageCluster = reinterpret_cast<int8_t*>(const_cast<char*>(repeatedMessageValuesIt->second.get()->_buffer.data()));
nestedMessageCluster = nestedMessageCluster + (elementIndex * clusterSize);
LVMessageEfficient nestedMessage(metadata, nestedMessageCluster);
protobuf_ptr = ctx->ParseMessage(&nestedMessage, protobuf_ptr);

elementIndex++;
Expand All @@ -173,24 +208,44 @@ namespace grpc_labview
}
else
{
LVMessageEfficient nestedMessage(metadata);
const char* lv_ptr = (this->GetLVClusterHandleSharedPtr()) + fieldInfo.clusterOffset;
nestedMessage.SetLVClusterHandle(lv_ptr);
auto nestedClusterPtr = _LVClusterHandle + fieldInfo.clusterOffset;
LVMessageEfficient nestedMessage(metadata, nestedClusterPtr);
protobuf_ptr = ctx->ParseMessage(&nestedMessage, protobuf_ptr);
}
return protobuf_ptr;
}

//---------------------------------------------------------------------
//---------------------------------------------------------------------
void LVMessageEfficient::PostInteralParseAction()
{
if (_oneofContainerToSelectedIndexMap.size() > 0)
{
// Must iterate over _elements and not _mappedElements since all oneof selected_index fields use -1 for the field number
// and there can be multiple oneof fields in a message.
for (auto& fieldMetadata : _metadata->_elements)
{
if (fieldMetadata->isInOneof && fieldMetadata->protobufIndex < 0)
{
// This field is the selected_index field of a oneof. This field only exists in the cluster
// and is not a field in the message.
if (_oneofContainerToSelectedIndexMap.find(fieldMetadata->oneofContainerName) != _oneofContainerToSelectedIndexMap.end())
{
auto selectedIndexPtr = _LVClusterHandle + fieldMetadata->clusterOffset;
*(int*)selectedIndexPtr = _oneofContainerToSelectedIndexMap[fieldMetadata->oneofContainerName];
}
}
}
}

for (auto nestedMessage : _repeatedMessageValuesMap)
{
auto& fieldInfo = nestedMessage.second.get()->_fieldInfo;
auto& buffer = nestedMessage.second.get()->_buffer;
auto numClusters = nestedMessage.second.get()->_numElements;

auto metadata = fieldInfo._owner->FindMetadata(fieldInfo.embeddedMessageName);
const char* lv_ptr = (this->GetLVClusterHandleSharedPtr()) + fieldInfo.clusterOffset;
auto lv_ptr = _LVClusterHandle + fieldInfo.clusterOffset;
auto clusterSize = metadata->clusterSize;
auto alignment = metadata->alignmentRequirement;

Expand All @@ -202,22 +257,22 @@ namespace grpc_labview
{
alignedElementSize++;
}
NumericArrayResize(GetTypeCodeForSize(alignment), 1, reinterpret_cast<void*>(const_cast<char*>(lv_ptr)), alignedElementSize);
NumericArrayResize(GetTypeCodeForSize(alignment), 1, reinterpret_cast<void*>(lv_ptr), alignedElementSize);
auto arrayHandle = *(LV1DArrayHandle*)lv_ptr;
(*arrayHandle)->cnt = numClusters;

auto _vectorDataPtr = buffer.data();
auto _lvArrayDataPtr = (*arrayHandle)->bytes(0, alignment);
memcpy(_lvArrayDataPtr, _vectorDataPtr, byteSize);
auto vectorDataPtr = buffer.data();
auto lvArrayDataPtr = (*arrayHandle)->bytes(0, alignment);
memcpy(lvArrayDataPtr, vectorDataPtr, byteSize);
}

for (auto repeatedStringValue : _repeatedStringValuesMap)
{
auto& fieldInfo = repeatedStringValue.second.get()->_fieldInfo;
auto& repeatedString = repeatedStringValue.second.get()->_repeatedString;
const char* lv_ptr = (this->GetLVClusterHandleSharedPtr()) + fieldInfo.clusterOffset;
auto lv_ptr = _LVClusterHandle + fieldInfo.clusterOffset;

NumericArrayResize(GetTypeCodeForSize(sizeof(char*)), 1, reinterpret_cast<void*>(const_cast<char*>(lv_ptr)), repeatedString.size());
NumericArrayResize(GetTypeCodeForSize(sizeof(char*)), 1, reinterpret_cast<void*>(lv_ptr), repeatedString.size());
auto arrayHandle = *(LV1DArrayHandle*)lv_ptr;
(*arrayHandle)->cnt = repeatedString.size();

Expand Down
Loading
Loading