Generators should use smaller discriminants #69815
Assignees
Labels
A-codegen
Area: Code generation
A-coroutines
Area: Coroutines
C-enhancement
Category: An issue proposing an enhancement or a PR with one.
I-heavy
Issue: Problems and improvements with respect to binary size of generated code.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
jonas-schievink
added
C-enhancement
7 tasks
|
The discriminant is also never marked with a valid range, meaning that it never provides any niches that could be used to store other values. Not sure if doing that has any drawbacks, but it seems like a somewhat easy size optimization on first glance. |
4 tasks
|
Fixing this was way easier than I expected, but I'd appreciate help with benchmarking the runtime impact of the fix in #69837. So if anyone knows any async/await benchmarks that are not I/O heavy (so that they mostly test CPU overhead of async/await), I'd be happy to run them. |
Centril
added a commit
to Centril/rust
that referenced
this issue
Mar 10, 2020
…andry Use smaller discriminants for generators Closes rust-lang#69815 I'm not yet sure about the runtime performance impact of this, so I'll try running this on some benchmarks (if I can find any). (Update: No impact on the benchmarks I've measured on) * [x] Add test with a generator that has exactly 256 total states * [x] Add test with a generator that has more than 256 states so that it needs to use a u16 discriminant * [x] Add tests for the size of `Option<[generator]>` * [x] Add tests for the `discriminant_value` intrinsic in all cases
Centril
added a commit
to Centril/rust
that referenced
this issue
Mar 10, 2020
…andry Use smaller discriminants for generators Closes rust-lang#69815 I'm not yet sure about the runtime performance impact of this, so I'll try running this on some benchmarks (if I can find any). (Update: No impact on the benchmarks I've measured on) * [x] Add test with a generator that has exactly 256 total states * [x] Add test with a generator that has more than 256 states so that it needs to use a u16 discriminant * [x] Add tests for the size of `Option<[generator]>` * [x] Add tests for the `discriminant_value` intrinsic in all cases
Centril
added a commit
to Centril/rust
that referenced
this issue
Mar 18, 2020
…andry Use smaller discriminants for generators Closes rust-lang#69815 I'm not yet sure about the runtime performance impact of this, so I'll try running this on some benchmarks (if I can find any). (Update: No impact on the benchmarks I've measured on) * [x] Add test with a generator that has exactly 256 total states * [x] Add test with a generator that has more than 256 states so that it needs to use a u16 discriminant * [x] Add tests for the size of `Option<[generator]>` * [x] Add tests for the `discriminant_value` intrinsic in all cases
Centril
added a commit
to Centril/rust
that referenced
this issue
Mar 18, 2020
…andry Use smaller discriminants for generators Closes rust-lang#69815 I'm not yet sure about the runtime performance impact of this, so I'll try running this on some benchmarks (if I can find any). (Update: No impact on the benchmarks I've measured on) * [x] Add test with a generator that has exactly 256 total states * [x] Add test with a generator that has more than 256 states so that it needs to use a u16 discriminant * [x] Add tests for the size of `Option<[generator]>` * [x] Add tests for the `discriminant_value` intrinsic in all cases
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently they use a hardcoded
u32discriminant, which is quite a lot of room considering that generators don't typically have billions of states. They should be able to use the smallest unsigned integer type that fits all states instead (sou8in the vast majority of cases).The text was updated successfully, but these errors were encountered: