Intern predicates by lcnr · Pull Request #72055 · rust-lang/rust

lcnr · 2020-05-09T17:43:41Z

Implements the first step of rust-lang/compiler-team#285

Renames ty::Predicate to ty::PredicateKind, which is now interned.
To ease the transition, ty::Predicate is now a struct containing a reference
to ty::PredicateKind.

r? @ghost

nikomatsakis

Looks good thus far!

src/librustc_middle/ty/mod.rs

nikomatsakis · 2020-05-11T18:06:59Z

Noting down some notes from Zulip:

I actually started with type Predicate<'tcx> = &'tcx PredicateKind<'tcx>, but I reversed course, and I think the current design is correct. Three reasons:

pred.fold_with(&mut folder) resolves to the wrong type if pred: &PredicateKind<'_>, instead of of being Predicate<'tcx>.
If we succeed with the "extract a shared library of types" proposal, then we would want to adopt the general structure that there is a struct Predicate<I> with a kind() method that gives a &PredicateKind (that's the structure chalk is using).
If lets us implement pointer equality when you PartialEq two Predicate instances in a safe fashion.

src/librustc_middle/ty/mod.rs

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

lcnr · 2020-05-11T20:18:18Z

Should be ready for review.

r? @eddyb

bors · 2020-05-13T10:37:29Z

☔ The latest upstream changes (presumably #71451) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2020-05-16T04:10:16Z

☔ The latest upstream changes (presumably #72251) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr · 2020-05-16T07:35:54Z

rebased

rust-highfive · 2020-05-16T07:52:02Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Hosted Agent'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200430.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200430.2/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/1b15d593-1255-42bf-9e88-d1f8c3b432af.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72055/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/72055/merge:refs/remotes/pull/72055/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
    Checking rustc_feature v0.0.0 (/checkout/src/librustc_feature)
    Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
    Checking rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
    Checking rustc_hir v0.0.0 (/checkout/src/librustc_hir)
    Checking rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
    Checking chalk-rust-ir v0.10.0
    Checking rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
    Checking rustc_parse v0.0.0 (/checkout/src/librustc_parse)
    Checking rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
    Checking rustc_trait_selection v0.0.0 (/checkout/src/librustc_trait_selection)
error[E0061]: this function takes 1 argument but 0 arguments were supplied
   --> src/librustc_trait_selection/traits/mod.rs:560:46
    |
560 |         predicate: trait_ref.without_const().to_predicate(),
    |                                              |
    |                                              expected 1 argument

error: aborting due to previous error
error: aborting due to previous error

For more information about this error, try `rustc --explain E0061`.
error: could not compile `rustc_trait_selection`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:04:27
== clock drift check ==
  local time: Sat May 16 07:43:28 UTC 2020
  local time: Sat May 16 07:43:28 UTC 2020
  network time: Sat, 16 May 2020 07:43:28 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72055/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72055/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3402) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

nikomatsakis · 2020-05-18T20:00:19Z

@bors r+

I'm going to snipe this review from @eddyb since they seem busy and, while I did author the first commit or two here, they're also fairly rote, and the majority of the work came afterwards anyhow. (And it follows more-or-less the rust-lang/compiler-team#285 MCP, although the FCP there has not fully expired.)

bors · 2020-05-18T20:00:20Z

📌 Commit 99483282a6da51f71bdc663bf26fe416bbd02464 has been approved by nikomatsakis

bors · 2020-05-18T20:00:21Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

lcnr · 2020-05-20T13:47:03Z

This is once again ready 😅

nikomatsakis · 2020-05-20T22:25:16Z

@bors r+

bors · 2020-05-20T22:25:17Z

📌 Commit 3dd830b has been approved by nikomatsakis

nikomatsakis · 2020-05-20T22:25:26Z

@bors delegate+

bors · 2020-05-20T22:25:27Z

✌️ @lcnr can now approve this pull request

@ghost

Intern predicates Implements the first step of rust-lang/compiler-team#285 Renames `ty::Predicate` to `ty::PredicateKind`, which is now interned. To ease the transition, `ty::Predicate` is now a struct containing a reference to `ty::PredicateKind`. r? @ghost

@ghost

Rollup of 7 pull requests Successful merges: - rust-lang#72055 (Intern predicates) - rust-lang#72149 (Don't `type_of` on trait assoc ty without default) - rust-lang#72347 (Make intra-link resolve links for both trait and impl items) - rust-lang#72350 (Improve documentation of `slice::from_raw_parts`) - rust-lang#72382 (Show default values for debug-assertions & debug-assertions-std) - rust-lang#72421 (Fix anchor display when hovering impl) - rust-lang#72425 (fix discriminant_value sign extension) Failed merges: r? @ghost

ecstatic-morse · 2020-05-23T16:33:05Z

@lcnr The rollup containing this PR caused a large perf regression in trait solving. Was this expected?

lcnr · 2020-05-23T16:43:54Z

Not by this much 😅 There is one problem which I should be able to get a PR ready today, but a small regression is probably expected...

mati865 · 2020-05-23T17:09:47Z

@lcnr @nikomatsakis please use bors rollup=never when PR is expected to have performance implications (either good or bad).

nnethercote · 2020-05-25T23:18:42Z

Note that this was also a max-rss regression. max-rss measurements are noisy, but the change was big enough for the regression to be clear on the graphs.

nnethercote · 2020-05-25T23:19:36Z

@lcnr: please do a perf CI run before landing if a PR is expected/suspected to affect performance.

There is one problem which I should be able to get a PR ready today

What is the status of this?

lcnr · 2020-05-26T07:27:02Z

It ended up being part of #72494 which fixes about 1/3 of of this regression if I understand it correctly.

nikomatsakis · 2020-05-27T14:08:29Z

(I didn't really expect there to be much perf regression, but it would've been prudent to test.)

@nikomatsakis

Pass more `Copy` types by value. There are a lot of locations where we pass `&T where T: Copy` by reference, which should both be slightly less performant and less readable IMO. This PR currently consists of three fairly self contained commits: - passes `ty::Predicate` by value and stops depending on `AsRef<ty::Predicate>`. - changes `<&List<_>>::into_iter` to iterate over the elements by value. This would break `List`s of non copy types. But as the only list constructor requires `T` to be copy anyways, I think the improved readability is worth this potential future restriction. - passes `mir::PlaceElem` by value. Mir currently has quite a few copy types which are passed by reference, e.g. `Local`. As I don't have a lot of experience working with MIR, I mostly did this to get some feedback from people who use MIR more frequently - tries to reuse `ty::Predicate` in case it did not change in some places, which should hopefully fix the regression caused by rust-lang#72055 r? @nikomatsakis for the first commit, which continues the work of rust-lang#72055 and makes adding `PredicateKind::ForAll` slightly more pleasant. Feel free to reassign though

lcnr marked this pull request as draft May 9, 2020 17:44

nikomatsakis reviewed May 11, 2020

View reviewed changes

src/librustc_middle/ty/mod.rs Outdated Show resolved Hide resolved

nikomatsakis approved these changes May 11, 2020

View reviewed changes

lcnr marked this pull request as ready for review May 11, 2020 17:07

lcnr force-pushed the predicate-kind branch from 1072d39 to f628ff4 Compare May 11, 2020 18:05

lcnr force-pushed the predicate-kind branch 2 times, most recently from 4a0e5af to 6df0cd6 Compare May 11, 2020 19:36

lcnr commented May 11, 2020

View reviewed changes

src/librustc_middle/ty/mod.rs Outdated Show resolved Hide resolved

lcnr force-pushed the predicate-kind branch from 6df0cd6 to 99e3fdf Compare May 11, 2020 20:10

lcnr commented May 11, 2020

View reviewed changes

src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved

lcnr assigned eddyb and unassigned eddyb May 11, 2020

rust-highfive assigned eddyb May 11, 2020

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 13, 2020

lcnr force-pushed the predicate-kind branch from 99e3fdf to 8b4252b Compare May 13, 2020 11:21

lcnr force-pushed the predicate-kind branch from 8b4252b to c88a9da Compare May 16, 2020 07:35

lcnr force-pushed the predicate-kind branch from c88a9da to 9948328 Compare May 16, 2020 07:55

lcnr added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 16, 2020

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 May 18, 2020

lcnr added 3 commits May 20, 2020 15:44

intern PredicateKind

57746f9

change Predicate::kind to return a reference

6544d7b

ptr eq for Predicate

3dd830b

lcnr force-pushed the predicate-kind branch from 136ac94 to 3dd830b Compare May 20, 2020 13:46

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2020

RalfJung mentioned this pull request May 21, 2020

Rollup of 7 pull requests #72430

Closed

RalfJung mentioned this pull request May 21, 2020

Rollup of 7 pull requests #72433

Merged

bors merged commit 22438fc into rust-lang:master May 22, 2020

lcnr deleted the predicate-kind branch May 22, 2020 07:52

lcnr mentioned this pull request May 23, 2020

Pass more Copy types by value. #72494

Merged

LeSeulArtichaut mentioned this pull request May 23, 2020

Update outdated reference to ty::Predicate rust-lang/rustc-dev-guide#711

Merged

JohnTitor mentioned this pull request May 25, 2020

Rustup to the latest nightly rust-lang/rust-semverver#124

Merged

Uh oh!

Conversation

lcnr commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikomatsakis commented May 11, 2020

Uh oh!

Uh oh!

Uh oh!

lcnr commented May 11, 2020

Uh oh!

bors commented May 13, 2020

Uh oh!

bors commented May 16, 2020

Uh oh!

lcnr commented May 16, 2020

Uh oh!

rust-highfive commented May 16, 2020

Uh oh!

nikomatsakis commented May 18, 2020

Uh oh!

bors commented May 18, 2020

Uh oh!

bors commented May 18, 2020

Uh oh!

lcnr commented May 20, 2020

Uh oh!

nikomatsakis commented May 20, 2020

Uh oh!

bors commented May 20, 2020

Uh oh!

nikomatsakis commented May 20, 2020

Uh oh!

bors commented May 20, 2020

Uh oh!

ecstatic-morse commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr commented May 23, 2020

Uh oh!

mati865 commented May 23, 2020

Uh oh!

nnethercote commented May 25, 2020

Uh oh!

nnethercote commented May 25, 2020

Uh oh!

lcnr commented May 26, 2020

Uh oh!

nikomatsakis commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lcnr commented May 9, 2020 •

edited

Loading

ecstatic-morse commented May 23, 2020 •

edited

Loading