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

feat: reduce block overhead effect on estimations #7175

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

jakmeier
Copy link
Contributor

  1. Use account ids without a shared prefix.
  2. Subtract block overhead instead of just checking it is insignificant.

resolves #7003

1. Use account ids without a shared prefix.
2. Subtract block overhead instead of just checking it is insignificant.

resolves near#7003
@jakmeier jakmeier requested a review from matklad July 12, 2022 10:36
@jakmeier jakmeier requested a review from a team as a code owner July 12, 2022 10:36
@jakmeier
Copy link
Contributor Author

This change increases the unique DB accesses by about 50% in ActionReceiptCreation estimation. This is not very noticeable when using the icount metric. But on time measurements, the gas estimation goes up a lot. It will be interesting to see the continuous estimation results the day after.

@jakmeier
Copy link
Contributor Author

Oh and most importantly, now the block size of 10 gives the same results as block size of 100. (Reducing it down to 1 still makes a difference but it is no longer 2x, it is more like 1.3x and it is almost invisible for block_size > 3.)

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

This makes sense!

I wonder if there are situations where account sharing a prefix would be a worse case? I guess a good way to check that would be to see if any CEs go down tomorrow?

use std::path::{Path, PathBuf};
use std::sync::Arc;

fn get_account_id(account_index: u64) -> AccountId {
AccountId::try_from(format!("near_{}_{}", account_index, account_index)).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, never understood why we account-index twice here.

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's a bit weird. It's to make it longer I suppose, as the account name length does matter quite a bit for how deep in a trie it can be.

Not sure if it was intentional by whomever designed this but this creates somewhat interesting paths for every account that look something like this:

  • extension node from root to "near_"
  • several branches to get through first account index
  • another extension for the second index

Almost the same thing could be achieved with "near_{account_index}_blablabla". But now there is a potential of sharing the extension node, as the state will be the same for all accounts and I think they will produce the same hashes.

Honestly, I don't think it matters. But let's not swap out too many gears at once.

@jakmeier
Copy link
Contributor Author

This makes sense!

I wonder if there are situations where account sharing a prefix would be a worse case? I guess a good way to check that would be to see if any CEs go down tomorrow?

Interesting point! I guess it could lead to smaller branch trie nodes. Maybe other things as well, I haven't thought about it too much. But yeah, definitely I will take a close look at the changes tomorrow.

@jakmeier jakmeier merged commit d0d395f into near:master Jul 12, 2022
@jakmeier jakmeier deleted the stable_receipt_creation_estimation branch July 12, 2022 12:14
jakmeier added a commit to jakmeier/nearcore that referenced this pull request Aug 15, 2022
In near#7175 the subtraction of block overhead was introduced wrong.
Instead of subtracting it once per block, it was subtracted once per
transaction within the block. This leads to subtraction underflows that
also showed up in the continuous estimation.
jakmeier added a commit to jakmeier/nearcore that referenced this pull request Aug 15, 2022
In near#7175 the subtraction of block overhead was introduced wrong.
Instead of subtracting it once per block, it was subtracted once per
transaction within the block. This leads to subtraction underflows that
also showed up in the continuous estimation.
jakmeier added a commit to jakmeier/nearcore that referenced this pull request Aug 15, 2022
In near#7175 the subtraction of block overhead was introduced wrong.
Instead of subtracting it once per block, it was subtracted once per
transaction within the block. This leads to subtraction underflows that
also showed up in the continuous estimation.
near-bulldozer bot pushed a commit that referenced this pull request Aug 17, 2022
In #7175 the subtraction of block overhead was introduced wrong.
Instead of subtracting it once per block, it was subtracted once per
transaction within the block. This leads to subtraction underflows that
also showed up in the continuous estimation.
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.

Investigate impact of block size in estimator
2 participants