hir-ty: add incremental benchmarks#19934
hir-ty: add incremental benchmarks#19934davidbarsky wants to merge 1 commit intorust-lang:masterfrom
Conversation
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
left a comment
ChayimFriedman2
left a comment
There was a problem hiding this comment.
I'm quite confused. What is the point of those benchmarks? If we want to test incrementality, we should check salsa's logs. But it looks like you want to measure raw speed, why?
crates/hir-def/src/lib.rs
Outdated
| signatures::VariantFields, | ||
| }; | ||
|
|
||
| pub use crate::db::DefDatabase; |
There was a problem hiding this comment.
The db module is public, no need for this.
|
|
||
| #[salsa_macros::db] | ||
| #[derive(Clone)] | ||
| pub(crate) struct BenchmarkDB { |
There was a problem hiding this comment.
Maybe we need to do something with all the boilerplate with declaring a db.
There was a problem hiding this comment.
Can't we reuse the TestDB? Why do we need a new type
| fn benchmark_incremental_struct_addition(c: &mut Criterion) { | ||
| c.bench_function("incremental_struct_addition", |b| { | ||
| b.iter_batched( | ||
| || { |
There was a problem hiding this comment.
I'm afraid the cost of the setup will dwarf the cost of the benchmarked computation, not allowing criterion to accurately measure the time.
There was a problem hiding this comment.
The first closure, thankfully, is setup for the second closure, which is the actually benchmarked computation.
There was a problem hiding this comment.
I know that, what I meant is that the setup is too costly relative to the actual computation, and it runs on every iteration, so it can make the benchmarking inaccurate.
There was a problem hiding this comment.
Oh, I’m sorry: I misunderstood. I have noticed this is somewhat noisy, but I chalked it up to my work-managed computer being bad.
On Salsa, we've had success with http://codspeed.io/ and I wanted to set that up proper, useful benchmarks that measure incremental performance. The examples I have here are nowhere near sufficient to do reasonable performance measurements, but this contains some useful bones. |
davidbarsky
force-pushed
the
davidbarsky/add-some-incremental-benchmarks
branch
from
d8b8ad1 to
b926d5d
Compare
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
I think the source snippets here are likely way too small/simple to give us any interesting numbers
Didn't see this, sorry! At which size do you think the snippet would be sufficiently large? I imagine we'd want to keep it at one file, which would involve inlining dependencies if we're to use anything existing... |
|
Note sure, it doesn't need to be massive I think, but it likely wants to at least make use of some general type system things like coercion, autoderef, method lookups with multiple candidates etc. Just arithmetic on primitives doesn't do much, especially given that those traits on primitives have short circuiting logic iirc |
|
In fact, it would probably make more sense to just have these live in the main r-a crate as benchmarks that properly load up a small project? |
|
☔ The latest upstream changes (possibly #21804) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot
removed
the
S-waiting-on-review
4 participants
We didn't have these before, but we probably should. The only benchmarks here are for trait solving, but I can extend this to
inferas well.(Builds atop of #19914.)