Skip to content

refactor: Remove fields from ItemTree #19829

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShoyuVanilla
Copy link
Member

@ShoyuVanilla ShoyuVanilla commented May 20, 2025

Resolves #19549

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2025
@ShoyuVanilla
Copy link
Member Author

Well, this causes a non-trivial regression on inference time in CI's analysis-stats 🤔

@ShoyuVanilla ShoyuVanilla force-pushed the item-tree-sans-fields branch from 54f08dd to 3024ddd Compare May 20, 2025 18:34
@ChayimFriedman2
Copy link
Contributor

analysis-stats in CI runs on debug build, do not consider it. Try locally with a release build.

@ShoyuVanilla
Copy link
Member Author

analysis-stats in CI runs on debug build, do not consider it. Try locally with a release build.

Fair enough. It might worth switching into release build on analysis-stats in CI, I think?

@ShoyuVanilla
Copy link
Member Author

ShoyuVanilla commented May 20, 2025

Unfortunately, it shows regression in local release build as well. (I tried several iterations on each revision and got similar results)

Before this PR

Database loaded:     533.64ms, 315minstr, 99mb (metadata 190.67ms, 5535kinstr, 1055kb; build 139.13ms, 12minstr, 47kb)
  item trees: 1302
  dependency lines of code: 2_362_682, item trees: 4_952
  dependency item stats: traits: 1_442, impl: 23_416, mods: 5_683, macro calls: 8_219, macro rules: 1_935
Item Tree Collection: 4.76s, 51ginstr, 866mb
  Total Statistics:
    crates: 57, mods: 1152, decls: 29260, bodies: 28393, adts: 2043, consts: 1306
  Workspace:
    traits: 101, macro_rules macros: 20, proc_macros: 1
    lines of code: 477_654, item trees: 1_302
    usages: traits: 166, impl: 4_100, mods: 1_153, macro calls: 169, macro rules: 117
  Dependencies:
    lines of code: 2_362_682, item trees: 4_952
    declarations: traits: 1_442, impl: 23_416, mods: 5_683, macro calls: 8_219, macro rules: 1_935
Item Collection:     10.12s, 53ginstr, 1042mb
Body lowering:       3.44s, 24ginstr, 539mb
  exprs: 865095, ??ty: 107 (0%), ?ty: 146 (0%), !ty: 9
  pats: 188717, ??ty: 7 (0%), ?ty: 10 (0%), !ty: 0
  panics: 0
Inference:           88.27s, 475ginstr, 1111mb
MIR lowering:        15.55s, 74ginstr, 396mb
Mir failed bodies: 1104 (4%)
Data layouts:        86.78ms, 335minstr, 16mb
Failed data layouts: 6 (0%)
Const evaluation:    52.56ms, 227minstr, 3915kb
Failed const evals: 5 (0%)
Total:               123.61s, 685ginstr, 2275mb
After this PR

Database loaded:     499.90ms, 471minstr, 99mb (metadata 188.14ms, 94kinstr, 1058kb; build 145.23ms, 10minstr, 48kb)
  item trees: 1302
  dependency lines of code: 2_362_682, item trees: 4_952
  dependency item stats: traits: 1_442, impl: 23_416, mods: 5_683, macro calls: 8_219, macro rules: 1_935
Item Tree Collection: 4.72s, 51ginstr, 865mb
  Total Statistics:
    crates: 57, mods: 1152, decls: 29260, bodies: 28393, adts: 2043, consts: 1306
  Workspace:
    traits: 101, macro_rules macros: 20, proc_macros: 1
    lines of code: 477_654, item trees: 1_302
    usages: traits: 166, impl: 4_100, mods: 1_153, macro calls: 169, macro rules: 117
  Dependencies:
    lines of code: 2_362_682, item trees: 4_952
    declarations: traits: 1_442, impl: 23_416, mods: 5_683, macro calls: 8_219, macro rules: 1_935
Item Collection:     10.16s, 53ginstr, 1042mb
Body lowering:       3.50s, 24ginstr, 539mb
  exprs: 865095, ??ty: 107 (0%), ?ty: 146 (0%), !ty: 9
  pats: 188717, ??ty: 7 (0%), ?ty: 10 (0%), !ty: 0
  panics: 0
Inference:           97.95s, 506ginstr, 1110mb
MIR lowering:        15.85s, 76ginstr, 396mb
Mir failed bodies: 1104 (4%)
Data layouts:        85.32ms, 340minstr, 16mb
Failed data layouts: 6 (0%)
Const evaluation:    51.33ms, 227minstr, 3921kb
Failed const evals: 5 (0%)
Total:               133.64s, 717ginstr, 2272mb

I'll try with flamegraph tomorrow

edit) exchanged the result order, as I messed them up by mistake 😅

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented May 20, 2025

That looks faster with this PR?

@ChayimFriedman2
Copy link
Contributor

Fair enough. It might worth switching into release build on analysis-stats in CI, I think?

We tried, it doesn't produce a difference in time it takes (because the build was taking longer), so there was no incentive.

@ChayimFriedman2
Copy link
Contributor

@ShoyuVanilla I'm currently in the middle of a larger refactor that will make the item tree only an input to the def map (that is, the signature queries won't use it at all. The reason is to help incrementality). This will be trivial after. As such, I think it's best to postpone this PR.

@ShoyuVanilla
Copy link
Member Author

@ShoyuVanilla I'm currently in the middle of a larger refactor that will make the item tree only an input to the def map (that is, the signature queries won't use it at all. The reason is to help incrementality). This will be trivial after. As such, I think it's best to postpone this PR.

Oh, that would be nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move ADT fields out of the item tree
3 participants