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

perf(store): Add benchmarks #166

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

perf(store): Add benchmarks #166

wants to merge 1 commit into from

Conversation

john-z-yang
Copy link
Member

@john-z-yang john-z-yang commented Feb 5, 2025

Adds a benchmark for simulating worker ⇔ store interactions.

Use make bench or cargo bench to run.

bench_InflightActivationStore/process_activations
                        time:   [13.091 s 13.313 s 13.525 s]
                        thrpt:  [739.39  elem/s 751.15  elem/s 763.88  elem/s]
                 change:
                        time:   [-21.574% -19.199% -16.927%] (p = 0.00 < 0.05)
                        thrpt:  [+20.376% +23.760% +27.508%]
                        Performance has improved.

)
.await
.unwrap();
sleep(Duration::from_millis(50)).await;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to sleep in a benchmark test?

Copy link
Member Author

@john-z-yang john-z-yang Feb 5, 2025

Choose a reason for hiding this comment

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

I tried letting it run without a sleep, that gives me this error when I have a large number of workers (>16)

'tokio-runtime-worker' panicked at benches/inflight_activation_store_bench.rs:32:18:
called `Result::unwrap()` on an `Err` value: error returned from database: (code: 5) database is locked

Caused by:
    (code: 5) database is locked
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This seems like a very strange error to me as I expect if the db is contended, the .await call would just wait.

Some cursory googling suggests that this is because sqlite is actually single threaded under the hood for writes, and for those 2 calls we're doing writes with UPDATE.

The way I can get around with this error is to force only a single connection when we initialize the pool by pinning the max connection to 1 when the store is being initialized. Or just use a single worker.

let sqlite_pool = PoolOptions::new().max_connections(1).connect(url).await?;

But I feel like that should be a separate change.

@john-z-yang john-z-yang force-pushed the john/benchmark branch 4 times, most recently from b9e4d41 to 82c092d Compare February 5, 2025 22:24
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