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: assorted synthetic benchmarks for SyncState #3868

Merged
merged 17 commits into from
Apr 26, 2023

Conversation

feuGeneA
Copy link
Contributor

@changeset-bot
Copy link

changeset-bot bot commented Apr 18, 2023

⚠️ No Changeset found

Latest commit: 2415c08

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Apr 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 26, 2023 6:09pm
hardhat-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 26, 2023 6:09pm

crates/rethnet_evm/benches/state_ref_basic.rs Outdated Show resolved Hide resolved
crates/rethnet_evm/benches/state_ref_basic.rs Outdated Show resolved Hide resolved
|| state.clone(),
|mut state| {
for i in *number_of_addresses..1 {
use revm::db::State;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please move all of the local use statements to the top of the file. There is no benefit in Rust to have them locally, in this case.

Use cases where it would be helpful is:

  • if you have conflicting naming; or
  • if you are including use statement in a scope that's behind a feature flag, as you'd also need to put the use statement itself behind the feature flag if you do it at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry I've been stubborn about this, and I realize that there is no benefit in Rust, but as a reader of the code I actually find it immensely beneficial. Given that such imported traits have no direct mention in the file, I appreciate having their placement restricted to the scope they're actually needed in. If that scope is the entire file, due to repeated uses within the file, then at the top of the file is the right place. But if the use is only in a single inner scope, then I feel that the placement should be within that single inner scope. (Note that in this particular case I do have a repetition of revm::db::State within this file, but that's because I expect one of those usages to go away depending on which version of that function we end up keeping.)

Copy link
Member

Choose a reason for hiding this comment

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

A language server provides you that information if you need a reminder though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, but what if for example i'm just browsing the code on github? etc. maybe i'm being overly principled, but it doesn't seem right to depend on tooling for readability. in my latest draft all of the use are at the top, by virtue of using SyncState, so it's a moot point for this PR, but this does remain one of the (very) few smelly things in my limited Rust experience.

Copy link
Member

Choose a reason for hiding this comment

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

GitHub these days also offers redirects to type definitions. It seems like you're trying to tailor to a niche while cluttering the code with unnecessary noise for the majority of use cases.

crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
@feuGeneA feuGeneA requested a review from Wodann April 24, 2023 16:53
crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
@feuGeneA feuGeneA marked this pull request as draft April 26, 2023 04:01
crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
crates/rethnet_evm/benches/state.rs Outdated Show resolved Hide resolved
Co-authored-by: Wodann <Wodann@users.noreply.github.com>
@Wodann Wodann marked this pull request as ready for review April 26, 2023 18:11
@Wodann Wodann changed the title StateRef::basic worst-case benchmark feat: assorted worst-case benchmarks for SyncState Apr 26, 2023
@Wodann Wodann changed the title feat: assorted worst-case benchmarks for SyncState feat: assorted synthetic benchmarks for SyncState Apr 26, 2023
@Wodann Wodann linked an issue Apr 26, 2023 that may be closed by this pull request
@feuGeneA feuGeneA merged commit ff156bf into rethnet/main Apr 26, 2023
@feuGeneA feuGeneA deleted the rethnet/71-staterefbasic-worst-case-benchmark branch April 26, 2023 18:39
Wodann added a commit that referenced this pull request Apr 26, 2023
Co-authored-by: Wodann <Wodann@users.noreply.github.com>
Wodann added a commit that referenced this pull request Apr 26, 2023
Co-authored-by: Wodann <Wodann@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

StateRef::basic worst-case benchmark
2 participants