privacy: Assert that compared visibilities are (usually) ordered#155257
privacy: Assert that compared visibilities are (usually) ordered#155257rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
|
r? @adwinwhite rustbot has assigned @adwinwhite. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
privacy: Assert that compared visibilities are (usually) ordered
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
2b33358 to
8528576
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
privacy: Assert that compared visibilities are (usually) ordered
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0c52563): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.936s -> 490.568s (0.13%) |
|
One small regression on libc (which is a sort of stress test for large number of imports and other items). |
|
This could also be potentially blocked on #155213 which addresses on of the FIXMEs here. |
|
@petrochenkov Sorry but perhaps you can manually pick a reviewer who's familiar with this part? I'm not sure that reroll can get to the right person. |
#155608 has a way to avoid comparisons in both directions if this regression shows up again, so no need to switch to debug-only checking. |
|
I analyzed one of the FIXMEs, it will only be unordered in erroneous cases after some fixes, so we'll need a |
Also use `greater_than` instead of `is_at_least` for comparing visibilities, which we can do because visibilities are asserted to be ordered now.
7408ab0 to
714df2b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
(The CI failure is spurious.) |
|
I guess unordered comparison doesn't make sense anyway so it's intended and fine to err in those cases. @bors r+ |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing fb76025 (parent) -> 9838411 (this PR) Test differencesShow 25 test diffs25 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 9838411cb723b60dc62b1625751075c4d933b992 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (9838411): comparison URL. Overall result: ❌ regressions - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 487.131s -> 486.61s (-0.11%) |
View all comments
And make "greater than" (
>) the new primary operation for comparing visibilities instead of "is at least" (>=).