Use smaller discriminants for generators #69837

jonas-schievink · 2020-03-08T22:13:04Z

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)

Add test with a generator that has exactly 256 total states
Add test with a generator that has more than 256 states so that it needs to use a u16 discriminant
Add tests for the size of Option<[generator]>
Add tests for the discriminant_value intrinsic in all cases

rust-highfive · 2020-03-08T22:13:09Z

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

jonas-schievink · 2020-03-08T22:29:37Z

Running this on async-std's mutex.rs bench shows ~~some unfortunate regressions~~ that I compared the wrong compilers, there are no significant perf regressions on this benchmark (it got a bit faster, even):

master:

test contention    ... bench:   1,028,149 ns/iter (+/- 42,225)
test create        ... bench:           3 ns/iter (+/- 0)
test no_contention ... bench:     954,627 ns/iter (+/- 30,982)

This PR

test contention    ... bench:   1,025,108 ns/iter (+/- 87,055)
test create        ... bench:           3 ns/iter (+/- 0)
test no_contention ... bench:     886,630 ns/iter (+/- 24,225)

src/librustc/ty/layout.rs

ecstatic-morse · 2020-03-09T23:14:33Z

Nice! I don't have the requisite understanding of generators to approve this, so...

r? @tmandry

tmandry · 2020-03-09T23:49:02Z

src/test/ui/async-await/async-fn-size.rs

+    assert_eq!(3, std::mem::size_of_val(&await1_level1()));
+    assert_eq!(4, std::mem::size_of_val(&await2_level1()));
+    assert_eq!(5, std::mem::size_of_val(&await3_level1()));
+    assert_eq!(8, std::mem::size_of_val(&await3_level2()));
+    assert_eq!(11, std::mem::size_of_val(&await3_level3()));
+    assert_eq!(14, std::mem::size_of_val(&await3_level4()));
+    assert_eq!(17, std::mem::size_of_val(&await3_level5()));


much, much better :D

tmandry · 2020-03-09T23:54:05Z

src/test/ui/generator/discriminant.rs

+    assert_eq!(size_of_val(&gen_u8_tiny_niche()), 1);
+    assert_eq!(size_of_val(&Some(gen_u8_tiny_niche())), 1); // uses niche
+    assert_eq!(size_of_val(&Some(Some(gen_u8_tiny_niche()))), 2); // cannot use niche anymore
+    assert_eq!(size_of_val(&gen_u8_full()), 1);
+    assert_eq!(size_of_val(&Some(gen_u8_full())), 2); // cannot use niche
+    assert_eq!(size_of_val(&gen_u16()), 2);
+    assert_eq!(size_of_val(&Some(gen_u16())), 2); // uses niche
+
+    cycle(gen_u8_tiny_niche(), 254);
+    cycle(gen_u8_full(), 255);
+    cycle(gen_u16(), 256);


Great test. The discriminant checks are dependent on implementation details of the meaning of each value, but that's fine - we can update the test if those change.

tmandry · 2020-03-10T00:09:59Z

Looks great, thanks!

@bors r+

bors · 2020-03-10T00:10:00Z

📌 Commit b16d659 has been approved by tmandry

bors · 2020-03-10T00:10:01Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

…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

@ghost

Rollup of 6 pull requests Successful merges: - #69122 (Backtrace Debug tweaks) - #69591 (Use TypeRelating for instantiating query responses) - #69760 (Improve expression & attribute parsing) - #69837 (Use smaller discriminants for generators) - #69838 (Expansion-driven outline module parsing) - #69859 (fix #62456) Failed merges: r? @ghost

jonas-schievink · 2020-03-14T13:24:47Z

Fixed size assertions after merge of #69716

@bors r=tmandry

bors · 2020-03-14T13:24:49Z

📌 Commit 49aabd8 has been approved by tmandry

…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

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#67749 (keyword docs for else and inkeyword docs for else and in.) - rust-lang#69139 (clean up E0308 explanation) - rust-lang#69189 (Erase regions in writeback) - rust-lang#69837 (Use smaller discriminants for generators) - rust-lang#69838 (Expansion-driven outline module parsing) - rust-lang#69839 (Miri error reform) - rust-lang#69899 (Make methods declared by `newtype_index` macro `const`) - rust-lang#69920 (Remove some imports to the rustc crate) - rust-lang#70075 (Fix repr pretty display) - rust-lang#70106 (Tidy: fix running rustfmt twice) Failed merges: - rust-lang#70051 (Allow `hir().find` to return `None`) - rust-lang#70074 (Expand: nix all fatal errors) r? @ghost

Probably comes from rust-lang/rust#69837

jplatte · 2020-05-27T13:44:19Z

src/test/ui/generator/resume-arg-size.rs

@@ -23,6 +23,6 @@ fn main() {

    // Neither of these generators have the resume arg live across the `yield`, so they should be
    // 4 Bytes in size (only storing the discriminant)


@jonas-schievink Looks like you forgot to update the comment here.

@jplatte

…r=nikomatsakis Fix incorrect comment in generator test rust-lang#69837 (comment) (thanks for the catch, @jplatte)

Probably comes from rust-lang/rust#69837

rust-highfive assigned ecstatic-morse Mar 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2020

jonas-schievink mentioned this pull request Mar 8, 2020

Generators should use smaller discriminants #69815

Closed

jonas-schievink commented Mar 8, 2020

View reviewed changes

src/librustc/ty/layout.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rust-highfive assigned tmandry and unassigned ecstatic-morse Mar 9, 2020

tmandry reviewed Mar 10, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2020

Centril mentioned this pull request Mar 10, 2020

Rollup of 6 pull requests #69883

Closed

Centril mentioned this pull request Mar 10, 2020

Rollup of 6 pull requests #69884

Closed

jonas-schievink added 3 commits March 14, 2020 14:09

Use smaller discriminants for generators

b2779d8

Add a test for generator discriminants

4266807

Fix rebase fallout

49aabd8

Centril mentioned this pull request Mar 18, 2020

Rollup of 8 pull requests #70099

Closed

Centril mentioned this pull request Mar 18, 2020

Rollup of 10 pull requests #70109

Closed

Centril mentioned this pull request Mar 18, 2020

Rollup of 10 pull requests #70112

Merged

bors merged commit 4118ff6 into rust-lang:master Mar 19, 2020

SimonSapin mentioned this pull request Mar 20, 2020

Importing modules from inside include! no longer works #70185

Closed

goffrie mentioned this pull request Mar 28, 2020

Async performance regression #70488

Closed

Nemo157 added a commit to rust-lang/futures-rs that referenced this pull request Apr 7, 2020

Update expected size of select_on_non_unpin_size

3fc9310

Probably comes from rust-lang/rust#69837

Nemo157 mentioned this pull request Apr 7, 2020

Update expected size of select_on_non_unpin_size rust-lang/futures-rs#2119

Merged

jplatte reviewed May 27, 2020

View reviewed changes

jonas-schievink mentioned this pull request May 27, 2020

Fix incorrect comment in generator test #72670

Merged

jonas-schievink deleted the gen-discr-opt branch May 27, 2020 19:39

exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021

Update expected size of select_on_non_unpin_size

1f55f71

Probably comes from rust-lang/rust#69837

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use smaller discriminants for generators #69837

Use smaller discriminants for generators #69837

jonas-schievink commented Mar 8, 2020 •

edited

Loading

rust-highfive commented Mar 8, 2020

jonas-schievink commented Mar 8, 2020 •

edited

Loading

ecstatic-morse commented Mar 9, 2020 •

edited

Loading

This comment has been minimized.

tmandry Mar 9, 2020

tmandry Mar 9, 2020

tmandry commented Mar 10, 2020

bors commented Mar 10, 2020

bors commented Mar 10, 2020

jonas-schievink commented Mar 14, 2020

bors commented Mar 14, 2020

jplatte May 27, 2020

		@@ -23,6 +23,6 @@ fn main() {

		// Neither of these generators have the resume arg live across the `yield`, so they should be
		// 4 Bytes in size (only storing the discriminant)

Use smaller discriminants for generators #69837

Use smaller discriminants for generators #69837

Conversation

jonas-schievink commented Mar 8, 2020 • edited Loading

rust-highfive commented Mar 8, 2020

jonas-schievink commented Mar 8, 2020 • edited Loading

ecstatic-morse commented Mar 9, 2020 • edited Loading

This comment has been minimized.

tmandry Mar 9, 2020

Choose a reason for hiding this comment

tmandry Mar 9, 2020

Choose a reason for hiding this comment

tmandry commented Mar 10, 2020

bors commented Mar 10, 2020

bors commented Mar 10, 2020

jonas-schievink commented Mar 14, 2020

bors commented Mar 14, 2020

jplatte May 27, 2020

Choose a reason for hiding this comment

jonas-schievink commented Mar 8, 2020 •

edited

Loading

jonas-schievink commented Mar 8, 2020 •

edited

Loading

ecstatic-morse commented Mar 9, 2020 •

edited

Loading