Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catchpoints: Small tweaks, mostly to comments #5195

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

jannotti
Copy link
Contributor

Existing tests pass, very few code changes.

id: childNodeID,
hashIndex: d[0],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This is because we write them in this order in all the nearby initializations.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #5195 (44efbb0) into master (2594c32) will increase coverage by 0.05%.
The diff coverage is 53.57%.

@@            Coverage Diff             @@
##           master    #5195      +/-   ##
==========================================
+ Coverage   53.55%   53.60%   +0.05%     
==========================================
  Files         430      430              
  Lines       54085    54080       -5     
==========================================
+ Hits        28966    28992      +26     
+ Misses      22873    22848      -25     
+ Partials     2246     2240       -6     
Impacted Files Coverage Δ
crypto/merkletrie/committer.go 100.00% <ø> (ø)
ledger/store/merkle_committer.go 51.85% <ø> (ø)
crypto/merkletrie/cache.go 88.72% <14.28%> (ø)
crypto/merkletrie/trie.go 69.62% <50.00%> (+3.20%) ⬆️
crypto/merkletrie/node.go 93.39% <100.00%> (+1.76%) ⬆️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

crypto/merkletrie/node.go Show resolved Hide resolved
@@ -63,7 +63,7 @@ type Trie struct {
root storedNodeIdentifier
nextNodeID storedNodeIdentifier
lastCommittedNodeID storedNodeIdentifier
cache *merkleTrieCache
cache merkleTrieCache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: most of the changes in the PR that changes from mt.cache to &mt.cache are related with this line.

Is it because of memory management that we want to change from pointer to instance of cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it just didn't need to be a pointer, so I figured we'd avoid the allocation.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are fine but what is rationale going from pointer to a value for merkleTrieCache ?

@jannotti
Copy link
Contributor Author

Changes are fine but what is rationale going from pointer to a value for merkleTrieCache ?

It was just a pointless allocation. The pointed to thing is never assigned into another merkleTrieCache, so it might as well be embedded.

@jannotti
Copy link
Contributor Author

I just added a couple more very safe typo changes.

If anyone wants me to back out the pointer change, I will. I'm just trying neaten things up before I dig into performance for state commitments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants