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

Pipelining #12015

Merged
merged 12 commits into from
Sep 23, 2024
Merged

Pipelining #12015

merged 12 commits into from
Sep 23, 2024

Conversation

nagisa
Copy link
Collaborator

@nagisa nagisa commented Aug 29, 2024

This reverts commit 4e93e46.
This reverts commit 86cd45b.

To address the issues seen that led to the functionality being reverted in the first place I had to add some mechanisms to query The State in a side-effect-free (pure) manner and to also revert some changes from the original change-set where we the new code would not produce the side-effects that were expected by the other nodes.

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 94.31034% with 33 lines in your changes missing coverage. Please review.

Project coverage is 71.54%. Comparing base (d6d6e47) to head (4d2ac5e).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
core/store/src/trie/mod.rs 77.14% 6 Missing and 10 partials ⚠️
runtime/runtime/src/pipelining.rs 95.26% 9 Missing ⚠️
runtime/runtime/src/lib.rs 97.19% 3 Missing ⚠️
core/store/src/lib.rs 89.47% 2 Missing ⚠️
core/store/src/trie/receipts_column_helper.rs 92.30% 0 Missing and 1 partial ⚠️
core/store/src/trie/update.rs 94.73% 1 Missing ⚠️
runtime/runtime/src/ext.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12015      +/-   ##
==========================================
+ Coverage   71.50%   71.54%   +0.03%     
==========================================
  Files         819      821       +2     
  Lines      164896   165172     +276     
  Branches   164896   165172     +276     
==========================================
+ Hits       117912   118169     +257     
- Misses      41829    41859      +30     
+ Partials     5155     5144      -11     
Flag Coverage Δ
backward-compatibility 0.17% <0.00%> (-0.01%) ⬇️
db-migration 0.17% <0.00%> (-0.01%) ⬇️
genesis-check 1.26% <0.00%> (-0.01%) ⬇️
integration-tests 38.73% <87.24%> (+0.08%) ⬆️
linux 71.33% <93.79%> (+0.01%) ⬆️
linux-nightly 71.12% <94.31%> (+0.05%) ⬆️
macos 52.51% <34.99%> (-0.40%) ⬇️
pytests 1.52% <0.00%> (-0.01%) ⬇️
sanity-checks 1.32% <0.00%> (-0.01%) ⬇️
unittests 65.24% <87.24%> (+0.01%) ⬆️
upgradability 0.21% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nagisa nagisa marked this pull request as ready for review August 30, 2024 13:32
@nagisa nagisa requested a review from a team as a code owner August 30, 2024 13:32
@nagisa nagisa requested review from Longarithm and pugachAG and removed request for Longarithm August 30, 2024 13:32
@nagisa
Copy link
Collaborator Author

nagisa commented Aug 30, 2024

Hey, @pugachAG this is now good to look at. In particular curious what's going to be your opinion on introducing function arguments to control side-effectfulness across the near_store crate, as opposed to adding a global switch (like what we already have in e.g. TrieAccountingCache.) I personally had some pretty bad experiences with a flag like this already so it seemed to make more sense to me to go for the argument, even if it ended up being a little less… straightforward…? pretty?

Should be quite reviewable on a commit-by-commit basis, with the first two being simple reverts of the reverts.

@nagisa
Copy link
Collaborator Author

nagisa commented Sep 2, 2024

This code was running without a hitch throughout the weekend until my instance ran out of disk early this morning 😓

Copy link
Contributor

@pugachAG pugachAG left a comment

Choose a reason for hiding this comment

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

@nagisa I assume that pipelining implementation is the same as it was in the reverted commit. Please let me know if that is not the case and I take another look.
New changes on the storage side look good to me 👍

Comment on lines 261 to 267
if let Some(key_value) = self.prospective.get(&key) {
return Ok(key_value.value.as_ref().map(<Vec<u8>>::clone));
} else if let Some(changes_with_trie_key) = self.committed.get(&key) {
if let Some(RawStateChange { data, .. }) = changes_with_trie_key.changes.last() {
return Ok(data.as_ref().map(<Vec<u8>>::clone));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks exactly the same as for get, consider extracting shared code to a separate method, something like get_from_updates

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I'll hold off on merging this until #12027 comes to a conclusion...

@nagisa
Copy link
Collaborator Author

nagisa commented Sep 6, 2024

Huh, it started failing tests badly after the rebase... Oh well... Something for future me to figure out.

EDIT: looks like something went wrong in 5cfd0d7...

The test was accidentally checking for the wrong things and has stopped
working once those wrong things have moved to other places...
Merged via the queue into near:master with commit e516989 Sep 23, 2024
27 of 29 checks passed
@nagisa nagisa deleted the pipelining branch September 23, 2024 14:17
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.

2 participants