Skip to content

Commit

Permalink
Several bug fixes for serialization or nested/repeated messages (#387)
Browse files Browse the repository at this point in the history 
* Fix serialization issues with repeated messages.

* - Enable toggle for EfficientMessageCopy again in C++ code.
- 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.

* Undo accidental submission of file.

* Replacing tabs with spaces.

* Update CalculateTagWireSize so that it handles tag sizes larger than 2 bytes.

* Using constant instead of local variable for dimension size.
  • Loading branch information
@jasonmreding
jasonmreding authored Oct 16, 2024
1 parent e9b48fa commit 76f6a0c
Show file tree
Hide file tree
Showing 10 changed files with 305 additions and 254 deletions.
35 changes: 21 additions & 14 deletions src/cluster_copier.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ namespace grpc_labview {
case LVMessageMetadataType::SInt32Value:
CopySInt32ToCluster(fieldMetadata, start, value);
break;
case LVMessageMetadataType:: SInt64Value:
case LVMessageMetadataType::SInt64Value:
CopySInt64ToCluster(fieldMetadata, start, value);
break;
case LVMessageMetadataType::Fixed32Value:
CopyFixed32ToCluster(fieldMetadata, start, value);
break;
case LVMessageMetadataType::Fixed64Value:
CopyFixed64ToCluster(fieldMetadata, start, value);
break;
CopyFixed64ToCluster(fieldMetadata, start, value);
break;
case LVMessageMetadataType::SFixed32Value:
CopySFixed32ToCluster(fieldMetadata, start, value);
break;
Expand All @@ -95,13 +95,13 @@ 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!
// 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)
{
auto fieldMetadata = val.second;
if (fieldMetadata->isInOneof&& fieldMetadata->protobufIndex < 0)
{
auto fieldMetadata = val.second;
if (fieldMetadata->isInOneof && fieldMetadata->protobufIndex < 0)
{
// This field is the selected_index field of a oneof
if (oneof_containerToSelectedIndexMap.find(fieldMetadata->oneofContainerName) != oneof_containerToSelectedIndexMap.end())
{
Expand All @@ -119,7 +119,7 @@ namespace grpc_labview {
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)
{
{
auto fieldMetadata = val.second;
if (fieldMetadata->isInOneof)
{
Expand All @@ -140,7 +140,7 @@ namespace grpc_labview {
if (fieldMetadata->protobufIndex >= 0)
{
auto it = oneof_containerToSelectedIndexMap.find(fieldMetadata->oneofContainerName);
assert (it != oneof_containerToSelectedIndexMap.end());
assert(it != oneof_containerToSelectedIndexMap.end());
auto selected_index = it->second;
if (selected_index != fieldMetadata->protobufIndex)
{
Expand Down Expand Up @@ -287,7 +287,7 @@ namespace grpc_labview {
auto repeatedString = static_cast<const LVRepeatedMessageValue<std::string>&>(*value);
if (repeatedString._value.size() != 0)
{
NumericArrayResize(0x08, 1, start, repeatedString._value.size());
NumericArrayResize(GetTypeCodeForSize(sizeof(char*)), 1, start, repeatedString._value.size());
auto array = *(LV1DArrayHandle*)start;
(*array)->cnt = repeatedString._value.size();
int x = 0;
Expand Down Expand Up @@ -330,14 +330,21 @@ namespace grpc_labview {
{
auto nestedMetadata = repeatedNested->_value.front()->_metadata;
auto clusterSize = nestedMetadata->clusterSize;
auto byteSize = repeatedNested->_value.size() * clusterSize;
auto alignment = nestedMetadata->alignmentRequirement;
auto alignedElementSize = byteSize / alignment;
if (byteSize % alignment != 0)
{
alignedElementSize++;
}

NumericArrayResize(0x08, 1, start, repeatedNested->_value.size() * clusterSize);
NumericArrayResize(GetTypeCodeForSize(alignment), 1, start, alignedElementSize);
auto array = *(LV1DArrayHandle*)start;
(*array)->cnt = repeatedNested->_value.size();
int x = 0;
for (auto str : repeatedNested->_value)
{
auto lvCluster = (LVCluster**)(*array)->bytes(x * clusterSize, nestedMetadata->alignmentRequirement);
auto lvCluster = (LVCluster**)(*array)->bytes(x * clusterSize, alignment);
*lvCluster = nullptr;
CopyToCluster(*str, (int8_t*)lvCluster);
x += 1;
Expand Down Expand Up @@ -419,7 +426,7 @@ namespace grpc_labview {
auto byteCount = count * sizeof(int32_t);
memcpy((*array)->bytes<int32_t>(), mappedArray, byteCount);
}

free(mappedArray);
}
else
Expand Down Expand Up @@ -810,7 +817,7 @@ namespace grpc_labview {
repeatedValue->_value.Reserve(count);
auto dest = repeatedValue->_value.AddNAlreadyReserved(count);
memcpy(dest, mappedArray, count * sizeof(int32_t));

free(mappedArray);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/feature_toggles.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace grpc_labview {
// Constructor to initialize with default values
FeatureConfig() {
featureFlags["gRPC"] = true; // Enable gRPC by default as an example, this will never be overridden by config file
featureFlags["data_EfficientMessageCopy"] = false;
featureFlags["data_EfficientMessageCopy"] = true;
featureFlags["data_useOccurrence"] = true;
}

Expand Down
78 changes: 47 additions & 31 deletions src/lv_interop.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ static int kCleanOnIdle = 2;
//---------------------------------------------------------------------
//---------------------------------------------------------------------
typedef int (*NumericArrayResize_T)(int32_t, int32_t, void* handle, size_t size);
typedef int (*PostLVUserEvent_T)(grpc_labview::LVUserEventRef ref, void *data);
typedef int (*PostLVUserEvent_T)(grpc_labview::LVUserEventRef ref, void* data);
typedef int (*Occur_T)(grpc_labview::MagicCookie occurrence);
typedef int32_t(*RTSetCleanupProc_T)(grpc_labview::CleanupProcPtr cleanUpProc, grpc_labview::gRPCid* id, int32_t mode);
typedef unsigned char** (*DSNewHandlePtr_T)(size_t);
Expand All @@ -44,14 +44,14 @@ namespace grpc_labview
{
grpc_labview::PointerManager<grpc_labview::gRPCid> gPointerManager;

//---------------------------------------------------------------------
// Allows for definition of the LVRT DLL path to be used for callback functions
// This function should be called prior to calling InitCallbacks()
//---------------------------------------------------------------------
void SetLVRTModulePath(std::string modulePath)
{
ModulePath = modulePath;
}
// Allows for definition of the LVRT DLL path to be used for callback functions
// This function should be called prior to calling InitCallbacks()
//---------------------------------------------------------------------
void SetLVRTModulePath(std::string modulePath)
{
ModulePath = modulePath;
}

#ifdef _WIN32

Expand All @@ -67,24 +67,24 @@ namespace grpc_labview
return;
}

HMODULE lvModule;

if(ModulePath != "")
{
lvModule = GetModuleHandle(ModulePath.c_str());
}
else
{
lvModule = GetModuleHandle("LabVIEW.exe");
if (lvModule == nullptr)
{
lvModule = GetModuleHandle("lvffrt.dll");
}
if (lvModule == nullptr)
{
lvModule = GetModuleHandle("lvrt.dll");
}
}
HMODULE lvModule;

if (ModulePath != "")
{
lvModule = GetModuleHandle(ModulePath.c_str());
}
else
{
lvModule = GetModuleHandle("LabVIEW.exe");
if (lvModule == nullptr)
{
lvModule = GetModuleHandle("lvffrt.dll");
}
if (lvModule == nullptr)
{
lvModule = GetModuleHandle("lvrt.dll");
}
}
NumericArrayResizeImp = (NumericArrayResize_T)GetProcAddress(lvModule, "NumericArrayResize");
PostLVUserEvent = (PostLVUserEvent_T)GetProcAddress(lvModule, "PostLVUserEvent");
Occur = (Occur_T)GetProcAddress(lvModule, "Occur");
Expand Down Expand Up @@ -131,15 +131,15 @@ namespace grpc_labview
//---------------------------------------------------------------------
//---------------------------------------------------------------------
int NumericArrayResize(int32_t typeCode, int32_t numDims, void* handle, size_t size)
{
{
return NumericArrayResizeImp(typeCode, numDims, handle, size);
}

//---------------------------------------------------------------------
//---------------------------------------------------------------------
int PostUserEvent(LVUserEventRef ref, void *data)
int PostUserEvent(LVUserEventRef ref, void* data)
{
return PostLVUserEvent(ref, data);
return PostLVUserEvent(ref, data);
}

//---------------------------------------------------------------------
Expand Down Expand Up @@ -167,7 +167,7 @@ namespace grpc_labview
//---------------------------------------------------------------------
void SetLVString(LStrHandle* lvString, std::string str)
{
auto length = str.length();
auto length = str.length();
auto error = NumericArrayResize(0x01, 1, lvString, length);
memcpy((**lvString)->str, str.c_str(), length);
(**lvString)->cnt = (int)length;
Expand All @@ -176,7 +176,7 @@ namespace grpc_labview
//---------------------------------------------------------------------
//---------------------------------------------------------------------
std::string GetLVString(LStrHandle lvString)
{
{
if (lvString == nullptr || *lvString == nullptr)
{
return std::string();
Expand Down Expand Up @@ -216,4 +216,20 @@ namespace grpc_labview
return clusterOffset;
#endif
}

int32_t GetTypeCodeForSize(int byteSize)
{
switch (byteSize)
{
case 1:
return 0x5; // uB
case 2:
return 0x6; // uW
case 4:
return 0x7; // uL
case 8:
default:
return 0x8; // uQ
}
}
}
13 changes: 8 additions & 5 deletions src/lv_interop.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#define LIBRARY_EXPORT extern "C" __attribute__((visibility("default")))
#endif

namespace grpc_labview
namespace grpc_labview
{
class gRPCid;
extern PointerManager<gRPCid> gPointerManager;
Expand All @@ -50,6 +50,9 @@ namespace grpc_labview

int AlignClusterOffset(int clusterOffset, int alignmentRequirement);

// Provides type code for use with NumericArrayResize function for various sizes of data types.
int32_t GetTypeCodeForSize(int byteSize);

//---------------------------------------------------------------------
// LabVIEW definitions
//---------------------------------------------------------------------
Expand All @@ -64,7 +67,7 @@ namespace grpc_labview
};

using LStrPtr = LStr*;
using LStrHandle = LStr**;
using LStrHandle = LStr**;

struct LV1DArray {
int32_t cnt; /* number of bytes that follow */
Expand Down Expand Up @@ -99,7 +102,7 @@ namespace grpc_labview
#pragma pack (push, 1)
#endif
struct AnyCluster
{
{
LStrHandle TypeUrl;
LV1DArrayHandle Bytes;
};
Expand All @@ -110,11 +113,11 @@ namespace grpc_labview
//---------------------------------------------------------------------
//---------------------------------------------------------------------
void SetLVRTModulePath(std::string modulePath);
void InitCallbacks();
void InitCallbacks();
void SetLVString(LStrHandle* lvString, std::string str);
std::string GetLVString(LStrHandle lvString);
int NumericArrayResize(int32_t typeCode, int32_t numDims, void* handle, size_t size);
int PostUserEvent(LVUserEventRef ref, void *data);
int PostUserEvent(LVUserEventRef ref, void* data);
unsigned char** DSNewHandle(size_t n);
int DSSetHandleSize(void* h, size_t n);
long DSDisposeHandle(void* h);
Expand Down
45 changes: 30 additions & 15 deletions src/lv_message.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,12 @@ namespace grpc_labview
{
v = std::static_pointer_cast<LVRepeatedMessageValue<std::string>>((*it).second);
}
protobuf_ptr -= 1;

auto tagSize = CalculateTagWireSize(tag);
protobuf_ptr -= tagSize;
do
{
protobuf_ptr += 1;
protobuf_ptr += tagSize;
auto str = v->_value.Add();
protobuf_ptr = InlineGreedyStringParser(str, protobuf_ptr, ctx);
if (!ctx->DataAvailable(protobuf_ptr))
Expand Down Expand Up @@ -400,6 +402,17 @@ namespace grpc_labview
}
}

//---------------------------------------------------------------------
//---------------------------------------------------------------------
int LVMessage::CalculateTagWireSize(google::protobuf::uint32 tag)
{
return (tag < (1 << 7)) ? 1
: (tag < (1 << 14)) ? 2
: (tag < (1 << 21)) ? 3
: (tag < (1 << 28)) ? 4
: 5;
}

//---------------------------------------------------------------------
//---------------------------------------------------------------------
const char *LVMessage::ParseSInt32(const MessageElementMetadata &fieldInfo, uint32_t index, const char *ptr, ParseContext *ctx)
Expand Down Expand Up @@ -527,21 +540,23 @@ namespace grpc_labview
auto metadata = fieldInfo._owner->FindMetadata(fieldInfo.embeddedMessageName);
if (fieldInfo.isRepeated)
{
protobuf_ptr -= 1;
std::shared_ptr<LVRepeatedNestedMessageMessageValue> v;
auto it = _values.find(index);
if (it == _values.end())
{
v = std::make_shared<LVRepeatedNestedMessageMessageValue>(index);
_values.emplace(index, v);
}
else
{
v = std::static_pointer_cast<LVRepeatedNestedMessageMessageValue>((*it).second);
}

auto tagSize = CalculateTagWireSize(tag);
protobuf_ptr -= tagSize;
do
{
std::shared_ptr<LVRepeatedNestedMessageMessageValue> v;
auto it = _values.find(index);
if (it == _values.end())
{
v = std::make_shared<LVRepeatedNestedMessageMessageValue>(index);
_values.emplace(index, v);
}
else
{
v = std::static_pointer_cast<LVRepeatedNestedMessageMessageValue>((*it).second);
}
protobuf_ptr += 1;
protobuf_ptr += tagSize;
auto nestedMessage = std::make_shared<LVMessage>(metadata);
protobuf_ptr = ctx->ParseMessage(nestedMessage.get(), protobuf_ptr);
v->_value.push_back(nestedMessage);
Expand Down
5 changes: 3 additions & 2 deletions src/lv_message.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

using namespace google::protobuf::internal;

namespace grpc_labview
namespace grpc_labview
{
//---------------------------------------------------------------------
//---------------------------------------------------------------------
Expand All @@ -37,7 +37,7 @@ namespace grpc_labview
int GetCachedSize(void) const final;
size_t ByteSizeLong() const final;
virtual void PostInteralParseAction() {};

void MergeFrom(const google::protobuf::Message &from) final;
void MergeFrom(const LVMessage &from);
void CopyFrom(const google::protobuf::Message &from) final;
Expand Down Expand Up @@ -82,5 +82,6 @@ namespace grpc_labview
virtual const char *ParseBytes(unsigned int tag, const MessageElementMetadata& fieldInfo, uint32_t index, const char *ptr, google::protobuf::internal::ParseContext *ctx);
virtual const char *ParseNestedMessage(google::protobuf::uint32 tag, const MessageElementMetadata& fieldInfo, uint32_t index, const char *ptr, google::protobuf::internal::ParseContext *ctx);
bool ExpectTag(google::protobuf::uint32 tag, const char* ptr);
int CalculateTagWireSize(google::protobuf::uint32 tag);
};
}
Loading

0 comments on commit 76f6a0c

Please sign in to comment.