-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Reduce size of parse error #6146
Conversation
99ed381
to
6aadc6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to this sort of change, but I noticed a couple of things that I think need to change with this before merges.
I also want to confirm with @teoxoy and/or @jimblandy that this is a desirable change. I suspect that it might get rejected unless we can justify a quantitative gain here in terms of our own benchmarks (which should be easy?). @kornelski: Does this empirically reduce a memory high mark, or improve run times?
6aadc6c
to
f6fdf07
Compare
It reduces stack usage of |
How does this compare with boxing the error type itself? Maybe it can be done non-invasively via a newtype. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concerns have been resolved. I will defer to the rest of @gfx-rs/naga for merging.
@teoxoy I've tried boxing errors. It decreases perf by 2%, and the change is way more invasive — needs changing all return types, all There are some functions that match on the errors, so to avoid boxing, unboxing, and boxing again, they need to keep returning the unboxed error. |
f6fdf07
to
49525db
Compare
I see, thanks for giving it a shot! I think we can land this. |
Description
Reduces size of parse error from 112 bytes to 48 bytes. This should help with stack usage during parsing, which is especially important given the huge warning about heavy stack usage.
Testing
There's a test asserting the new size.
It has a noticeable performance improvement:
Checklist
cargo fmt
.cargo clippy
. If applicable, add:cargo xtask test
to run tests.