Reduce the number of `RefCell`s in `InferCtxt`. #68694

nnethercote · 2020-01-31T08:25:32Z

InferCtxt contains six structures within RefCells. Every time we
create and dispose of (commit or rollback) a snapshot we have to
borrow_mut each one of them.

This commit moves the six structures under a single RefCell, which
gives significant speed-ups by reducing the number of borrow_mut
calls. To avoid runtime errors I had to reduce the lifetimes of dynamic
borrows in a couple of places.

r? @varkor

nnethercote · 2020-01-31T08:27:20Z

Some local results (instruction counts):

wf-projection-stress-65510-che...
        avg: -3.6%      min: -5.5%      max: -0.0%
deep-vector-check
        avg: -1.4%      min: -2.0%      max: -0.0%
packed-simd-check
        avg: -1.2%      min: -1.9%      max: -0.4%
tuple-stress-check
        avg: -1.1%      min: -1.8%      max: -0.0%
futures-check
        avg: -0.7%      min: -1.5%      max: -0.0%
ucd-check
        avg: -0.8%      min: -1.4%      max: 0.0%
coercions-check
        avg: -0.7%?     min: -1.3%?     max: 0.0%?
serde-check
        avg: -0.7%      min: -1.2%      max: 0.0%
hyper-2-check
        avg: -0.6%      min: -1.0%      max: -0.0%
piston-image-check
        avg: -0.5%      min: -1.0%      max: 0.0%
tokio-webpush-simple-check
        avg: -0.5%      min: -0.9%      max: -0.0%
regression-31157-check
        avg: -0.4%      min: -0.9%      max: -0.0%
html5ever-check
        avg: -0.4%      min: -0.8%      max: 0.0%
wg-grammar-check
        avg: -0.5%      min: -0.8%      max: 0.0%
regex-check
        avg: -0.2%      min: -0.7%      max: 0.0%
cargo-check
        avg: -0.3%      min: -0.7%      max: 0.0%
webrender-check
        avg: -0.3%      min: -0.7%      max: 0.0% 
ripgrep-check
        avg: -0.3%      min: -0.7%      max: 0.0%
webrender-wrench-check
        avg: -0.3%      min: -0.7%      max: 0.0%
script-servo-check
        avg: -0.2%      min: -0.6%      max: 0.1%
cranelift-codegen-check
        avg: -0.4%      min: -0.6%      max: 0.0%
clap-rs-check
        avg: -0.3%      min: -0.6%      max: 0.0%
encoding-check
        avg: -0.3%      min: -0.6%      max: 0.0%
syn-check
        avg: -0.2%      min: -0.6%      max: 0.0%
style-servo-check
        avg: -0.3%      min: -0.5%      max: 0.0%

@bors try @rust-timer queue

rust-timer · 2020-01-31T08:27:21Z

Awaiting bors try build completion

bors · 2020-01-31T08:27:35Z

⌛ Trying commit cf1039f with merge 20d9fc0...

@varkor

Reduce the number of `RefCell`s in `InferCtxt`. `InferCtxt` contains six structures within `RefCell`s. Every time we create and dispose of (commit or rollback) a snapshot we have to `borrow_mut` each one of them. This commit moves the six structures under a single `RefCell`, which gives significant speed-ups by reducing the number of `borrow_mut` calls. To avoid runtime errors I had to reduce the lifetimes of dynamic borrows in a couple of places. r? @varkor

bors · 2020-01-31T11:31:53Z

☀️ Try build successful - checks-azure
Build commit: 20d9fc0 (20d9fc0f22e840065c9122aec9a4732a6d4d4749)

Mark-Simulacrum · 2020-01-31T11:44:00Z

@rust-timer build 20d9fc0

rust-timer · 2020-01-31T11:44:02Z

Queued 20d9fc0 with parent 266ecd6, future comparison URL.

rust-timer · 2020-01-31T19:57:23Z

Finished benchmarking try commit 20d9fc0, comparison URL.

nnethercote · 2020-01-31T21:08:29Z

It's unfortuanate that InferCtxt uses RefCell and Cell so heavily, but I can't see how to change things to avoid them altogether; there's just too much mutation going on.

varkor

This looks good, thanks! r=me after rebasing.

nnethercote · 2020-02-09T01:02:12Z

AFAICT no rebasing is required.

@bors r=varkor

bors · 2020-02-09T01:02:13Z

📌 Commit cf1039f has been approved by varkor

nnethercote · 2020-02-09T01:03:09Z

Oops, there are conflicts.

nnethercote · 2020-02-09T02:22:51Z

I rebased.

@bors r=varkor

bors · 2020-02-09T02:22:52Z

📌 Commit e183f82 has been approved by varkor

rust-highfive · 2020-02-09T02:34:54Z

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.

2020-02-09T02:23:01.6105934Z ========================== Starting Command Output ===========================
2020-02-09T02:23:01.6107466Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/6638de6c-46d5-4e11-9183-c40a69ac503a.sh
2020-02-09T02:23:01.6107506Z 
2020-02-09T02:23:01.6110266Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-09T02:23:01.6116219Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68694/merge to s
2020-02-09T02:23:01.6117851Z Task         : Get sources
2020-02-09T02:23:01.6117900Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-09T02:23:01.6117934Z Version      : 1.0.0
2020-02-09T02:23:01.6117967Z Author       : Microsoft
---
2020-02-09T02:23:02.4718464Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-09T02:23:02.4809101Z ##[command]git config gc.auto 0
2020-02-09T02:23:02.4892275Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-09T02:23:02.4961247Z ##[command]git config --get-all http.proxy
2020-02-09T02:23:02.5117122Z ##[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/68694/merge:refs/remotes/pull/68694/merge
---
2020-02-09T02:30:00.5943522Z     Checking rustc_parse v0.0.0 (/checkout/src/librustc_parse)
2020-02-09T02:30:03.0304294Z     Checking rustc_ast_passes v0.0.0 (/checkout/src/librustc_ast_passes)
2020-02-09T02:30:03.8329294Z     Checking rustc_expand v0.0.0 (/checkout/src/librustc_expand)
2020-02-09T02:30:05.4007644Z     Checking rustc_builtin_macros v0.0.0 (/checkout/src/librustc_builtin_macros)
2020-02-09T02:30:10.3007868Z error[E0063]: missing field `skip_leak_check` in initializer of `infer::InferCtxtInner<'_>`
2020-02-09T02:30:10.3010017Z     |
2020-02-09T02:30:10.3011132Z 180 |         InferCtxtInner {
2020-02-09T02:30:10.3013020Z     |         ^^^^^^^^^^^^^^ missing `skip_leak_check`
2020-02-09T02:30:10.3013478Z 
2020-02-09T02:30:10.3013478Z 
2020-02-09T02:30:10.3407283Z error[E0560]: struct `infer::InferCtxt<'_, '_>` has no field named `skip_leak_check`
2020-02-09T02:30:10.3408450Z    --> src/librustc/infer/mod.rs:578:17
2020-02-09T02:30:10.3408933Z     |
2020-02-09T02:30:10.3409424Z 578 |                 skip_leak_check: Cell::new(false),
2020-02-09T02:30:10.3410063Z     |                 ^^^^^^^^^^^^^^^ `infer::InferCtxt<'_, '_>` does not have this field
2020-02-09T02:30:10.3410481Z     |
2020-02-09T02:30:10.3411068Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3411807Z error[E0560]: struct `infer::InferCtxt<'_, '_>` has no field named `region_obligations`
2020-02-09T02:30:10.3412275Z    --> src/librustc/infer/mod.rs:579:17
2020-02-09T02:30:10.3412673Z     |
2020-02-09T02:30:10.3412673Z     |
2020-02-09T02:30:10.3413172Z 579 |                 region_obligations: RefCell::new(vec![]),
2020-02-09T02:30:10.3413749Z     |                 ^^^^^^^^^^^^^^^^^^ `infer::InferCtxt<'_, '_>` does not have this field
2020-02-09T02:30:10.3414154Z     |
2020-02-09T02:30:10.3414719Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3414897Z 
2020-02-09T02:30:10.3820852Z error[E0609]: no field `skip_leak_check` on type `&infer::InferCtxt<'a, 'tcx>`
2020-02-09T02:30:10.3822049Z     |
2020-02-09T02:30:10.3822049Z     |
2020-02-09T02:30:10.3822362Z 751 |             was_skip_leak_check: self.skip_leak_check.get(),
2020-02-09T02:30:10.3825141Z     |
2020-02-09T02:30:10.3825141Z     |
2020-02-09T02:30:10.3825579Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3825634Z 
2020-02-09T02:30:10.3849210Z error[E0609]: no field `skip_leak_check` on type `&infer::InferCtxt<'a, 'tcx>`
2020-02-09T02:30:10.3849882Z     |
2020-02-09T02:30:10.3849882Z     |
2020-02-09T02:30:10.3850224Z 776 |         self.skip_leak_check.set(was_skip_leak_check);
2020-02-09T02:30:10.3851091Z     |
2020-02-09T02:30:10.3851091Z     |
2020-02-09T02:30:10.3851639Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3851699Z 
2020-02-09T02:30:10.3878741Z error[E0609]: no field `skip_leak_check` on type `&infer::InferCtxt<'a, 'tcx>`
2020-02-09T02:30:10.3879547Z     |
2020-02-09T02:30:10.3879547Z     |
2020-02-09T02:30:10.3879925Z 805 |         self.skip_leak_check.set(was_skip_leak_check);
2020-02-09T02:30:10.3880558Z     |
2020-02-09T02:30:10.3880558Z     |
2020-02-09T02:30:10.3880944Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3880993Z 
2020-02-09T02:30:10.3973087Z error[E0609]: no field `skip_leak_check` on type `&infer::InferCtxt<'a, 'tcx>`
2020-02-09T02:30:10.3973685Z     |
2020-02-09T02:30:10.3973685Z     |
2020-02-09T02:30:10.3974011Z 867 |         let skip_leak_check = should_skip || self.skip_leak_check.get();
2020-02-09T02:30:10.3974657Z     |
2020-02-09T02:30:10.3974657Z     |
2020-02-09T02:30:10.3975039Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3975103Z 
2020-02-09T02:30:10.3986155Z error[E0609]: no field `skip_leak_check` on type `&infer::InferCtxt<'a, 'tcx>`
2020-02-09T02:30:10.3986708Z     |
2020-02-09T02:30:10.3986708Z     |
2020-02-09T02:30:10.3987019Z 868 |         self.skip_leak_check.set(skip_leak_check);
2020-02-09T02:30:10.3987590Z     |
2020-02-09T02:30:10.3987590Z     |
2020-02-09T02:30:10.3987984Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:10.3988028Z 
2020-02-09T02:30:11.0206345Z error[E0609]: no field `skip_leak_check` on type `&infer::InferCtxt<'a, 'tcx>`
2020-02-09T02:30:11.0206767Z    --> src/librustc/infer/higher_ranked/mod.rs:137:68
2020-02-09T02:30:11.0207052Z     |
2020-02-09T02:30:11.0207466Z 137 |         if self.tcx.sess.opts.debugging_opts.no_leak_check || self.skip_leak_check.get() {
2020-02-09T02:30:11.0208149Z     |
2020-02-09T02:30:11.0208149Z     |
2020-02-09T02:30:11.0208778Z     = note: available fields are: `tcx`, `in_progress_tables`, `inner`, `lexical_region_resolutions`, `selection_cache` ... and 7 others
2020-02-09T02:30:20.2725696Z error: aborting due to 9 previous errors
2020-02-09T02:30:20.2726505Z 
2020-02-09T02:30:20.2727104Z Some errors have detailed explanations: E0063, E0560, E0609.
2020-02-09T02:30:20.2727873Z For more information about an error, try `rustc --explain E0063`.
2020-02-09T02:30:20.2727873Z For more information about an error, try `rustc --explain E0063`.
2020-02-09T02:30:20.3005883Z error: could not compile `rustc`.
2020-02-09T02:30:20.3006632Z 
2020-02-09T02:30:20.3007634Z To learn more, run the command again with --verbose.
2020-02-09T02:30:20.3053193Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "-Zconfig-profile" "--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"
2020-02-09T02:30:20.3057062Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2020-02-09T02:30:20.3057422Z Build completed unsuccessfully in 0:05:12
2020-02-09T02:30:20.3114155Z == clock drift check ==
2020-02-09T02:30:20.3146073Z   local time: Sun Feb  9 02:30:20 UTC 2020
2020-02-09T02:30:20.3146073Z   local time: Sun Feb  9 02:30:20 UTC 2020
2020-02-09T02:30:20.4680992Z   network time: Sun, 09 Feb 2020 02:30:20 GMT
2020-02-09T02:30:20.4685655Z == end clock drift check ==
2020-02-09T02:30:21.1172139Z 
2020-02-09T02:30:21.1284204Z ##[error]Bash exited with code '1'.
2020-02-09T02:30:21.1296085Z ##[section]Finishing: Run build
2020-02-09T02:30:21.1310763Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68694/merge to s
2020-02-09T02:30:21.1312575Z Task         : Get sources
2020-02-09T02:30:21.1312623Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-09T02:30:21.1312688Z Version      : 1.0.0
2020-02-09T02:30:21.1312730Z Author       : Microsoft
2020-02-09T02:30:21.1312730Z Author       : Microsoft
2020-02-09T02:30:21.1312776Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-09T02:30:21.1312845Z ==============================================================================
2020-02-09T02:30:21.5531193Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-09T02:30:21.5533589Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/68694/merge to s
2020-02-09T02:30:21.5650593Z Cleaning up task key
2020-02-09T02:30:21.5651361Z Start cleaning up orphan processes.
2020-02-09T02:30:21.5762672Z Terminate orphan process: pid (3685) (python)
2020-02-09T02:30:21.5956988Z ##[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 @TimNN. (Feature Requests)

@varkor

…rCtxt, r=varkor Reduce the number of `RefCell`s in `InferCtxt`. `InferCtxt` contains six structures within `RefCell`s. Every time we create and dispose of (commit or rollback) a snapshot we have to `borrow_mut` each one of them. This commit moves the six structures under a single `RefCell`, which gives significant speed-ups by reducing the number of `borrow_mut` calls. To avoid runtime errors I had to reduce the lifetimes of dynamic borrows in a couple of places. r? @varkor

jonas-schievink · 2020-02-09T14:38:16Z

@bors r-

`InferCtxt` contains six structures within `RefCell`s. Every time we create and dispose of (commit or rollback) a snapshot we have to `borrow_mut` each one of them. This commit moves the six structures under a single `RefCell`, which gives significant speed-ups by reducing the number of `borrow_mut` calls. To avoid runtime errors I had to reduce the lifetimes of dynamic borrows in a couple of places.

nnethercote · 2020-02-09T21:57:20Z

Bah, I fell for the old git-push-with-uncommitted-changes trick. Let's try again.

@bors r=varkor

bors · 2020-02-09T21:57:21Z

📌 Commit 7426853 has been approved by varkor

@varkor

…rCtxt, r=varkor Reduce the number of `RefCell`s in `InferCtxt`. `InferCtxt` contains six structures within `RefCell`s. Every time we create and dispose of (commit or rollback) a snapshot we have to `borrow_mut` each one of them. This commit moves the six structures under a single `RefCell`, which gives significant speed-ups by reducing the number of `borrow_mut` calls. To avoid runtime errors I had to reduce the lifetimes of dynamic borrows in a couple of places. r? @varkor

@ghost

Rollup of 6 pull requests Successful merges: - #68694 (Reduce the number of `RefCell`s in `InferCtxt`.) - #68966 (Improve performance of coherence checks) - #68976 (Make `num::NonZeroX::new` an unstable `const fn`) - #68992 (Correctly parse `mut a @ b`) - #69005 (Small graphviz improvements for the new dataflow framework) - #69006 (parser: Keep current and previous tokens precisely) Failed merges: r? @ghost

Zoxc · 2020-02-10T05:23:58Z

@nnethercote Could we use a CellVec, CellHashMap to avoid the RefCell all together or do these generally need to borrow their stored values?

@ghost

Rollup of 6 pull requests Successful merges: - #68694 (Reduce the number of `RefCell`s in `InferCtxt`.) - #68966 (Improve performance of coherence checks) - #68976 (Make `num::NonZeroX::new` an unstable `const fn`) - #68992 (Correctly parse `mut a @ b`) - #69005 (Small graphviz improvements for the new dataflow framework) - #69006 (parser: Keep current and previous tokens precisely) Failed merges: r? @ghost

nnethercote · 2020-02-10T23:11:23Z

@nnethercote Could we use a CellVec, CellHashMap to avoid the RefCell all together or do these generally need to borrow their stored values?

I don't know anything about CellVec or CellHashMap. I tried searching with Google and on crates.io without luck. Can you provide a link to some information about them?

Zoxc · 2020-02-11T06:49:09Z

Oh, they're just hypothetical data structures which avoid the cost of a RefCell by only exposing operations which do not return references to their interior, like Cell. This makes them mostly useful when doing simple get/set operations with Copy types.

rust-highfive assigned varkor Jan 31, 2020

rust-highfive added the S-waiting-on-review label Jan 31, 2020

varkor approved these changes Feb 9, 2020

View changes

bors added S-waiting-on-bors and removed S-waiting-on-review labels Feb 9, 2020

nnethercote force-pushed the nnethercote:reduce-RefCells-in-InferCtxt branch from cf1039f to e183f82 Feb 9, 2020

Dylan-DPC mentioned this pull request Feb 9, 2020

Rollup of 4 pull requests #68996

Closed

bors added S-waiting-on-author and removed S-waiting-on-bors labels Feb 9, 2020

nnethercote force-pushed the nnethercote:reduce-RefCells-in-InferCtxt branch from e183f82 to 7426853 Feb 9, 2020

bors added S-waiting-on-bors and removed S-waiting-on-author labels Feb 9, 2020

Dylan-DPC mentioned this pull request Feb 10, 2020

Rollup of 6 pull requests #69012

Merged

nnethercote deleted the nnethercote:reduce-RefCells-in-InferCtxt branch Feb 10, 2020

Marwes mentioned this pull request Apr 16, 2020

perf: Unify the undo log of all snapshot types #69464

Merged

rust-lang / rust

Reduce the number of `RefCell`s in `InferCtxt`. #68694

Reduce the number of `RefCell`s in `InferCtxt`. #68694

nnethercote commented Jan 31, 2020

nnethercote commented Jan 31, 2020

rust-timer commented Jan 31, 2020

bors commented Jan 31, 2020

bors commented Jan 31, 2020

Mark-Simulacrum commented Jan 31, 2020

rust-timer commented Jan 31, 2020

rust-timer commented Jan 31, 2020

nnethercote commented Jan 31, 2020

varkor left a comment

nnethercote commented Feb 9, 2020

bors commented Feb 9, 2020

nnethercote commented Feb 9, 2020

nnethercote commented Feb 9, 2020

bors commented Feb 9, 2020

rust-highfive commented Feb 9, 2020

jonas-schievink commented Feb 9, 2020

nnethercote commented Feb 9, 2020

bors commented Feb 9, 2020

Zoxc commented Feb 10, 2020

nnethercote commented Feb 10, 2020

Zoxc commented Feb 11, 2020

rust-lang / rust

Join GitHub today

GitHub is where the world builds software

Reduce the number of `RefCell`s in `InferCtxt`. #68694

Reduce the number of `RefCell`s in `InferCtxt`. #68694

Conversation

nnethercote commented Jan 31, 2020

nnethercote commented Jan 31, 2020

rust-timer commented Jan 31, 2020

bors commented Jan 31, 2020

bors commented Jan 31, 2020

Mark-Simulacrum commented Jan 31, 2020

rust-timer commented Jan 31, 2020

rust-timer commented Jan 31, 2020

nnethercote commented Jan 31, 2020

varkor left a comment

nnethercote commented Feb 9, 2020

bors commented Feb 9, 2020

nnethercote commented Feb 9, 2020

nnethercote commented Feb 9, 2020

bors commented Feb 9, 2020

rust-highfive commented Feb 9, 2020

jonas-schievink commented Feb 9, 2020

nnethercote commented Feb 9, 2020

bors commented Feb 9, 2020

Zoxc commented Feb 10, 2020

nnethercote commented Feb 10, 2020

Zoxc commented Feb 11, 2020

Essential cookies

Always active

Analytics cookies