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

EVM-899: Skip write when no change #709

Conversation

aakoshh
Copy link

@aakoshh aakoshh commented Sep 23, 2022

This PR is an investigation into why for example the array_read_n1 scenario in #697 has a constant put size, even though it's a read only transaction.

After some digging it turns out the culprit is HAMT::flush which puts the root into CBOR store even if it wasn't dirty, if for nothing else then to calculate its CID. This could be avoided if the root CID was stored in the beginning and kept until some operation actually changes the content.

One could argue that this is a pathological case, because if there's no change in a contract then why would anyone send a transaction to the blockchain. Indeed the Solidity transaction is a view which can be executed off-chain, which costs no gas.

Still, the HAMT could avoid serialising the data to calculate the CID. Maybe in the future we will have actors sending read-only messages to each other, and this will be doing unnecessary work?

Not doing unnecessary flushing would also work better with my proposal for typed CIDs at #489, which currently live in here on a fork maintained by @adlrocha, where a call to modify may or may not actually change the value, but it will incur the cost of re-serializing the root node anyway.

I opened filecoin-project/ref-fvm#901 in the ref-fvm repo to make this go away, but maybe people won't like the extra book keeping required, we'll see. The updated measurements in this PR are collected with that fix and show the reduction in bytes written in the tests which did read-only queries. The remaining 87 bytes written are the MockRuntime saving the supposedly updated actor state, but that's just two CIDs. This is not something we can avoid because Runtime::transaction don't have Eq constraints on the state, so it can't check whether putting it into the store is worth the effort.

The PR also changes System::set_storage to return early if there's no modification to the data, to avoid traversing the HAMT again.

closes filecoin-project/ref-fvm#899

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2022

Codecov Report

Merging #709 (7f2fef6) into next-optimize-evm-859-storage-footprint (856ca95) will increase coverage by 0.15%.
The diff coverage is 81.81%.

Additional details and impacted files

Impacted file tree graph

@@                             Coverage Diff                             @@
##           next-optimize-evm-859-storage-footprint     #709      +/-   ##
===========================================================================
+ Coverage                                    68.71%   68.86%   +0.15%     
===========================================================================
  Files                                          113      113              
  Lines                                        22253    22248       -5     
===========================================================================
+ Hits                                         15291    15322      +31     
+ Misses                                        6962     6926      -36     
Impacted Files Coverage Δ
actors/evm/src/interpreter/system.rs 90.00% <81.81%> (-7.15%) ⬇️
actors/market/src/lib.rs 63.80% <0.00%> (+0.57%) ⬆️
actors/power/src/lib.rs 84.53% <0.00%> (+0.63%) ⬆️
test_vm/src/lib.rs 64.87% <0.00%> (+1.15%) ⬆️
actors/cron/src/lib.rs 96.55% <0.00%> (+3.44%) ⬆️
actors/miner/src/deadline_info.rs 96.62% <0.00%> (+16.85%) ⬆️

@aakoshh aakoshh marked this pull request as ready for review September 23, 2022 19:52
@aakoshh aakoshh requested review from vyzo, raulk and Stebalien September 23, 2022 20:03
@Stebalien
Copy link
Member

The extra book-keeping upstream is fine, and honestly seems like the best approach.

@aakoshh aakoshh reopened this Oct 1, 2022
@aakoshh aakoshh merged commit 4707c62 into next-optimize-evm-859-storage-footprint Oct 1, 2022
@aakoshh aakoshh deleted the evm-899-skip-write-when-no-change branch October 1, 2022 08:46
aakoshh added a commit that referenced this pull request Oct 1, 2022
* EVM-899: Don't even call the HAMT store if there was no change.

* EVM-899: Measurement updates with flush fix.
Stebalien pushed a commit that referenced this pull request Dec 7, 2022
* EVM-899: Skip write when no change (#709)

* EVM-899: Don't even call the HAMT store if there was no change.

* EVM-899: Measurement updates with flush fix.

* EVM-890: No hashing by HAMT (#730)

* EVM-890: Use the identity hasher in the EVM

* EVM-890: Update the measurements.

* EVM-890: Fix newline in bash script.

* EVM-890: Fix charts missing series.

* EVM-890: Add key wrapper and use big endian order in hash.

* EVM-890: Update measurements with big endian key order.

* EVM-890: Remove leftover let binding.

* EVM-890: Derive serde

* EVM-890: Fix jq check.

* EVM-891: HAMT link extension measurements (#739)

* EVM-891: Use extensions.

* EVM-891: Update measurements

* EVM-891: Point at the PR branch.

* EVM-891: Update measurements after single char fields

* EVM-890: Fix jq check.

* EVM-892: Keep root small (#740)

* EVM-892: Keep the first 2 levels free of data.

* EVM-892: Update patch branch

* EVM-892: Reduce bit width to 5 (#741)

* EVM-1085: Use Kamt (#855)

* EVM-1085: Use Kamt

* EVM-1085: Update measurements

* EVM-1085: Use the kamt branch on CI.

* EVM-1085: Remove use_extensions option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants