-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Crash while running compiler with self profile #99282
Comments
This is unfortunate but somewhat known for bigger crates, there's more than a u32's worth of strings or similar, e.g. rust-lang/measureme#165 or rust-lang/measureme#196 Maybe we should switch to a u64 🤡. There's no wg-self-profile GH team so I'll just cc the team members directly @wesleywiser @michaelwoerister in case they have a preference on how to fix this, and I'd look at doing that. |
I've not fully grasped the code yet, but it looks like there's a constant here that's significantly less than u32::MAX -- 100 million. Is that perhaps the limit we're actually hitting? It seems a little unlikely to me that we're actually seeing u32::MAX unique(?) string IDs in a compilation that will succeed -- presumably, each of those strings is at least several bytes long, which quickly adds up to several dozen gigabytes of memory usage, right? I guess that makes some sense for very slow compilations.... If bumping to u64 isn't a large memory/performance regression, that seems an OK out here, but I'll also suggest that we could consider just giving up on allocating new strings but not panicking? I'm still a little surprised that we run up against a u32::MAX limit... |
I haven't grasped the code either, and don't know yet what this 100 million cutoff between "virtual string ids" and "real string ids" means, or why this addition overflows after adding that value. It could for sure be a different issue than there being so many unique strings. Maybe that 100 million base is somehow added multiple times through a different codepath than the expected checked_sub. I'll try to investigate. |
Ok that makes more sense: IIUC some of the |
Yes, that sounds right. Increasing the size of Any optimizations to allow for a bigger string address space while keeping |
Another way to (temporarily) go at this could be to reduce the size of the strings that rustc records: maybe some of the args' debug output is recorded (and/or some of the Maybe we can make these more compact, while retaining the key information (of course, until another bigger crate then overflows this compact output and we have to go to usizes anyways). It could also make these args more readable for people. Here's an example of such a slightly verbose case, from Amos' recent article: |
IMO we should probably be forcing a pow2 size, and aligning things to the start of a page, to not end up e.g. straddling cache lines randomly and whatnot - though the impact is probably minimal either way. |
I made an attempt at fixing this issue in rust-lang/measureme#216 , and would appreciate feedback! |
v9 format: Increase StringId and Addr size to u64, fixing ICEs during self-profiling This PR introduces a new "v9" format for profdata files, which uses u64s for addressing into files instead of u32s. This avoids ICEs which were possible with large traces, mentioned in the attached issues. Fixes: #214, rust-lang/rust#99282 --- This is my first contribution to this repo, so I've made sure that tests are passing, but I'm interested in ensuring the following work: - [x] I originally encountered this issue from #214 -- I'd love to get advice on "how to rebuild the self-profiling feature using this code" to ensure this change works end-to-end too. EDIT: I did this, it works! No longer seeing ICEs on large profile traces. - [x] I see that there are compatibility tests with v7 and v8 formats (e.g. `can_read_v8_profdata_files`) -- I'd be interested in creating one for v9 too, but I'm not actually that familiar with "how to generate a new `.mm_profdata` file" without the whole toolchain built to help me. I'm interested in fixing this before merging, but would appreciate pointers! EDIT: This is done -- I added a v9 format test after tracing a minimal rust binary created via `cargo new`.
We merged @smklein's fix, extending the StringId size to 64 bits. We'll need to update perf.rlos version of measureme before updating the compiler. |
Update measureme crate to version 11.0.0 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang#99282 Fixes rust-lang#119103
Update measureme crate to version 11.0.0 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang#99282 Fixes rust-lang#119103
…leywiser,lqd Update measureme crate to version 11.0.0 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang#99282 Fixes rust-lang#119103
Update measureme crate to version 11 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang#99282 Fixes rust-lang#119103
Update measureme crate to version 11 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang/rust#99282 Fixes rust-lang/rust#119103
Update measureme crate to version 11 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang/rust#99282 Fixes rust-lang/rust#119103
Update measureme crate to version 11 perf.rlo has been updated to use 11.0.0 already, so it should be able to handle the new file format. r? `@Mark-Simulacrum` Fixes rust-lang/rust#99282 Fixes rust-lang/rust#119103
Code
https://github.com/Ten0/diesel_3223_repro
Meta
I'm trying to debug an issue with extensive compile times in diesel. While running the compiler on an example with RUSTFLAGS="-Z self-profile -Z self-profile-events=default,args" I've got the following ICE, which prompt me to report it here.
rustc --version --verbose
:Backtrace
The text was updated successfully, but these errors were encountered: