Avoid querying unwritten values from changesets during historical lookups#2966
Merged
gakonst merged 1 commit intoparadigmxyz:mainfrom Jun 5, 2023
Merged
Conversation
petertdavies
force-pushed
the
optimize-historical-lookups
branch
from
3ca965f to
eb2e59f
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #2966 +/- ##
==========================================
+ Coverage 70.97% 70.99% +0.01%
==========================================
Files 520 520
Lines 67144 67195 +51
==========================================
+ Hits 47656 47704 +48
- Misses 19488 19491 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Member
|
@joshieDo PTAL |
gakonst
approved these changes
Jun 5, 2023
Member
gakonst
left a comment
gakonst
left a comment
There was a problem hiding this comment.
love it. makes a ton of sense.
Comment on lines
+52
to
+67
| if let Some(chunk) = | ||
| cursor.seek(history_key)?.filter(|(key, _)| key.key == address).map(|x| x.1 .0) | ||
| { | ||
| let chunk = chunk.enable_rank(); | ||
| let rank = chunk.rank(self.block_number as usize); | ||
| if rank == 0 && !cursor.prev()?.is_some_and(|(key, _)| key.key == address) { | ||
| return Ok(HistoryInfo::NotWritten) | ||
| } | ||
| if rank < chunk.len() { | ||
| Ok(HistoryInfo::InChangeset(chunk.select(rank) as u64)) | ||
| } else { | ||
| Ok(HistoryInfo::InPlainState) | ||
| } | ||
| } else { | ||
| Ok(HistoryInfo::NotWritten) | ||
| } |
Member
There was a problem hiding this comment.
It could be helpful if we had some inline ascii diagram showing how this works, I think basically it says:
If there's a value in the history table then it is:
- Not Written: If there are 0 elements in that entry AND there exists a previous entry in the table for that key, meaning it was previously written and now there was no change. We use this later for the optimization in the changeset read. In practice this is going to be rare as in most cases it will be
rank != 0and the if statement will short circuit. - InChangeset: If the number of items less than block number in that bucket is less than the items in the bucket, return the corresponding changeset key.
- InPlainState: If it's larger than the bucket len, then it means it's in the plain state
If there's no value, also return NotWritten.
| ) -> Result<HistoryInfo> { | ||
| // history key to search IntegerList of block number changesets. | ||
| let history_key = StorageShardedKey::new(address, storage_key, self.block_number); | ||
| let mut cursor = self.tx.cursor_read::<tables::StorageHistory>()?; |
Member
There was a problem hiding this comment.
same idea as above here but with the duptable making it a little more verbose
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR avoids querying the plain state or changesets for accounts and storage values if the history tables show the value has not been written. This will return incorrect results for queries about genesis accounts if you synced prior to #2606.
<1% of historical queries will incur an extra
cursor.prev(). I have not benchmarked the cost of this, but I assume it worth it to save full lookups in the PlainState/Changeset tables.The semantics of return value of
storage()have changed when the value is zero. The old semantics were "If the key has ever been written (including after the requested block) returnSome(StorageValue::ZERO), else returnNone. The new semantics are "If the key has not been written prior to the requested block returnNone, else returnSome(StorageValue::ZERO).