Skip to content
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

Heuristically undo path prefix mappings. #106853

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Conversation

TimNN
Copy link
Contributor

@TimNN TimNN commented Jan 14, 2023

Because the compiler produces better diagnostics if it can find the source of (potentially remapped) dependencies.

The new test fails without the other changes in this PR. Let me know if you have better suggestions for the test directory. I moved the existing remapping test to be in the same location as the new one.

Some more context: I'm exploring running UI tests with remapped paths by default in #105924 and this was one of the issues discovered.

This may also be useful in the context of rust-lang/rfcs#3127 ("New rustc and Cargo options to allow path sanitisation by default").

Because the compiler produces better diagnostics if it can find the
source of (potentially remapped) dependencies.
@TimNN
Copy link
Contributor Author

TimNN commented Jan 14, 2023

Oh, I just remembered that I should probably run this through the Windows CI to ensure there are no Windows specific path handling issues.

Feel free to review already but please hold off on approving while I temporarily modify the CI config.

Also: r? compiler

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 14, 2023
@TimNN
Copy link
Contributor Author

TimNN commented Jan 14, 2023

Windows CI looks good, I've removed the CI changes again.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2023

How will this affect

// Hide libstd sources from ui tests to make sure we generate the stderr
// output that users will see.
// Without this, we may be producing good diagnostics in-tree but users
// will not see half the information.
rustc.arg("-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX");
rustc.arg("-Ztranslate-remapped-path-to-local-path=no");
if applied to the entire test suite? We do want to hide libstd/libcore sources from ui tests, so that we actually test the diagnostics shown to users, instead of only diagnostics shown to users who installed the rustc-src rustup component

@TimNN
Copy link
Contributor Author

TimNN commented Jan 16, 2023

I don't believe the flags will be affected at all. AFAICT neither flag affects the contents of the FilePathMappings and thus their behavior won't be influenced by this patch at all.

Both flags seem to operate on a lower level (fiddling with the file paths when decoding metadata).

@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2023

Thanks, that makes sense

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2023

📌 Commit 869df76 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 16, 2023
@bors
Copy link
Contributor

bors commented Jan 16, 2023

⌛ Testing commit 869df76 with merge 4817259...

@bors
Copy link
Contributor

bors commented Jan 16, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 4817259 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 16, 2023
@bors bors merged commit 4817259 into rust-lang:master Jan 16, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 16, 2023
@TimNN TimNN deleted the undo-remap branch January 16, 2023 18:03
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4817259): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.7% [0.7%, 0.8%] 3
Regressions ❌
(secondary)
0.4% [0.3%, 0.6%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.7% [0.7%, 0.8%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
6.8% [6.8%, 6.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 2
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [1.5%, 4.5%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Jan 16, 2023
@TimNN
Copy link
Contributor Author

TimNN commented Jan 17, 2023

Is there any straightforward way to see where the slowdown is coming from? I clicked through some of the pages in the report and the self profiles weren't really helpful.

Is this significant enough to warrant some kind of follow up?

@rylev
Copy link
Member

rylev commented Jan 18, 2023

I could certainly see going either way on whether this is significant enough to look into - definitely not worth a huge investigation but might be worth some small peek.

Looking at the callgrind-diff output for the primary regression, I see FromGenerator<<rustc_metadata::creader::CrateMetadataRef>::get_module_children with the biggest increase in all regressions, but I don't see how this code would change how many times we're calling into that function.

@pnkfelix
Copy link
Member

From the graph, I think this is just noise

@rustbot label: perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants