Started adding support for dhat cost summary #71
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for contributing! I basically like what you've implemented already. However, some problems I currently see are: Running dhat in instead of The I think it would be great to use the PID and Parent PID in the summary. Currently the output is for example: but I would expect something like that: The pids are only printed if there are multiple processes and log files. But, the pids don't need to be compared or at least don't need a comparison printed. If you haven't used it already, there is a test case for multiple processes which you can run for example with If we compare dhat stats, I think it's better to not print the should be: Just a small hint: I've seen some formatting issues in the diff. We are using |
|
I implemented most of your suggestions, I wasn't sure if you wanted the PID comparison only for that, but I implemented it for all tools. I removed the |
Did you run the
fn bad_memory() {
for _ in 0..100 {
let left = Rc::new(RefCell::new(Left(None)));
let right = Rc::new(Right(Some(Rc::clone(&left))));
left.borrow_mut().0 = Some(Rc::clone(&right));
}
}
A problem I have noticed when running the above. When running the benchmark the first time, there was no output for dhat besides the When running the benchmark the second time, the fields were printed twice:
I think we should concentrate on I think the formatting and printing of That's all I've discovered at the moment. |
|
Sorry I should have been more clear about the problem with
#[library_benchmark]
fn bench_bubble_sort_allocate() -> i32 {
black_box(bubble_sort_allocate(black_box(4000), black_box(2000)))
}to: #[library_benchmark]
fn bench_bubble_sort_allocate() -> i32 {
black_box(bubble_sort_allocate(black_box(8000), black_box(2000)))
}
The issue is that in both cases the The issue is caused by the fact that dhat generates its cost summary by comparing |
|
oh yeah. You're right concerning the different runs. If you haven't discovered it already, there's a method I hope that helps. Keep asking if there is something else or you need any further help. |
|
Sorry, some linting errors ahead. We're running the |
|
The schema file for # In the root directory of `iai-callgrind`. This produces the schema file `summary.schema.json`
cargo run --package iai-callgrind-runner --release --features schema --bin schema-gen
prettier --write summary.schema.json
mv summary.schema.json iai-callgrind-runner/schemas/summary.v1.schema.json |
|
The test run failed because there is a file expected with the name |
|
This change made it easier to make the |
|
Ok, I see. I think using the |
gamma0987
requested changes
Jan 17, 2024
|
Calculating the |
gamma0987
requested changes
Jan 17, 2024
|
There's something else going wrong when I run the benchmark the first time: I'm missing two things: The DHAT summary output and the |
|
I just noticed there's more missing: Here's the output from the main branch when I run it the first time: I actually expected that only the DHAT output differs and shows a summary output. However, I guess the root of the problem is the same. |
|
We're getting closer :) However, the second, third, ... run doesn't compare DHAT to the previous run |
gamma0987
requested changes
Jan 18, 2024
|
Could you please don't resolve the reviews. Thanks. |
|
|
Would it be world adding a pre-commit script? eg: |
No problem.
Would be great. The only problem I see is that last However, don't make this part of this pr. I'm going through the changes a last time until tomorrow or so, but as far as I can see right now it's working as it should. Thanks for your great work on this pr, so far. |
gamma0987
requested changes
Jan 19, 2024
gamma0987
reviewed
Jan 24, 2024
|
Great! As a last request, please squash the commits from ef53ef9 onwards. |
dewert99
force-pushed
the
dhat-cost-summary2
branch
from
62c8cfb to
e7565b0
Compare
|
Looks like, something went wrong during the squash |
dewert99
force-pushed
the
dhat-cost-summary2
branch
from
e7565b0 to
d0e3cfa
Compare
|
Sorry, I tried squashing again, does this seem better? |
|
yeah looks better |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I thought it could be useful to allow tools other than Callgrind to produce cost summaries. I implemented a version that mostly works but doesn't properly handle
--save-baseline(in this case it always reports no changes). I think that supporting--save-baselinewould require even deeper changes so I decided to see if you would be interested in this and if you had any suggestions before going further.