RUST-2004 Benchmark client bulk write #1293
Conversation
benchmarks/src/bench.rs
Outdated
| @@ -57,6 +62,38 @@ pub static TARGET_ITERATION_COUNT: Lazy<usize> = Lazy::new(|| { | |||
| .expect("invalid TARGET_ITERATION_COUNT") | |||
| }); | |||
|
|
|||
| static SMALL_DOC: OnceCell<Document> = OnceCell::const_new(); | |||
There was a problem hiding this comment.
Nit: I think these statics can be moved into the scope of the get_ functions that use them.
benchmarks/src/bench.rs
Outdated
| let data_path = DATA_PATH | ||
| .join("single_and_multi_document") | ||
| .join("small_doc.json"); | ||
| let mut file = spawn_blocking_and_await!(std::fs::File::open(data_path)) |
There was a problem hiding this comment.
Nit: It might be worth factoring out a utility function like
async fn from_json_file<T: DeserializeOwned>(path: impl AsRef<Path>) -> Result<T>
There was a problem hiding this comment.
makes sense, added a util function and statics for the other json files we read
benchmarks/src/bench.rs
Outdated
| @@ -69,7 +106,7 @@ pub trait Benchmark: Sized { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| async fn do_task(&self) -> Result<()>; | |||
| async fn do_task(&mut self) -> Result<()>; | |||
There was a problem hiding this comment.
My understanding is that the introduction of mut here is to support copying the write models in before_task rather than do_task (which absolutely makes sense).
Personally, I'd rather avoid the mut on do_task because I think it makes it harder to reason about; how would you feel about, instead, updating the Benchmark trait with
type TaskState = ();
async fn before_task(&mut self) -> Result<TaskState> {
Ok(())
}
async fn do_task(&self, state: TaskState) -> Result<()>;
That avoids the need for mut and the Option/take ceremony, but might be more type-level complication than it's worth. WDYT?
There was a problem hiding this comment.
good idea - I never love an option/take hack like this. updated with your suggestion, it's a bit noisy to add type TaskState = () to all the types but hopefully associated type defaults will be stabilized sometime soon
benchmarks/src/bench.rs
Outdated
| let data_path = DATA_PATH | ||
| .join("single_and_multi_document") | ||
| .join("small_doc.json"); | ||
| let mut file = spawn_blocking_and_await!(std::fs::File::open(data_path)) |
There was a problem hiding this comment.
makes sense, added a util function and statics for the other json files we read
benchmarks/src/bench.rs
Outdated
| @@ -69,7 +106,7 @@ pub trait Benchmark: Sized { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| async fn do_task(&self) -> Result<()>; | |||
| async fn do_task(&mut self) -> Result<()>; | |||
There was a problem hiding this comment.
good idea - I never love an option/take hack like this. updated with your suggestion, it's a bit noisy to add type TaskState = () to all the types but hopefully associated type defaults will be stabilized sometime soon
RUST-2004
This also includes some clippy fixes/general cleanup in the second and third commits.