-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Upgrade Chalk #6597
Upgrade Chalk #6597
Conversation
@flodiebold that PR is merged; feel free to do a release for Chalk if you want to merge this before the release next weekend |
@flodiebold Chalk 41 has been released which contains your fix. |
😢 |
rust-lang/chalk#658 has been closed, so #6628 should work now? |
It should, but there's no new Chalk release yet. |
@flodiebold 0.43 has been released. |
Also make overflow depth and max type size configurable through env variables. This can be helpful at least for debugging. Fixes rust-lang#6628.
bors r+ |
Looks like this brought a nice perf improvement (the visible drop towards the right), cc @jackh726: |
@lnicola wow! That's impressive. I didn't even think about it, but that's probably the fold by value change. I'm actually really shocked, because when the same thing was implemented in rustc, IIRC there wasn't a huge perf win. |
While we are at this exciting topic, has anyone by any chance looked at enabling instruction counts on github actions? SO that we could have timings much more precise than wall-clock time. |
Last time we tried, it didn't seem to work, probably because of security limitations. And on the topic of metrics, we're no longer reporting memory usage since #5581. |
I think rust-lang/chalk#638 might also have played a role; I think/hope it's preventing us from enumerating all impls for a trait in a lot of cases where we previously would. |
Ah. That seems a bit more likely to me. |
Also make overflow depth and max type size configurable through env variables. This can be helpful at least for debugging.
Tests currently fail because of rust-lang/chalk#656, so we'll need to wait for the next update to merge this.