Adapt dotnet memory allocations#705
Conversation
ricardoboss
force-pushed
the
issues/699-dotnet-memory-allocations
branch
from
2841ffc to
8edf525
Compare
epompeii
left a comment
epompeii
left a comment
There was a problem hiding this comment.
@ricardoboss thank you for all of your work so far!
I've made a few suggestions and had a couple of questions.
There was a problem hiding this comment.
Would it be possible to get a smaller example file or are they all this large?
There was a problem hiding this comment.
Yeah of course, it was just easily accessible for me. I can provide a smaller one
| pub mod dotnet { | ||
| use bencher_valid::{BYTES, NANOSECONDS}; | ||
|
|
||
| create_measure!(Latency, "Latency", "latency", NANOSECONDS); |
There was a problem hiding this comment.
We shouldn't need to create a new Latency Measure, we should just use the existing one.
There was a problem hiding this comment.
Oh ok, I thought every adapter should define its own set. Will switch to the default measure then
| Some(results_map.into()) | ||
| } | ||
|
|
||
| pub fn new_dotnet(benchmark_metrics: Vec<(BenchmarkName, Vec<DotNetMeasure>)>) -> Option<Self> { |
There was a problem hiding this comment.
For consistency, lets move this method above new_iai. Adapters are (mostly) sorted alphabetically across the code base.
| } | ||
|
|
||
| #[test] | ||
| fn adapter_c_sharp_dot_net_memory() { |
There was a problem hiding this comment.
Can the harness collect both memory and wall-clock time measurements at the same time or are they mutually exclusive?
There was a problem hiding this comment.
That is above my paygrade... I have no idea 😅
There was a problem hiding this comment.
It seems like it does:
Yes, BenchmarkDotNet collects both simultaneously (well, technically in separate runs to avoid interference, but in the same invocation). When you add
[MemoryDiagnoser], the harness:
- Runs timing measurements normally
- Performs a separate run for memory/GC metrics (so they don't skew latency numbers)
- Combines everything into one unified JSON output per benchmark
In the sample file, there are both:
{
"Method": "Tokenize",
"Parameters": "ExampleFileName=expressions.step",
"Statistics": {
"Mean": 106.338...,
"Median": 105.607...,
"StandardDeviation": 3.223...,
...
},
"Memory": {
"Gen0Collections": 168,
"Gen1Collections": 0,
"Gen2Collections": 0,
"TotalOperations": 4194304,
"BytesAllocatedPerOperation": 672
}
}
With that being the case, we're going to want to make sure we:
- Always collect the wall-clock time measurements
- Optionally collect the memory measurements (if they are present)
Right now, it seems like we are only collecting the memory measurements if they are present (skipping 1.).
There was a problem hiding this comment.
I'm not sure I follow... How is the statistics object relevant for the memory allocations? Isn't this used for the latency measurement? That part should mostly be untouched by my changes (latency is still being collected)
There was a problem hiding this comment.
Great! Then we just need to make sure we are testing that the latency measures are still being collected properly.
That is, this adapter_c_sharp_dot_net_memory test should assert on each expected measure (including Latency).
| ); | ||
| } | ||
|
|
||
| pub mod dotnet { |
There was a problem hiding this comment.
For consistency, lets move this below json and above iai.
ricardoboss
force-pushed
the
issues/699-dotnet-memory-allocations
branch
from
18c15d6 to
315621f
Compare
|
Bencher Report| Branch | issues/699-dotnet-memory-allocations |
| Testbed | ubuntu-22.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result microseconds (µs) (Result Δ%) | Upper Boundary microseconds (µs) (Limit %) |
|---|---|---|---|
| Adapter::Json | 📈 view plot 🚷 view threshold | 3.90 µs(+12.31%)Baseline: 3.48 µs | 4.62 µs (84.44%) |
| Adapter::Magic (JSON) | 📈 view plot 🚷 view threshold | 3.98 µs(+15.17%)Baseline: 3.46 µs | 4.52 µs (88.02%) |
| Adapter::Magic (Rust) | 📈 view plot 🚷 view threshold | 25.39 µs(-1.04%)Baseline: 25.65 µs | 31.05 µs (81.75%) |
| Adapter::Rust | 📈 view plot 🚷 view threshold | 2.91 µs(+2.06%)Baseline: 2.85 µs | 3.34 µs (87.20%) |
| Adapter::RustBench | 📈 view plot 🚷 view threshold | 2.89 µs(+1.60%)Baseline: 2.85 µs | 3.32 µs (87.21%) |
| head_version_insert/batch/10 | 📈 view plot 🚷 view threshold | 109.25 µs(+9.09%)Baseline: 100.15 µs | 120.45 µs (90.70%) |
| head_version_insert/batch/100 | 📈 view plot 🚷 view threshold | 250.60 µs(+5.59%)Baseline: 237.33 µs | 266.98 µs (93.86%) |
| head_version_insert/batch/255 | 📈 view plot 🚷 view threshold | 481.82 µs(+4.47%)Baseline: 461.21 µs | 492.34 µs (97.86%) |
| head_version_insert/batch/50 | 📈 view plot 🚷 view threshold | 170.48 µs(+6.27%)Baseline: 160.42 µs | 182.51 µs (93.41%) |
| threshold_query/join/10 | 📈 view plot 🚷 view threshold | 162.62 µs(+12.60%)Baseline: 144.42 µs | 170.07 µs (95.62%) |
| threshold_query/join/20 | 📈 view plot 🚷 view threshold | 174.69 µs(+10.17%)Baseline: 158.57 µs | 186.03 µs (93.90%) |
| threshold_query/join/5 | 📈 view plot 🚷 view threshold | 150.22 µs(+10.11%)Baseline: 136.42 µs | 159.81 µs (94.00%) |
| threshold_query/join/50 | 📈 view plot 🚷 view threshold | 218.66 µs(+9.34%)Baseline: 199.99 µs | 232.82 µs (93.92%) |
epompeii
left a comment
epompeii
left a comment
There was a problem hiding this comment.
@ricardoboss it is looking good!
You just need to run some final cleanup and you should be good to go.
cargo fmtcargo clippy(to be extra sure you can run./scripts/clippy.shto run exactly what is run in CI)
This reverts commit 4dddf99.
Co-authored-by: Everett Pompeii <epompeii@protonmail.com>
ricardoboss
force-pushed
the
issues/699-dotnet-memory-allocations
branch
from
50468e1 to
3dd5e41
Compare
2 participants
Fixes #699
Adds handling for the
memorykey in BenchmarkDotNet, in case C# benchmarks are run with the[MemoryDiagnoser]attribute.If found, adds a new metric
allocatedmeasured in bytes.