-
Notifications
You must be signed in to change notification settings - Fork 384
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
Conversation
😂 the test runs OOM on 32bit machines.^^ |
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 |
We can also just block this on landing #2315. Or increase it back to 4096 in that PR. |
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. |
Can you rebase and increase the size to 4096 again? Does that work now? :D |
@bors r+ |
☀️ Test successful - checks-actions |
I just noticed we already have bench-cargo-miri/backtraces/src/main.rs. But that seems to be something else? |
Yes. That benchmark exists because I noticed that I'm pretty sure the |
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 insideStack::item_popped
that I was looking for, I think because this allocation is never deallocated. But withVec
, I get the profile I'm looking for.