-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[indexer-alt] Add benchmark command #20352
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're missing some files in the PR -- what's here seems reasonable, but I left some questions for your consideration! Thanks @lxfind .
crates/sui-indexer-alt/src/args.rs
Outdated
#[clap(long)] | ||
consistent_range: Option<u64>, | ||
#[command(flatten)] | ||
sequential_pipeline_config: SequentialPipelineConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is even more specific than for sequential pipelines -- let's call it a ConsistencyConfig
or something?
crates/sui-indexer-alt/src/lib.rs
Outdated
// If true, the indexer will bootstrap from genesis. | ||
// Otherwise it will skip the pipelines that rely on genesis data. | ||
with_genesis: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth doing this the other way? I.e. infer whether we need to bootstrap genesis based on whether the pipelines we're about to run need it or not? Seems like that would be less error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to infer that other than hardcode the list of pipelines that need genesis somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding it is really the only way, but it's driven by the types, because the pipelines that need bootstrapping need a value that is only generated from the bootstrapping process to initialise themselves with.
b914ad1
to
97010a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some small nits.
There's also the small thread about bootstrapping genesis -- as I was thinking about it, that discussion dove-tails with a separate discussion in the standalone synthetic benchmark generation logic where you get rid of checkpoint 0 -- is it worth simplifying things by keeping both? I.e. simulacrum always generates the genesis checkpoint, and we always bootstrap the genesis table, even during benchmarks?
|
||
[features] | ||
default = [] | ||
benchmark = ["sui-synthetic-ingestion"] # This will be used to enable benchmark mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly for my own education -- in some places, I've seen this written like it is here, and in other cases the feature name is prefixed with a "dep:" when it enables the dependency.
Is there a difference between the two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I am now confused on the difference too.
pipeline: Vec<String>, | ||
|
||
#[command(flatten)] | ||
sequential_pipeline_config: ConsistencyConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: naming.
sequential_pipeline_config: ConsistencyConfig, | |
consistency_config: ConsistencyConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think this belongs inside pipeline
-- it's specific to our indexer, while pipeline
is part of the indexer framework. Let's keep it at the top-level for now.
I was hoping to be able to also download some checkpoint data from mainnet, and use them to benchmark the indexer. The genesis checkpoint would not be there. |
Description
This PR adds a benchmark command to indexer-alt.
It would start the indexer using the provided parameters, start and stop based on the local ingestion data.
It then measures the time and TPS.
A few structural changes to indexer-alt code:
Test plan
Run locally
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.