Use a slice to store section address for a field #358
Conversation
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
CascadingRadium
changed the title
CascadingRadium
requested review from
Likith101,
Thejas-bhat,
abhinavdangeti,
capemox,
Copilot and
maneuvertomars
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request optimizes memory usage and lookup performance for the fieldsSectionsMap data structure by replacing an inner map with a slice. The change leverages the fact that section IDs form a dense range from 0 to SectionCount.
Key Changes:
- Changed
fieldsSectionsMaptype from[]map[uint16]uint64to[][]uint64, eliminating map overhead - Added
SectionCountconstant to track the total number of section types - Refactored
loadFieldmethod to return the field section map instead of accepting it as a parameter
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| section.go | Adds SectionCount constant to automatically track the total number of section types for sizing slice allocations |
| segment.go | Updates fieldsSectionsMap type definition, refactors loadField signature to return the section map, and initializes section maps as slices instead of maps |
| write.go | Removes obsolete FIXME comment about hard-coding section count, as the code now properly uses dynamic len(segmentSections) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add new sections above this line. | ||
| // SectionCount automatically reflects the total number of sections | ||
| // and is used to track how many sections can be registered. | ||
| SectionCount |
Member
There was a problem hiding this comment.
Perhaps, rename to NumSections - as this is not quite a section enum.
| // create an address mapping array for each of the segment sections | ||
| // if the field has a valid section index, then the address will be non-zero | ||
| // else it will be zero. | ||
| fieldSectionMap := make([]uint64, SectionCount) |
Member
There was a problem hiding this comment.
I don't quite* follow the reason for this in this method - what're we using this for.
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.
fieldsSectionsMapis of type[]map[uint16]uint64, mappingfieldID → section → address.SectionCount, determined at process initialization, we can replace the inner map with a slice of lengthSectionCount.