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

internal: Bump salsa #14700

Closed
wants to merge 1 commit into from
Closed

internal: Bump salsa #14700

wants to merge 1 commit into from

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented May 1, 2023

@lnicola
Copy link
Member Author

lnicola commented May 1, 2023

@Veykril can you give this a try (your old commit probably works too, though)?

@bors
Copy link
Contributor

bors commented May 1, 2023

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

@Veykril
Copy link
Member

Veykril commented May 2, 2023

Checked it out on my machine, the perf decrease is also in the 10% for me so I guess its fine (maybe analysis overall gains from this elsewhere if we are lucky)

@lnicola
Copy link
Member Author

lnicola commented May 2, 2023

Oof, okay, I hoped it would be slightly less :-(. Do you think we should merge this?

@Veykril
Copy link
Member

Veykril commented May 2, 2023

dot completion went from 56 to 63, path from 145 to 161 (though my machine is also a bit noisy)

I think its fine to merge (as part of bumping everything to syn 2), if it worsens more things like analysis stats we might want to look into not having the slot changes

@lnicola
Copy link
Member Author

lnicola commented May 2, 2023

It does show up in analysis-stats:

# baseline, 7d48bbadd449db111289d12db15d3a0dde1a9b14, best of 3
Database loaded:     506.62ms, 284minstr (metadata 300.22ms, 25minstr; build 102.00ms, 9815kinstr)
  crates: 44, mods: 918, decls: 19627, fns: 14681
Item Collection:     11.24s, 100ginstr
  exprs: 462280, ??ty: 33 (0%), ?ty: 129 (0%), !ty: 1                                                                                                                
  pats: 104232, ??ty: 4 (0%), ?ty: 11 (0%), !ty: 193
Inference:           38.46s, 280ginstr
Total:               49.70s, 381ginstr

# pr, best of 3
Database loaded:     509.21ms, 284minstr (metadata 301.47ms, 25minstr; build 105.61ms, 9815kinstr)
  crates: 44, mods: 918, decls: 19627, fns: 14681
Item Collection:     11.25s, 99ginstr
  exprs: 462280, ??ty: 33 (0%), ?ty: 129 (0%), !ty: 1                                                                                                                
  pats: 104232, ??ty: 4 (0%), ?ty: 11 (0%), !ty: 193
Inference:           41.70s, 296ginstr
Total:               52.96s, 395ginstr

@Veykril
Copy link
Member

Veykril commented May 2, 2023

hmm, not sure what to think about 3 seconds here to be honest 😅

@lnicola
Copy link
Member Author

lnicola commented May 2, 2023

I think it might be a bit less than that because of noise. Another couple of runs:

# baseline
Total:               48.82s, 383ginstr
Total:               50.38s, 385ginstr
Total:               50.79s, 400ginstr

# pr
Total:               49.01s, 377ginstr
Total:               53.74s, 406ginstr
Total:               52.95s, 410ginstr
Total:               50.36s, 385ginstr

The instruction counts are all over the place, too.

@Veykril
Copy link
Member

Veykril commented May 2, 2023

The instruction counts are all over the place, too.

That sounds terrible 😬. But okay, then let's just try it with the commits.

@lnicola lnicola marked this pull request as draft May 2, 2023 10:27
@bors
Copy link
Contributor

bors commented May 18, 2023

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

@lnicola
Copy link
Member Author

lnicola commented Aug 15, 2023

This would still be nice to have, but we'll have to fork or get someone to publish a new version before.

@lnicola lnicola closed this Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants