Reduce proto CopyFrom for deeply nested structures#440
Merged
Conversation
performance gains are marginal =(
There was a problem hiding this comment.
Pull request overview
This PR refactors the protobuf serialization code to improve performance by replacing CopyFrom() calls with in-place population methods (populate_*) for deeply nested structures. The change eliminates the need to create temporary protobuf objects, instead populating them directly in their final destination.
Key changes:
- Replaced
_expr_to_proto()with_populate_expr_proto()that takes a protobuf object to populate - Replaced
_decl_to_proto()with_populate_decl_proto()that populates in-place - Replaced
_instance_to_proto()with_populate_portlike_proto()that populates in-place - Removed unused
StructuredMetadatabase class that is no longer needed
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| edg/core/Ports.py | Updated Port classes to use _populate_portlike_proto() and _populate_decl_proto() methods instead of returning proto objects |
| edg/core/HierarchyBlock.py | Changed constraint and parameter initialization to use populate methods |
| edg/core/Core.py | Removed unused StructuredMetadata class and updated metadata type hints |
| edg/core/ConstraintExpr.py | Refactored expression types to use _populate_decl_proto() and _populate_expr_proto() with SetInParent() for optional fields |
| edg/core/Blocks.py | Updated block proto generation to use populate methods for parameters, ports, and constraints |
| edg/core/Binding.py | Comprehensive refactoring of all binding classes to use populate_expr_proto() and populate_literal_proto() methods; made ArrayBinding inherit from Binding instead of LiteralBinding |
| edg/core/ArrayExpr.py | Updated array expression proto generation to use populate methods |
| edg/core/Array.py | Updated array and vector proto generation to use _populate_portlike_proto() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces it with populate_x, which is more efficient.
CopyFrom with a ref_map (or any other kind of dict) remains, the data there tends to be small so it's probably the least worst option.
Negligible effect on performance in practice.
Resolves #18