Disable visible path calculation for PrettyPrinter in Ok path of compiler

[In-line]

No description provided.

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[In-line]

Please, kindly schedule a perf run

[Mark-Simulacrum]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit f6fc9789d728bb1df6a90e50ebfcfa18aa670224 with merge 4bf6e5c32905d0f765eab7c792a5e1a72f9afee1...

[bors]

☀️ Try build successful - checks-actions
Build commit: 4bf6e5c32905d0f765eab7c792a5e1a72f9afee1 (4bf6e5c32905d0f765eab7c792a5e1a72f9afee1)

[rust-timer]

Queued 4bf6e5c32905d0f765eab7c792a5e1a72f9afee1 with parent db1fb85, future comparison URL.

[In-line]

I forgot to commit main check, sorry! Please cancel scheduled perf run and redo it 😄

[rust-timer]

Finished benchmarking commit (4bf6e5c32905d0f765eab7c792a5e1a72f9afee1): comparison url.

Summary: This change led to small relevant mixed results 🤷 in compiler performance.

• Small improvement in instruction counts (up to -0.5% on full builds of coercions)
• Small regression in instruction counts (up to 0.4% on incr-unchanged builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[jyn514]

@bors try @rust-timer queue

#89120 (comment)

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 9da27f0 with merge 635ba4d6ff55c13d3f40dd9b82d773d172377606...

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

☀️ Try build successful - checks-actions
Build commit: 635ba4d6ff55c13d3f40dd9b82d773d172377606 (635ba4d6ff55c13d3f40dd9b82d773d172377606)

[rust-timer]

Queued 635ba4d6ff55c13d3f40dd9b82d773d172377606 with parent 60e70cc, future comparison URL.

[rust-timer]

Finished benchmarking commit (635ba4d6ff55c13d3f40dd9b82d773d172377606): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

• Very large improvement in instruction counts (up to -23.2% on incr-full builds of await-call-tree)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

[In-line]

There are multiple benchmarks in rustc-perf, which generate a lot of warnings. Even a single warning which will print the visible path of type will invoke visible_parent_map.

This explains a lot of neutral results.

I think we have a few questions now.

1. Is regression with uninhabited type panic acceptable?
2. Is regression with query description acceptable?
   1. If no, will converting description() to be calculated lazily help?

For more information on impact to user, you can check out changed tests output.

[jyn514]

Wow, this is a really amazing perf improvement! Definitely don't worry about the one or two regressions, this is really good :)

My only concern is the diagnostic regressions - I'm worried it will be confusing to get errors about items that aren't publicly visible. Maybe we could change those error paths to call try_print_visible_def_path explicitly?

[jyn514]

Is regression with query description acceptable?

This seems fine to me - it should only be shown in ICE messages and logging anyway.

[Mark-Simulacrum]

The query description is shown in cycle errors (as can be observed with the test result changes). I think it is unfortunate to regress there. Could we check performance impact without that change? I'd guess that it is the primary contributor here, but would be happy to be surprised.

[In-line]

Performance improvement comes from removal of `visible_parent_map`. Even one `def_path_str` call will execute this query. I think the solution should be to calculate description lazily. -------- Original Message --------

…

On Sep 21, 2021, 19:12, Mark Rousskov wrote: The query description is shown in cycle errors (as can be observed with the test result changes). I think it is unfortunate to regress there. Could we check performance impact without that change? I'd guess that it is the primary contributor here, but would be happy to be surprised. — You are receiving this because you authored the thread. Reply to this email directly, [view it on GitHub](#89120 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AETPTKHPBRUIWFUZC7P5FKLUDCOEXANCNFSM5EMKN64A). Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).

[estebank]

My only concern is the diagnostic regressions - I'm worried it will be confusing to get errors about items that aren't publicly visible. Maybe we could change those error paths to call try_print_visible_def_path explicitly?

I think those regressions are unfortunate, but not terrible. We've had much worse diagnostic regressions in the past. As long as we have a plan, or at least a ticket to address these, I'm ok with taking that hit. (My gut tells me that cycle errors aren't as common, and if a newcomer hits them they type's paths will not be in the top 3 things that confuse them about these.)

[estebank]

@bors r+

[bors]

📌 Commit 9da27f0 has been approved by estebank

[bors]

⌛ Testing commit 9da27f0 with merge 197fc85...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 197fc85 to master...

[rust-timer]

Finished benchmarking commit (197fc85): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

• Very large improvement in instruction counts (up to -23.1% on incr-full builds of await-call-tree)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression