Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…-labview into serializationTraits
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jasonmreding
left a comment
There was a problem hiding this comment.
I would like to review changes again after changes to consolidate some of the code duplication.
| auto it = _repeatedMessageValuesMap.find(fieldInfo.fieldName); | ||
| if (it == _repeatedMessageValuesMap.end()) | ||
| { | ||
| auto m_val = std::make_shared<RepeatedMessageValue>(fieldInfo, google::protobuf::RepeatedPtrField<google::protobuf::Message>()); |
There was a problem hiding this comment.
This is still using google::protobuf::Message. I think this is further proof this code path is largely untested from an automated test standpoint and is languishing. It would be nice to either get it back into a functioning state or abandon the code and the performance optimizations it can provide. Spending time to maintain and review code that is otherwise unreachable seems like a waste of time.
There was a problem hiding this comment.
After completing this upgrade, we plan to work on the lv_message_efficient class. Since it is currently disabled, it would be better to address these changes once that work is done.
| }; | ||
|
|
||
| public: | ||
| std::unordered_map<std::string, std::shared_ptr<RepeatedMessageValue>> _repeatedMessageValuesMap; |
There was a problem hiding this comment.
This is probably another bug in waiting of sorts that has been waiting here, but this class should probably override the Clear method and reset all of these fields. It should probably also reset _LVClusterHandle. However, since that is only assigned during construction, that pretty much makes the instance useless. If we ever want to reuse this instance for streaming calls in the future, we'll probably need to add another API that allows for clearing and updating of the cluster pointer. If you decide this is out of scope for this PR, I'm OK with that.
There was a problem hiding this comment.
Thanks for pointing this out, will pick this up later.
There was a problem hiding this comment.
These tests are a nice addition! Thanks for adding them. I'm still looking through the tests, but the immediate thoughts I had are:
- I didn't immediately see any oneof testing. It would be good to add a few of those.
- This test file is pretty long. Consider breaking it into multiple files for different categories of tests, perhaps by using
lv_message_tests_<category>.ccas a naming convention. This is just a suggestion so feel free to ignore if you don't see any value in it. - Consider using protoc to generate message classes for test messages you define in a proto file. Then use the generated messages to produce serialized data that is parsed from our message class and verify interoperability. Do the same thing for the other direction. At least having a few tests like this would be nice. I'm not sure if I would completely replace the existing round tripping with this approach (risks two wrongs making a right) or just augment it with this approach. I know manually writing a bunch of test messages in a proto file can be pretty tedious compared to programmatically generating message data for testing purposes.
There was a problem hiding this comment.
We’re planning to add more tests, including LabVIEW tests, sometime next month. The unit tests I added here are mainly to ensure that the recent refactoring didn’t break existing functionality, since the changes were fairly extensive.
I think we can note the additional unit test suggestions you mentioned and incorporate them in a future PR. Let me know what you think.
There was a problem hiding this comment.
I think having some interoperability tests that compare against a "trusted source" similar to how we have a Python client communicate with a LV service are a good idea and probably more of a requirement long term. I'm OK if that is deferred to a future PR as long as there is a commitment to revisiting it in the relative near term.
There was a problem hiding this comment.
I did some code cleanup for the unit tests. The remaining work like breaking down into smaller unit tests can be handled later when we begin dedicated testing.
bkeryan
left a comment
There was a problem hiding this comment.
I haven't gotten through all of this but the basic approach makes sense to me.
| int32_t mapped = enumMetadata->GetLVEnumValueFromProtoValue(static_cast<int32_t>(raw)); | ||
| auto arr = *(LV1DArrayHandle*)lv_ptr; | ||
| int32_t cnt = (arr != nullptr) ? (*arr)->cnt : 0; | ||
| NumericArrayResize(GetTypeCodeForSize(sizeof(int32_t)), 1, reinterpret_cast<void*>(lv_ptr), static_cast<size_t>(cnt) + 1); |
There was a problem hiding this comment.
Call AppendToLVArray here.
| auto cnt = vals.size(); | ||
| if (cnt > 0) | ||
| { | ||
| NumericArrayResize(GetTypeCodeForSize(sizeof(int32_t)), 1, reinterpret_cast<void*>(lv_ptr), cnt); |
There was a problem hiding this comment.
Call AppendVectorToLVArray here.
jasonmreding
left a comment
There was a problem hiding this comment.
The overall change looks good to me. I would still like to see some minor cleanup from the comments that are still open. The biggest issues for me at this point are:
- Answering the question about Linux RT compatibility.
- Cleaning up the unit tests a bit more to minimize code duplication through parametric tests where appropriate.
When testing this change against LV 2025 and some LV measurement plugins, I encountered crashes on project close that took me a while to figure out. It turns out this is due to existing bugs that I am not going to try and include fixes for as part of this PR. However, they are fairly serious issues so I would like to create a follow on PR after this one is completed and before publishing a new release.
There was a problem hiding this comment.
@jasonmreding Thanks for identifying the issues with lv_message_efficient. For now, I’m keeping those issues open since its future is still uncertain - we haven’t yet decided whether to enable it again or remove it. If we do decide to re-enable it, I’ll take your comments into account while making the necessary changes.
1. Pull request overview
This PR upgrades the gRPC submodule and refactors LVMessage to no longer inherit from google::protobuf::Message, integrating with gRPC via a custom grpc::SerializationTraits specialization and migrating serialization/parsing to public protobuf CodedInputStream/CodedOutputStream APIs.
Changes:
2. Architecture Diagrams
2.1 How LVMessage Worked Before (gRPC version <= v1.62.0)
flowchart TB subgraph LabVIEW["LabVIEW Application"] LV_CLUSTER["LabVIEW Cluster Data"] end subgraph DLL["labview_grpc_server.dll"] COPIER["ClusterDataCopier"] LV_MSG["LVMessage<br/>(inherits from protobuf::Message)"] subgraph LVMessage_Internal["LVMessage Internal"] VALUES["_values map"] METADATA["_metadata"] SERIALIZE["_InternalSerialize()"] PARSE["_InternalParse()"] end end subgraph gRPC_Layer["gRPC Library"] GRPC_CALL["BlockingUnaryCall<LVMessage>()"] DEFAULT_TRAITS["Default SerializationTraits<br/>(for protobuf::Message)"] subgraph Protobuf_Old["Protobuf v3.x (old)"] MSG_BASE["google::protobuf::Message"] GET_CLASS["GetClassData()<br/>✅ Had default impl"] CACHED_SIZE["GetCachedSize()"] SERIALIZE_PB["SerializeWithCachedSizes()"] end end subgraph Network["Network"] WIRE["Protobuf Wire Format<br/>(binary bytes)"] end LV_CLUSTER -->|"CopyFromCluster()"| COPIER COPIER --> VALUES VALUES --> SERIALIZE GRPC_CALL -->|"Serialize message"| DEFAULT_TRAITS DEFAULT_TRAITS -->|"Calls virtual methods"| MSG_BASE MSG_BASE --> GET_CLASS GET_CLASS -->|"Default impl OK"| CACHED_SIZE CACHED_SIZE --> SERIALIZE_PB SERIALIZE_PB -->|"Delegates to"| SERIALIZE SERIALIZE --> WIRE LV_MSG -.->|"inherits"| MSG_BASE style GET_CLASS fill:#90EE90,stroke:#228B22 style LV_MSG fill:#87CEEB,stroke:#4682B4 style DEFAULT_TRAITS fill:#DDA0DD,stroke:#8B008BHow it worked:
LVMessage._valuesviaClusterDataCopierBlockingUnaryCall<LVMessage>()SerializationTraitstreatsLVMessageas aprotobuf::MessageGetClassData()→ had a default implementation ✅LVMessage::_InternalSerialize()to write wire format2.2 What Broke Now (gRPC version > v1.63.0)
flowchart TB subgraph LabVIEW["LabVIEW Application"] LV_CLUSTER["LabVIEW Cluster Data"] end subgraph DLL["labview_grpc_server.dll"] COPIER["ClusterDataCopier"] LV_MSG["LVMessage<br/>(inherits from protobuf::Message)"] subgraph LVMessage_Internal["LVMessage Internal"] VALUES["_values map"] SERIALIZE["_InternalSerialize()"] GET_CLASS_IMPL["GetClassData() override<br/>❌ returns nullptr"] end end subgraph gRPC_Layer["gRPC Library"] GRPC_CALL["BlockingUnaryCall<LVMessage>()"] DEFAULT_TRAITS["Default SerializationTraits<br/>(for protobuf::Message)"] subgraph Protobuf_New["Protobuf v3.29.0+ (new)"] MSG_BASE["google::protobuf::Message"] GET_CLASS["GetClassData()<br/>⚠️ NOW PURE VIRTUAL"] ACCESS_CACHE["AccessCachedSize()"] DEREF["classData->cached_size_offset<br/>💥 CRASH: nullptr dereference"] end end subgraph Crash["CRASH"] EXCEPTION["❌ Access Violation<br/>GetClassData() returned nullptr"] end LV_CLUSTER -->|"CopyFromCluster()"| COPIER COPIER --> VALUES GRPC_CALL -->|"Serialize message"| DEFAULT_TRAITS DEFAULT_TRAITS -->|"Calls virtual methods"| MSG_BASE MSG_BASE --> GET_CLASS GET_CLASS -->|"Calls our override"| GET_CLASS_IMPL GET_CLASS_IMPL -->|"returns nullptr"| ACCESS_CACHE ACCESS_CACHE -->|"Tries to use nullptr"| DEREF DEREF --> EXCEPTION LV_MSG -.->|"inherits"| MSG_BASE style GET_CLASS fill:#FF6B6B,stroke:#8B0000 style GET_CLASS_IMPL fill:#FF6B6B,stroke:#8B0000 style DEREF fill:#FF6B6B,stroke:#8B0000 style EXCEPTION fill:#FF0000,stroke:#8B0000,color:#FFFFFF style SERIALIZE fill:#D3D3D3,stroke:#808080What breaks:
LVMessage._valuesBlockingUnaryCall<LVMessage>()SerializationTraitstreatsLVMessageas aprotobuf::MessageGetClassData()→ now pure virtual, requires implementationGetClassData()returnsnullptr(we can't construct validClassData)classData->cached_size_offset→ 💥 CRASHReadINT32,ReadUINT32,ReadINT64,ReadSINT32,ReadFIXED32,ReadFLOAT, etc.) have been removed frommap_type_handler.h(commit titled "Remove dead code" - file went from 610 to 377 lines). These were internal implementation details for map parsing, never part of the public API. The corresponding encoder functions (WireFormatLite::WriteInt32ToArray, etc.) remain available.2.3 The New Architecture (Solution with SerializationTraits)
flowchart TB subgraph LabVIEW["LabVIEW Application"] LV_CLUSTER["LabVIEW Cluster Data"] end subgraph DLL["labview_grpc_server.dll"] COPIER["ClusterDataCopier"] subgraph LVMessage_New["LVMessage (standalone - NO protobuf inheritance)"] VALUES["_values map"] METADATA["_metadata"] SERIALIZE["SerializeToByteBuffer()"] PARSE["ParseFromByteBuffer()"] CODED["Uses CodedOutputStream<br/>& CodedInputStream"] end subgraph Traits["grpc::SerializationTraits<LVMessage>"] TRAIT_SER["Serialize()"] TRAIT_DES["Deserialize()"] end end subgraph gRPC_Layer["gRPC Library"] GRPC_CALL["BlockingUnaryCall<LVMessage>()"] CUSTOM_TRAITS["Custom SerializationTraits<br/>✅ Bypasses protobuf::Message"] end subgraph Protobuf_Helpers["Protobuf (helper only)"] CODED_STREAM["io::CodedOutputStream<br/>io::CodedInputStream"] WIRE_FORMAT["WireFormatLite"] end subgraph Network["Network"] WIRE["Protobuf Wire Format<br/>(binary bytes)"] end LV_CLUSTER -->|"CopyFromCluster()"| COPIER COPIER --> VALUES GRPC_CALL -->|"Looks up traits"| CUSTOM_TRAITS CUSTOM_TRAITS -->|"Calls our custom"| TRAIT_SER TRAIT_SER -->|"Calls"| SERIALIZE SERIALIZE -->|"Uses helper APIs"| CODED CODED --> CODED_STREAM CODED_STREAM --> WIRE_FORMAT WIRE_FORMAT --> WIRE style CUSTOM_TRAITS fill:#90EE90,stroke:#228B22 style TRAIT_SER fill:#90EE90,stroke:#228B22 style TRAIT_DES fill:#90EE90,stroke:#228B22 style LVMessage_New fill:#87CEEB,stroke:#4682B4 style CODED_STREAM fill:#DDA0DD,stroke:#8B008BHow the solution works:
LVMessage._values(unchanged)BlockingUnaryCall<LVMessage>()SerializationTraits<LVMessage>✅LVMessage::SerializeToByteBuffer()directlyLVMessageusesCodedOutputStream(public protobuf helper API)GetClassData()call ever happens - we bypassprotobuf::Messageentirely3. Summary per file
data()instead ofc_str()for binary copies.VerifyUtf8Stringsignature and removes dependency onWireFormatLite::Operation.WireFormatLite::VerifyUtf8Stringusage withutf8_rangevalidation.<semaphore.h>conflicts (but see suggestion re: rename).CodedOutputStreamand introduces cached-size fields.NOMINMAXfor Windows builds.std::string.NOMINMAXfor Windows builds.CodedOutputStreamand public helpers.CodedInputStream-based overrides.CodedInputStream(direct-to-cluster).CodedInputStreamAPIs.NOMINMAXfor Windows builds.