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

Rollup of 9 pull requests #114481

Merged
merged 20 commits into from
Aug 4, 2023
Merged

Rollup of 9 pull requests #114481

merged 20 commits into from
Aug 4, 2023

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

ttsugriy and others added 20 commits August 1, 2023 16:57
Comparing vectors of string parts yields the same result but avoids
unnecessary `join` and potential allocation for resulting `String`.
This code is cold so it's unlikely to have any measurable impact, but
considering but since it's also simpler, why not? :)
Indexing is similar to method calls in having an arbitrary
left-hand-side and then something on the right, which is the main part
of the expression. Method calls already have a span for that right part,
but indexing does not. This means that long method chains that use
indexing have really bad spans, especially when the indexing panics and
that span in coverted into a panic location.

This does the same thing as method calls for the AST and HIR, storing an
extra span which is then put into the `fn_span` field in THIR.
When suggesting to clone a reference-counted value, be less uncertain.
…-2, r=cjgillot

Fix wrong span for trait selection failure error reporting

Fixes rust-lang#113447
[rustc_span][perf] Remove unnecessary string joins and allocs.

Comparing vectors of string parts yields the same result but avoids unnecessary `join` and potential allocation for resulting `String`. This code is cold so it's unlikely to have any measurable impact, but considering but since it's also simpler, why not? :)
bump parking_lot to 0.12

Bumps parking_lot to 0.12, replaces few explicit uses of parking_lot with rustc_data_structures::sync ones.

<strike>cc `@oli-obk` this touches recent https://github.com/rust-lang/rust/pull/114283</strike>
cc `@SparrowLii` i've checked that this builds with parallel-compiler

measureme's bump rust-lang/measureme#209
https://github.com/rust-lang/rust/blob/fcf3006e0133365ecd26894689c086387edcbecb/compiler/rustc_data_structures/src/sync.rs#L18-L34
Improve spans for indexing expressions

fixes rust-lang#114388

Indexing is similar to method calls in having an arbitrary left-hand-side and then something on the right, which is the main part of the expression. Method calls already have a span for that right part, but indexing does not. This means that long method chains that use indexing have really bad spans, especially when the indexing panics and that span in coverted into a panic location.

This does the same thing as method calls for the AST and HIR, storing an extra span which is then put into the `fn_span` field in THIR.

r? compiler-errors
…mpiler-errors

Fix ICE failed to get layout for ReferencesError

Fixes rust-lang#114435

r? `@compiler-errors`
interpret: add mplace_to_ref helper method
Account for `Rc` and `Arc` when suggesting to clone

When suggesting to clone a reference-counted value, be less uncertain.
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Aug 4, 2023
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=9

@bors
Copy link
Contributor

bors commented Aug 4, 2023

📌 Commit 35b2713 has been approved by matthiaskrgr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2023
@bors
Copy link
Contributor

bors commented Aug 4, 2023

⌛ Testing commit 35b2713 with merge e4c1446...

@bors
Copy link
Contributor

bors commented Aug 4, 2023

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing e4c1446 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2023
@bors bors merged commit e4c1446 into rust-lang:master Aug 4, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 4, 2023
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#113945 Fix wrong span for trait selection failure error reporting 8ca4ade576034889aae70fbd1735160b9d13bf9b (link)
#114351 [rustc_span][perf] Remove unnecessary string joins and allo… 6fd828aeebb65dffb6065227f003c113de72e1a8 (link)
#114418 bump parking_lot to 0.12 06f066e399900ce7c83b2b8bb638db48811b5607 (link)
#114434 Improve spans for indexing expressions 45523252d52090c4c32cf7b51c6073da272f0af5 (link)
#114450 Fix ICE failed to get layout for ReferencesError eb8e7a307b2fb51c3eb500cfec92896eae5a9cc1 (link)
#114461 Fix unwrap on None 82e1bc18addd4a70927f8bc9b86281db4a0d2e75 (link)
#114462 interpret: add mplace_to_ref helper method bb0d991d758d026ca66896d9a171c9696e836cee (link)
#114472 Reword confusable_idents lint fc1423acf5321d6dabb0948cfb6abfbfc6ba244d (link)
#114477 Account for Rc and Arc when suggesting to clone 7eddd3c2a58e74323225939404a7b254fe8d2776 (link)

previous master: fe896efa97

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e4c1446): 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.8% [0.4%, 1.5%] 80
Regressions ❌
(secondary)
0.7% [0.2%, 1.2%] 25
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.4%, 1.5%] 80

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)
3.1% [3.0%, 3.4%] 3
Regressions ❌
(secondary)
2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.1% [3.0%, 3.4%] 3

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)
1.7% [1.7%, 1.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.7% [1.7%, 1.7%] 1

Binary size

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.0% [0.0%, 0.1%] 15
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [0.0%, 0.1%] 15

Bootstrap: 650.185s -> 650.779s (0.09%)

@rustbot rustbot added the perf-regression Performance regression. label Aug 5, 2023
@lqd
Copy link
Member

lqd commented Aug 5, 2023

There is a trend up in the most recent merges, but these incremental compilation numbers are weird:

  • half of them went down in the next merge that only changed a lint.
  • half of those then went back up in the next merge, which only changed risc-v targets.
  • the bitmaps check incr-unchanged regression looks like the dep graph marking got slower, but no PR stands out as possibly modifying this here. And the dep graph, query cache, and crate metadata didn't seem to change shape or sizes?
15,710,220  ???:<rustc_query_system::dep_graph::graph::DepGraphData<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
-3,253,495  ???:<rustc_middle::metadata::ModChild as rustc_serialize::serialize::Decodable<rustc_metadata::rmeta::decoder::DecodeContext>>::decode
 3,167,659  ???:<rustc_query_system::dep_graph::graph::CurrentDepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::intern_node
 2,300,834  ???:<rustc_span::symbol::Ident as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
-1,791,094  ???:<rustc_middle::ty::Ty as rustc_serialize::serialize::Encodable<rustc_middle::query::on_disk_cache::CacheEncoder>>::encode
-1,704,182  ???:<rustc_span::span_encoding::Span as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
 1,602,558  ???:<rustc_span::symbol::Symbol as rustc_serialize::serialize::Decodable<rustc_metadata::rmeta::decoder::DecodeContext>>::decode
-1,602,468  ???:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, rustc_middle::query::erase::Erased<[u8; 8]>>, false, false, false>, rustc_query_impl::plumbing::QueryCtxt, true>
 1,510,354  ???:rustc_middle::ty::codec::encode_with_shorthand::<rustc_middle::query::on_disk_cache::CacheEncoder, rustc_middle::ty::Ty, <rustc_middle::query::on_disk_cache::CacheEncoder as rustc_type_ir::codec::TyEncoder>::type_shorthands>
  • the mir interpret PR could have been related to the ctfe-stress changes, but since it's also an incr comp "regression", it looks similar. And nothing seems to change the iterators of fluent diagnostics messages in this rollup that could make those slower?
  809,462  ???:<rustc_query_system::dep_graph::graph::DepGraphData<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
  760,194  ???:<alloc::vec::Vec<fluent_syntax::ast::PatternElement<&str>> as alloc::vec::spec_from_iter::SpecFromIter<fluent_syntax::ast::PatternElement<&str>, core::iter::adapters::map::Map<core::iter::adapters::enumerate::Enumerate<core::iter::adapters::take::Take<alloc::vec::into_iter::IntoIter<fluent_syntax::parser::pattern::PatternElementPlaceholders<&str>>>>, <fluent_syntax::parser::core::Parser<&str>>::get_pattern::{closure#0}>>>::from_iter
 -573,567  ???:<fluent_syntax::parser::core::Parser<&str>>::get_pattern
  217,426  ???:<rustc_query_system::dep_graph::dep_node::DepNode<rustc_middle::dep_graph::dep_node::DepKind> as rustc_middle::dep_graph::dep_node::DepNodeExt>::extract_def_id
 -201,135  ???:<rustc_middle::ty::Ty as rustc_serialize::serialize::Encodable<rustc_middle::query::on_disk_cache::CacheEncoder>>::encode

Looks very noisy. I won't mark this triaged and we'll see what weekly triage thinks of this. By then more PRs will have been merged, and we can see where things settled.

@Mark-Simulacrum
Copy link
Member

Interesting. Yeah, the regressions here seem to be entirely limited to incremental loading(?) -- incr-unchanged and incr-patched are the only scenarios that show regressions. Poking at just one benchmark it does seem like we are seeing a bunch of noise, though I think mostly not immediately after this PR:

image

Overall though things are getting better for that benchmark:

image

Let's maybe wait some more time, the noise seems to be coming in and out. Not clear what PR within the rollup here may have caused this kind of noise... maybe the parking_lot change?

flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 11, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#113945 (Fix wrong span for trait selection failure error reporting)
 - rust-lang#114351 ([rustc_span][perf] Remove unnecessary string joins and allocs.)
 - rust-lang#114418 (bump parking_lot to 0.12)
 - rust-lang#114434 (Improve spans for indexing expressions)
 - rust-lang#114450 (Fix ICE failed to get layout for ReferencesError)
 - rust-lang#114461 (Fix unwrap on None)
 - rust-lang#114462 (interpret: add mplace_to_ref helper method)
 - rust-lang#114472 (Reword `confusable_idents` lint)
 - rust-lang#114477 (Account for `Rc` and `Arc` when suggesting to clone)

r? `@ghost`
`@rustbot` modify labels: rollup
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Oct 23, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#113945 (Fix wrong span for trait selection failure error reporting)
 - rust-lang#114351 ([rustc_span][perf] Remove unnecessary string joins and allocs.)
 - rust-lang#114418 (bump parking_lot to 0.12)
 - rust-lang#114434 (Improve spans for indexing expressions)
 - rust-lang#114450 (Fix ICE failed to get layout for ReferencesError)
 - rust-lang#114461 (Fix unwrap on None)
 - rust-lang#114462 (interpret: add mplace_to_ref helper method)
 - rust-lang#114472 (Reword `confusable_idents` lint)
 - rust-lang#114477 (Account for `Rc` and `Arc` when suggesting to clone)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr deleted the rollup-58pczpl branch March 16, 2024 18:18
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#113945 (Fix wrong span for trait selection failure error reporting)
 - rust-lang#114351 ([rustc_span][perf] Remove unnecessary string joins and allocs.)
 - rust-lang#114418 (bump parking_lot to 0.12)
 - rust-lang#114434 (Improve spans for indexing expressions)
 - rust-lang#114450 (Fix ICE failed to get layout for ReferencesError)
 - rust-lang#114461 (Fix unwrap on None)
 - rust-lang#114462 (interpret: add mplace_to_ref helper method)
 - rust-lang#114472 (Reword `confusable_idents` lint)
 - rust-lang#114477 (Account for `Rc` and `Arc` when suggesting to clone)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.