-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[compiletest] Ignore known paths when abbreviating output #96551
[compiletest] Ignore known paths when abbreviating output #96551
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
802d025
to
6ad414f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6ad414f
to
47d8ba9
Compare
I'm OK with approving this with the nit fixed. Still feels a little unfortunate/off to me, but I'm not seeing massively better options, particularly if there's a soft goal of trying to prevent 'excessively large' outputs from rustc. (Not that several hundred kilobytes of JSON isn't already quite big for the ~1 KB input we typically have...). |
Addressed the last suggestion you wrote. When fixing it, the amount of length manipulations we're doing started worrying me without tests, so I added them in another commit. Those tests also uncovered an existing bug (if the first |
out.extend(&data, &[]); | ||
|
||
let mut expected = vec![b'.'; HEAD_LEN]; | ||
expected.extend_from_slice(b"\n\n<<<<<< SKIPPED 16 BYTES >>>>>>\n\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think we should make the read2_abbreviated return an error or panic instead of inserting this skipped message -- that typically leads to a JSON de-serialization failure and a pretty opaque one at that, which I think isn't too helpful -- we can probably say something more useful like "captured X bytes, which is above the limit in compiletest. See abbreviated output: {output}".
(Can be out of scope for this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's do that in a separate PR though.
|
This comment has been minimized.
This comment has been minimized.
.count(); | ||
*excluded_len += matches as isize | ||
* (EXCLUDED_PLACEHOLDER_LEN - pattern_bytes.len() as isize); | ||
*filtered_len -= matches * path_bytes.len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to run into some problems if we have overlapping matches (e.g., one of the patterns is /home/mark and the other is /home/mark/rust, then we'll subtract out twice).
I think src_base and build_base won't do that in practice so we're OK for now, but ideally we'd either (a) assert that it doesn't happen or (b) somehow fix it. I'm not quite sure what the best approach to fixing it would be -- trying to detect this kind of overlap seems annoying.
r=me if you don't want to deal with this additional problem, but maybe add a comment about it to where we define paths so we think about it. |
📌 Commit 8ea9598 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (6609c67): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
To prevent out of memory conditions, compiletest limits the amount of output a test can generate, abbreviating it if the test emits more than a threshold. While the behavior is desirable, it also causes some issues (like #96229, #94322 and #92211).
The latest one happened recently, when the
src/test/ui/numeric/numeric-cast.rs
test started to fail on systems where the path of the rust-lang/rust checkout is too long. This includes my own development machine and LLVM's CI. Rust's CI uses a pretty short directory name for the checkout, which hides these sort of problems until someone runs the test suite on their own computer.When developing the fix I tried to find the most targeted fix that would prevent this class of failures from happening in the future, deferring the decision on if/how to redesign abbreviation to a later date. The solution I came up with was to ignore known base paths when calculating whether the output exceeds the abbreviation threshold, which removes this kind of nondeterminism.
This PR is best reviewed commit-by-commit.