Skip to content

Add a benchmark of the hang-on-test-failure code path #2352

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

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

saethlin
Copy link
Member

This is the code pattern that produces the performance problem in #2273

I figured out what I was stuck on in #2315 (comment). For a while I was just doing let x: &[u8] = &[0u8; 4096]; but that doesn't produce the runtime inside Stack::item_popped that I was looking for, I think because this allocation is never deallocated. But with Vec, I get the profile I'm looking for.

@RalfJung
Copy link
Member

😂 the test runs OOM on 32bit machines.^^

@saethlin
Copy link
Member Author

I'm pretty sure this will pass with size 4096 after #2315 due to the ~3x memory reduction that PR provides. But I also think this benchmark makes a poorer case for the implementation details of that PR with the smaller size.

The irony is cruel.

Also I'm pretty sure the fact that the test fails means that cargo miri test with a failing test will currently OOM any 32-bit system. But slowly.

@RalfJung
Copy link
Member

We can also just block this on landing #2315. Or increase it back to 4096 in that PR.

@saethlin
Copy link
Member Author

I think that making a benchmark slower as part of an optimization PR would be very confusing if someone were to use the benchmarks to evaluate the impact of PRs later.

So for now I think just sharing this up in a PR makes me feel less dishonest about using a new benchmark to justify an implementation choice, and we can land it after #2315, with the larger size.

@RalfJung
Copy link
Member

Can you rebase and increase the size to 4096 again? Does that work now? :D

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2022

📌 Commit 7d9f04f has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 13, 2022

⌛ Testing commit 7d9f04f with merge a45d6ef...

@bors
Copy link
Contributor

bors commented Jul 13, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing a45d6ef to master...

@bors bors merged commit a45d6ef into rust-lang:master Jul 13, 2022
@RalfJung
Copy link
Member

I just noticed we already have bench-cargo-miri/backtraces/src/main.rs. But that seems to be something else?

@saethlin saethlin deleted the new-benchmark branch July 13, 2022 23:05
@saethlin
Copy link
Member Author

Yes. That benchmark exists because I noticed that I'm pretty sure the Debug impl for BacktraceFrame or whatever I'm formatting there has an oddly low hit rate in the Stack cache.

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.

3 participants