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

Fix generator size regressions due to optimization #63034

Merged
merged 3 commits into from Aug 7, 2019

Conversation

@tmandry
Copy link
Contributor

commented Jul 27, 2019

I tested the generator optimizations in #60187 and #61922 on the Fuchsia
build, and noticed that some small generators (about 8% of the async fns
in our build) increased in size slightly.

This is because in #60187 we split the fields into two groups, a
"prefix" non-overlap region and an overlap region, and lay them out
separately. This can introduce unnecessary padding bytes between the two
groups.

In every single case in the Fuchsia build, it was due to there being
only a single variant being used in the overlap region. This means that
we aren't doing any overlapping, period. So it's better to combine the
two regions into one and lay out all the fields at once, which is what
this change does.

r? @cramertj
cc @eddyb @Zoxc

@tmandry

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

This actually exposed a latent bug related to uninhabitedness (basically #59972). I'm working on a PR to fix this, which this PR is blocked on.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-07-27T04:06:34.3443571Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-07-27T04:06:34.3667527Z ##[command]git config gc.auto 0
2019-07-27T04:06:34.3712150Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-07-27T04:06:34.3763801Z ##[command]git config --get-all http.proxy
2019-07-27T04:06:34.3906271Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63034/merge:refs/remotes/pull/63034/merge
---
2019-07-27T04:07:06.6229567Z do so (now or later) by using -b with the checkout command again. Example:
2019-07-27T04:07:06.6229597Z 
2019-07-27T04:07:06.6229808Z   git checkout -b <new-branch-name>
2019-07-27T04:07:06.6229856Z 
2019-07-27T04:07:06.6229921Z HEAD is now at 520f1b0de Merge fd1583738cbdd24f9ad0fdbc273be2d6496e9413 into 09e39897587dca70f0b15093d425a682c392349c
2019-07-27T04:07:06.6374560Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-07-27T04:07:06.6377371Z ==============================================================================
2019-07-27T04:07:06.6377445Z Task         : Bash
2019-07-27T04:07:06.6377493Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-07-27T05:09:15.3829107Z .................................................................................................... 700/5870
2019-07-27T05:09:19.4552917Z .................................................................................................... 800/5870
2019-07-27T05:09:25.1056259Z .................................................................................................... 900/5870
2019-07-27T05:09:30.2507540Z .................................................................................................... 1000/5870
2019-07-27T05:09:35.8449927Z i...........i....................................................................................... 1100/5870
2019-07-27T05:09:39.8620066Z ..............................iiiii................................................................. 1200/5870
2019-07-27T05:09:45.9588391Z .................................................................................................... 1400/5870
2019-07-27T05:09:48.6677913Z .................................................................................................... 1500/5870
2019-07-27T05:09:52.4815025Z .................................................................................................... 1600/5870
2019-07-27T05:09:55.1500778Z .................................................................................................... 1700/5870
---
2019-07-27T05:11:09.4534356Z .................................................................................................... 3400/5870
2019-07-27T05:11:14.5040559Z .................................................................................................... 3500/5870
2019-07-27T05:11:18.4724693Z ..........................i......................................................................... 3600/5870
2019-07-27T05:11:22.8288084Z .................................................................................................... 3700/5870
2019-07-27T05:11:26.3156207Z ....ii...i..ii...................................................................................... 3800/5870
2019-07-27T05:11:35.2422553Z .................................................................................................... 4000/5870
2019-07-27T05:11:39.1200471Z .......................ii........................................................................... 4100/5870
2019-07-27T05:11:41.3919664Z ............................................i....................................................... 4200/5870
2019-07-27T05:11:43.5816488Z .................................................................................................... 4300/5870
---
2019-07-27T05:13:05.9126948Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:535:22
2019-07-27T05:13:05.9127041Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-07-27T05:13:05.9138391Z 
2019-07-27T05:13:05.9138480Z 
2019-07-27T05:13:05.9144119Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-07-27T05:13:05.9144426Z 
2019-07-27T05:13:05.9144456Z 
2019-07-27T05:13:05.9155017Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-07-27T05:13:05.9155285Z Build completed unsuccessfully in 0:58:12
2019-07-27T05:13:05.9155285Z Build completed unsuccessfully in 0:58:12
2019-07-27T05:13:06.9925786Z ##[error]Bash exited with code '1'.
2019-07-27T05:13:06.9969208Z ##[section]Starting: Checkout
2019-07-27T05:13:06.9971575Z ==============================================================================
2019-07-27T05:13:06.9971633Z Task         : Get sources
2019-07-27T05:13:06.9971680Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

tmandry added 2 commits Jul 27, 2019
Wrap promoted generator fields in MaybeUninit
This prevents uninhabited fields from "infecting" the abi and
largest_niche of the generator layout.

This fixes a latent bug, where an uninhabited field could be promoted to
the generator prefix and cause the entire generator to become
uninhabited.
@JohnCSimon

This comment has been minimized.

Copy link

commented Aug 3, 2019

@tmandry Howdy from Triage! Is this PR still blocked?

@tmandry

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Fix generator size regressions due to optimization
I tested the generator optimizations in #60187 and #61922 on the Fuchsia
build, and noticed that some small generators (about 8% of the async fns
in our build) increased in size slightly.

This is because in #60187 we split the fields into two groups, a
"prefix" non-overlap region and an overlap region, and lay them out
separately. This can introduce unnecessary padding bytes between the two
groups.

In every single case in the Fuchsia build, it was due to there being
only a single variant being used in the overlap region. This means that
we aren't doing any overlapping, period. So it's better to combine the
two regions into one and lay out all the fields at once, which is what
this change does.

@tmandry tmandry force-pushed the tmandry:reduce-generator-size-regressions branch from fd15837 to b40788e Aug 6, 2019

@tmandry tmandry changed the title [WIP] Fix generator size regressions due to optimization Fix generator size regressions due to optimization Aug 6, 2019

@cramertj

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

📌 Commit b40788e has been approved by cramertj

Centril added a commit to Centril/rust that referenced this pull request Aug 6, 2019
Rollup merge of rust-lang#63034 - tmandry:reduce-generator-size-regre…
…ssions, r=cramertj

Fix generator size regressions due to optimization

I tested the generator optimizations in rust-lang#60187 and rust-lang#61922 on the Fuchsia
build, and noticed that some small generators (about 8% of the async fns
in our build) increased in size slightly.

This is because in rust-lang#60187 we split the fields into two groups, a
"prefix" non-overlap region and an overlap region, and lay them out
separately. This can introduce unnecessary padding bytes between the two
groups.

In every single case in the Fuchsia build, it was due to there being
only a single variant being used in the overlap region. This means that
we aren't doing any overlapping, period. So it's better to combine the
two regions into one and lay out all the fields at once, which is what
this change does.

r? @cramertj
cc @eddyb @Zoxc
@Centril Centril referenced this pull request Aug 6, 2019
bors added a commit that referenced this pull request Aug 7, 2019
Auto merge of #63341 - Centril:rollup-hkhxahb, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #63034 (Fix generator size regressions due to optimization)
 - #63035 (Use MaybeUninit to produce more correct layouts)
 - #63163 (add a pair of whitespace after remove parentheses)
 - #63294 (tests for async/await drop order)
 - #63307 (don't ignore mir_dump folder)
 - #63308 (PlaceRef's base is already a reference)
 - #63310 (Tests around moving parts of structs and tuples across await points)
 - #63314 (doc: the content has since been moved to the guide)
 - #63333 (Remove unnecessary features from compiler error code list)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

⌛️ Testing commit b40788e with merge 615c460...

@bors bors merged commit b40788e into rust-lang:master Aug 7, 2019

4 of 5 checks passed

homu Testing commit b40788e89fac38781ddb838bb4fb0d706a6a3546 with merge 615c46086a994f088c9ed569fc36df229ae115b6...
Details
pr Build #20190806.37 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

@tmandry tmandry deleted the tmandry:reduce-generator-size-regressions branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.