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

Avoid redundant read/write of allocation/claims HAMT root when unchanged #1030

Merged
merged 2 commits into from
Jan 18, 2023

Conversation

anorth
Copy link
Member

@anorth anorth commented Jan 12, 2023

This saves 464k gas (4.9%) on a simple transfer making a single allocation an no claims (or vice versa), on an otherwise empty state.

Progress on #1020

@codecov-commenter
Copy link

Codecov Report

Merging #1030 (5df7b71) into master (b84d38c) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1030      +/-   ##
==========================================
+ Coverage   89.26%   89.36%   +0.09%     
==========================================
  Files          93       93              
  Lines       19550    19554       +4     
==========================================
+ Hits        17452    17474      +22     
+ Misses       2098     2080      -18     
Impacted Files Coverage Δ
actors/verifreg/src/lib.rs 92.56% <100.00%> (ø)
actors/verifreg/src/state.rs 99.46% <100.00%> (+0.01%) ⬆️
actors/cron/src/lib.rs 88.23% <0.00%> (-5.89%) ⬇️
test_vm/src/lib.rs 89.60% <0.00%> (+0.22%) ⬆️
actors/miner/src/testing.rs 92.05% <0.00%> (+0.24%) ⬆️
actors/miner/src/deadline_info.rs 96.62% <0.00%> (+16.85%) ⬆️

@anorth
Copy link
Member Author

anorth commented Jan 12, 2023

Note that this will be transparently fixed also when the actors can depend on a version of H/AMT that includes the fixes for filecoin-project/ref-fvm#1443. Due to coupling of these datastructure with FVM development, the fixes aren't easy to just upgrade to without risking a dependency mess, making local repo patches more complex, etc.

This change is still beneficial as it avoids the redundant load, too.

@Stebalien
Copy link
Member

See #1052.

@anorth
Copy link
Member Author

anorth commented Jan 15, 2023

@arajasek I think this is still worth merging to avoid the redundant read too.

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@anorth anorth changed the title Avoid redundant write of allocation/claims HAMT root when unchanged Avoid redundant read/write of allocation/claims HAMT root when unchanged Jan 18, 2023
@anorth anorth enabled auto-merge (squash) January 18, 2023 21:54
@anorth anorth merged commit e28bc90 into master Jan 18, 2023
@anorth anorth deleted the anorth/datacap-optimise branch January 18, 2023 22:10
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants