Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes refmap construction by replacing the previous functional style (creating and concatenating new IdentityDicts) with a mutable builder pattern, significantly improving test suite performance by approximately 15 seconds total.
- Refactored
_get_ref_map()to_build_ref_map()across all classes, using mutation instead of functional concatenation - Modified
_populate_def_proto_*methods to accept a pre-builtref_mapparameter and removed return values - Added
interface_onlyparameter toBlock._build_ref_map()to generate refmaps only for sub-block interfaces
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| edg/core/Core.py | Added RefMapType type alias, introduced _create_ref_map() wrapper and refactored _build_ref_map() to use mutable pattern; updated _populate_metadata() signature |
| edg/core/Blocks.py | Updated _populate_def_proto_* methods to accept ref_map parameter and removed return statements; refactored _build_ref_map() to mutate instead of return |
| edg/core/HierarchyBlock.py | Added interface_only parameter to _build_ref_map(), updated _populate_def_proto_* methods, and moved refmap creation to top of _def_to_proto() |
| edg/core/Ports.py | Refactored _build_ref_map() from functional to mutable pattern for Port and Bundle classes |
| edg/core/Link.py | Moved refmap creation to top of _def_to_proto() and removed _get_ref_map_allocate() method, inline allocate path construction into Connection.make_connection() |
| edg/core/Array.py | Refactored Vector _build_ref_map() to mutable pattern and updated MapExtractBinding.expr_to_proto() to call _create_ref_map() |
| edg/core/Binding.py | Updated all expr_to_proto() signatures to use Refable.RefMapType |
| edg/core/ConstraintExpr.py | Updated _expr_to_proto() signature to use Refable.RefMapType |
| edg/core/ArrayExpr.py | Updated expr_to_proto() signature to use Refable.RefMapType |
| edg/core/Generator.py | Updated to use _create_ref_map() instead of _get_ref_map() |
| edg/core/DesignTop.py | Updated _populate_def_proto_block_contents() to accept ref_map parameter and use _create_ref_map() for multipack blocks |
| edg/core/PortBlocks.py | Removed obsolete _get_ref_map() overrides from PortBridge and PortAdapter |
| edg/electronics_model/CircuitBlock.py | Removed obsolete _get_ref_map() overrides from CircuitPortBridge and CircuitPortAdapter |
| edg/electronics_model/SvgPcbTemplateBlock.py | Updated to use _create_ref_map() instead of _get_ref_map() and reordered imports |
💡 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.
Optimizes refmap construction by using a mutable builder instead of the repeated copies needed with the previous functional style. Shaves about ~10s off the test suite.
Also generates refmaps only for sub-block interfaces (instead of the full sub-block, recursively) and avoids creating refmap multiple times by creating the refmap only once and passing the refmap into the populate..., and eliminates the return from those functions since they rely on mutating the input. These shave another ~5s off the test suite.
TODOs for future PRs: