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

forego caching cycles leads to a severe perf regression #60846

Closed
ipetkov opened this issue May 15, 2019 · 8 comments · Fixed by #61754
Closed

forego caching cycles leads to a severe perf regression #60846

ipetkov opened this issue May 15, 2019 · 8 comments · Fixed by #61754
Assignees
Labels
A-traits Area: Trait system I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ipetkov
Copy link
Contributor

ipetkov commented May 15, 2019

#60444 has introduced a severe perf regression when building/testing the conch-runtime crate.

Previously a test run would take ~5 mins, and with the latest nightly (rustc 1.36.0-nightly (372be4f36 2019-05-14)) it now takes ~82(!!) mins.

$ git clone https://github.com/ipetkov/conch-runtime.git
$ cd conch-runtime
$ cargo test --lib
   # snip
    Finished dev [unoptimized + debuginfo] target(s) in 4m 16s
$ cargo clean
$ cargo +nightly test --lib
   # snip
    Finished dev [unoptimized + debuginfo] target(s) in 82m 54s
Crate info The crate offers the functionality to execute shell programs. Each piece of the grammar is represented as a node which can hold generic sub-nodes. The reasoning for this is so that the crate consumer could customize their AST with different/custom nodes, while reusing existing implementations.

The shell grammar is deeply recursive. Basically each command can vary in complexity (compound commands such as case, for, or simple commands like echo foo), but is ultimately made up of a list of shell words (literals, interpolations, etc.). Because each word can contain a command substitution, the AST type is recursive (a Command<W> has a Word<C> type, which gives us Command<Word<Command<...>>).

There are two "top-level" type definitions which seek to unify the entire AST tree concretely which are basically TopLevelCommand(Command<TopLevelWord>) TopLevelWord(Word<TopLevelCommand>).

The crate also heavily uses generics and trait bounds (perhaps overly so), however, there's hopefully some low hanging fruits that can reduce the 16x slow down in performance.

cc @nikomatsakis @pnkfelix

@Centril Centril added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 15, 2019
@jonas-schievink jonas-schievink added I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 15, 2019
@pnkfelix pnkfelix added the A-traits Area: Trait system label May 15, 2019
@pnkfelix
Copy link
Member

a 16x compile-time regression does sound pretty bad indeed.

@pnkfelix
Copy link
Member

triage: P-high. Leaving nomination tag, as I would like to discuss strategies for addressing this at the meeting, if possible.

@pnkfelix pnkfelix added the P-high High priority label May 16, 2019
@cramertj
Copy link
Member

@matklad also thinks this might have caused this failure in rust-analyzer: rust-lang/rust-analyzer#1283

@pnkfelix
Copy link
Member

"discussed" at T-compiler meeting. Assigning to self to investigate. Removing nomination tag.

@pnkfelix pnkfelix self-assigned this May 23, 2019
@jonas-schievink jonas-schievink added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 23, 2019
@nikomatsakis
Copy link
Contributor

Well, it was known that this could cause problems in performance. I don't know that there is a simple fix. (I suspect the errors in rust-analyzer are legit, as well)

@nikomatsakis
Copy link
Contributor

But I was contemplating starting on a more complete re-write of the trait solver (kind of an intermediate step towards switching to chalk). I think that might be what is ultimately needed. (Note that chalk actually has a variant of this same bug...)

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 10, 2019

I have a potential fix for this.

UPDATE: But I may have just realized a flaw in the caching scheme I was planning on.

@nikomatsakis
Copy link
Contributor

OK, #61754 is up, though still doing final tests. 🤞 When I ran it locally, it seemed to resolve the perf slowdown.

pnkfelix added a commit to nikomatsakis/rust that referenced this issue Jun 14, 2019
based on rust-lang#61754 (comment) I am adding `bootstrap` to the cfg-preconditions for the two manual `unsafe impls`'s of `Send` and `Sync` for `TokenTree`.
bors added a commit that referenced this issue Jun 16, 2019
create a "provisional cache" to restore performance in the case of cycles

Introduce a "provisional cache" that caches the results of auto trait resolutions but keeps them from entering the *main* cache until everything is ready. This turned out a bit more complex than I hoped, but I don't see another short term fix -- happy to take suggestions! In the meantime, it's very clear we need to rework the trait solver. This resolves the extreme performance slowdown experienced in #60846 -- I plan to add a perf.rust-lang.org regression test to track this.

Caveat: I've not run `x.py test` in full yet.

r? @pnkfelix
cc @arielb1

Fixes #60846
Mark-Simulacrum pushed a commit to pietroalbini/rust that referenced this issue Jun 26, 2019
…caching-perf-3, r=pnkfelix

create a "provisional cache" to restore performance in the case of cycles

Introduce a "provisional cache" that caches the results of auto trait resolutions but keeps them from entering the *main* cache until everything is ready. This turned out a bit more complex than I hoped, but I don't see another short term fix -- happy to take suggestions! In the meantime, it's very clear we need to rework the trait solver. This resolves the extreme performance slowdown experienced in rust-lang#60846 -- I plan to add a perf.rust-lang.org regression test to track this.

Caveat: I've not run `x.py test` in full yet.

r? @pnkfelix
cc @arielb1

Fixes rust-lang#60846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants