Optimize size of Node and Array #7992
Merged
Conversation
This file contains 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
The size of Node and Array decresed from 64 and 128 to 56 and 112. Some redundant code removed in Spec class.
Reduse the stack usage of StringIndex::leaf_insert from 832 to 608 (debug) and StringIndex::TreeInsert from 208 to 128
tgoyne
approved these changes
Aug 22, 2024
|
This cuts the -O0 stack space used on the recursive path a bit so it'll reduce the maximum stack space required for an index update. There isn't really a clear answer to if it's by enough. |
ironage
reviewed
Aug 22, 2024
Comment on lines
937
to
+938
| case NodeChange::change_None: | ||
| return; | ||
| break; |
There was a problem hiding this comment.
It appears that this case should never happen, should we have a REALM_UNREACHABLE here?
ironage
reviewed
Aug 22, 2024
| m_keys.init_from_parent(); | ||
| } | ||
|
|
||
| m_keys.init_from_parent(); |
There was a problem hiding this comment.
consider adding an assertion that m_top.get_as_ref(s_col_keys_ndx) is not zero (not blocking)
There was a problem hiding this comment.
Well, we don't check the other entries and I am sure we will have an assertion down the line if the ref is zero.
Pull Request Test Coverage Report for Build jorgen.edelbo_406Details
💛 - Coveralls |
It is a long time since there was a difference in how init_from_parent() and update_from_parent() should work.
|
@ironage please have a second look. I have made some more changes. |
ironage
reviewed
Aug 26, 2024
Comment on lines
-1955
to
+1956
| m_top.update_from_parent(); | ||
| m_spec.update_from_parent(); | ||
| m_top.init_from_parent(); | ||
| m_spec.init_from_parent(); |
There was a problem hiding this comment.
nice, good to clean this confusion up.
ironage
approved these changes
Aug 26, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The size of Node and Array decresed from 64 and 128 to 56 and 112. Some redundant code removed in Spec class.
What, How & Why?
The second commit may fix #7847
☑️ ToDos
bindgen/spec.yml, if public C++ API changed