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

Don't instantiate so many copies of drop_in_place #67332

Merged
merged 2 commits into from Feb 27, 2020

Conversation

@matthewjasper
Copy link
Contributor

matthewjasper commented Dec 15, 2019

Split out from #66703.

r? @ghost

@matthewjasper matthewjasper changed the title [PERF] Don't instantiate so many copied of `real_drop_in_place` [PERF] Don't instantiate so many copies of `real_drop_in_place` Dec 15, 2019
@matthewjasper matthewjasper changed the title [PERF] Don't instantiate so many copies of `real_drop_in_place` [PERF] Don't instantiate so many copies of real_drop_in_place Dec 15, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Dec 15, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 15, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 15, 2019

⌛️ Trying commit 60f7930 with merge 9890db7...

bors added a commit that referenced this pull request Dec 15, 2019
[PERF] Don't instantiate so many copies of real_drop_in_place

Split out from #66703.

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2019

☀️ Try build successful - checks-azure
Build commit: 9890db7 (9890db7e0f814f43c84898c7c756fd14d63aeff6)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 16, 2019

Queued 9890db7 with parent a605441, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 16, 2019

Finished benchmarking try commit 9890db7, comparison URL.

src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
@matthewjasper matthewjasper force-pushed the matthewjasper:drop-in-place-cgus branch from 60f7930 to e95280d Dec 16, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Dec 16, 2019

I've removed the normalize change - it appears to be a perf regression
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 16, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2019

⌛️ Trying commit e95280d with merge 293615e...

bors added a commit that referenced this pull request Dec 16, 2019
[PERF] Don't instantiate so many copies of real_drop_in_place

Split out from #66703.

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2019

☀️ Try build successful - checks-azure
Build commit: 293615e (293615e39bf757241e69cf382fb5ace10d043c6f)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 16, 2019

Queued 293615e with parent f0d4b57, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 17, 2019

Finished benchmarking try commit 293615e, comparison URL.

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 20, 2019

The job mingw-check of your PR failed (pretty log, 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-12-20T21:09:41.2137530Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-20T21:09:41.2331741Z ##[command]git config gc.auto 0
2019-12-20T21:09:41.2410382Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-20T21:09:41.2471531Z ##[command]git config --get-all http.proxy
2019-12-20T21:09:41.2629329Z ##[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/67332/merge:refs/remotes/pull/67332/merge
---
2019-12-20T21:17:21.8143791Z For more information about this error, try `rustc --explain E0308`.
2019-12-20T21:17:21.8478324Z error: could not compile `rustc`.
2019-12-20T21:17:21.8479122Z 
2019-12-20T21:17:21.8480128Z To learn more, run the command again with --verbose.
2019-12-20T21:17:21.8504868Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "-Zconfig-profile" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2019-12-20T21:17:21.8518796Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2019-12-20T21:17:21.8519021Z Build completed unsuccessfully in 0:04:31
2019-12-20T21:17:21.8576280Z == clock drift check ==
2019-12-20T21:17:21.8593248Z   local time: Fri Dec 20 21:17:21 UTC 2019
2019-12-20T21:17:21.8593248Z   local time: Fri Dec 20 21:17:21 UTC 2019
2019-12-20T21:17:22.0158000Z   network time: Fri, 20 Dec 2019 21:17:22 GMT
2019-12-20T21:17:22.0162132Z == end clock drift check ==
2019-12-20T21:17:22.8487711Z 
2019-12-20T21:17:22.8594233Z ##[error]Bash exited with code '1'.
2019-12-20T21:17:22.8623585Z ##[section]Starting: Checkout
2019-12-20T21:17:22.8625773Z ==============================================================================
2019-12-20T21:17:22.8625848Z Task         : Get sources
2019-12-20T21:17:22.8625897Z 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)

@matthewjasper matthewjasper force-pushed the matthewjasper:drop-in-place-cgus branch from f0b74ed to b231749 Dec 21, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Dec 21, 2019

...and I've brought it back when Reveal::All is set...
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 21, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

⌛️ Trying commit b231749 with merge 877fa2e...

bors added a commit that referenced this pull request Dec 21, 2019
[PERF] Don't instantiate so many copies of real_drop_in_place

Split out from #66703.

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

☀️ Try build successful - checks-azure
Build commit: 877fa2e (877fa2ef606af569c8f8d64f5722130f884790a3)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 21, 2019

Queued 877fa2e with parent c64eecf, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 21, 2019

Finished benchmarking try commit 877fa2e, comparison URL.

@matthewjasper matthewjasper force-pushed the matthewjasper:drop-in-place-cgus branch from b231749 to 1953a37 Dec 21, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Dec 21, 2019

I guess the first version of this was fine...

This is fairly hacky, but it gives some significant wins, albeit in a case (release incremental) that maybe isn't considered as important as others. (#67332 (comment))

cc @rust-lang/wg-compiler-performance - does anyone want to review this.

@matthewjasper matthewjasper force-pushed the matthewjasper:drop-in-place-cgus branch from 1953a37 to d8a8822 Dec 21, 2019
@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Feb 24, 2020

OK, I'm up for merging this, the performance wins look great!

There's one optional modification that you could make, which is putting drop-glue into the module of the corresponding type. That should be possible by modifying this case:

| ty::InstanceDef::DropGlue(..)

i.e. create the Instance via Instance::resolve_drop_in_place, extract the type param and use characteristic_def_id_of_type().

I'll leave it up to you whether you want to try this or just r=me the current state.

@bjorn3

This comment has been minimized.

Copy link
Contributor

bjorn3 commented Feb 24, 2020

The single field of InstanceDef::DropGlue already is the type for which the instance is the drop glue.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Feb 24, 2020

Oh that's right. I forgot about that. Good catch!

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 24, 2020

I tried putting drop glue into the type's module and found it to have worse (compiler) performance in the tests that I did.
@bors r=michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 24, 2020

📌 Commit d49ec09 has been approved by michaelwoerister

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 24, 2020

@bors rollup=never

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Feb 25, 2020

OK, sounds good then 👍

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Feb 26, 2020

@bors p=1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2020

⌛️ Testing commit d49ec09 with merge 036d888...

bors added a commit that referenced this pull request Feb 26, 2020
…rister

Don't instantiate so many copies of drop_in_place

Split out from #66703.

r? @ghost
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 26, 2020

The job i686-gnu-nopt of your PR failed (pretty log, 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.
2020-02-26T06:38:28.2830352Z ---- [codegen-units] codegen-units/partitioning/extern-drop-glue.rs stdout ----
2020-02-26T06:38:28.2830677Z 
2020-02-26T06:38:28.2830921Z The following items were assigned to wrong codegen units:
2020-02-26T06:38:28.2831135Z 
2020-02-26T06:38:28.2831437Z fn core::ptr[0]::drop_in_place[0]<cgu_extern_drop_glue::Struct[0]>
2020-02-26T06:38:28.2832065Z   expected: extern_drop_glue-fallback.cgu[Internal] 
2020-02-26T06:38:28.2832602Z   actual:   extern_drop_glue-fallback.cgu[External] 
2020-02-26T06:38:28.2833847Z thread '[codegen-units] codegen-units/partitioning/extern-drop-glue.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2463:13
2020-02-26T06:38:28.2834382Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2020-02-26T06:38:28.2834607Z 
2020-02-26T06:38:28.2835097Z ---- [codegen-units] codegen-units/partitioning/local-drop-glue.rs stdout ----
2020-02-26T06:38:28.2835097Z ---- [codegen-units] codegen-units/partitioning/local-drop-glue.rs stdout ----
2020-02-26T06:38:28.2835323Z 
2020-02-26T06:38:28.2835533Z The following items were assigned to wrong codegen units:
2020-02-26T06:38:28.2835740Z 
2020-02-26T06:38:28.2835977Z fn core::ptr[0]::drop_in_place[0]<local_drop_glue::Struct[0]>
2020-02-26T06:38:28.2836636Z   expected: local_drop_glue-fallback.cgu[Internal] 
2020-02-26T06:38:28.2837156Z   actual:   local_drop_glue-fallback.cgu[External] 
2020-02-26T06:38:28.2837968Z thread '[codegen-units] codegen-units/partitioning/local-drop-glue.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2463:13
2020-02-26T06:38:28.2838328Z 
2020-02-26T06:38:28.2838421Z 
2020-02-26T06:38:28.2838559Z failures:
---
2020-02-26T06:38:28.2840475Z 
2020-02-26T06:38:28.2840934Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:348:22
2020-02-26T06:38:28.2849272Z 
2020-02-26T06:38:28.2849548Z 
2020-02-26T06:38:28.2858410Z command did not execute successfully: "/checkout/obj/build/i686-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/i686-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/i686-unknown-linux-gnu/stage2/lib/rustlib/i686-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/i686-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/codegen-units" "--build-base" "/checkout/obj/build/i686-unknown-linux-gnu/test/codegen-units" "--stage-id" "stage2-i686-unknown-linux-gnu" "--mode" "codegen-units" "--target" "i686-unknown-linux-gnu" "--host" "i686-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/i686-unknown-linux-gnu/llvm/build/bin/FileCheck" "--host-rustcflags" "-Crpath -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "9.0.1-rust-1.43.0-nightly\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2020-02-26T06:38:28.2864300Z 
2020-02-26T06:38:28.2864620Z 
2020-02-26T06:38:28.2864968Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2020-02-26T06:38:28.2865319Z Build completed unsuccessfully in 1:36:13
2020-02-26T06:38:28.2865319Z Build completed unsuccessfully in 1:36:13
2020-02-26T06:38:28.2915193Z == clock drift check ==
2020-02-26T06:38:28.2930088Z   local time: Wed Feb 26 06:38:28 UTC 2020
2020-02-26T06:38:28.5789410Z   network time: Wed, 26 Feb 2020 06:38:28 GMT
2020-02-26T06:38:28.5789840Z == end clock drift check ==
2020-02-26T06:38:30.8039080Z 
2020-02-26T06:38:30.8145238Z ##[error]Bash exited with code '1'.
2020-02-26T06:38:30.8228082Z ##[section]Starting: Checkout rust-lang/rust@auto to s
2020-02-26T06:38:30.8234089Z ==============================================================================
2020-02-26T06:38:30.8234513Z Task         : Get sources
2020-02-26T06:38:30.8234965Z 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)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2020

💔 Test failed - checks-azure

@matthewjasper matthewjasper force-pushed the matthewjasper:drop-in-place-cgus branch from d49ec09 to b944531 Feb 26, 2020
@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

matthewjasper commented Feb 26, 2020

@bors r=michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2020

📌 Commit b944531 has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 27, 2020

⌛️ Testing commit b944531 with merge d28560e...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 27, 2020

☀️ Test successful - checks-azure
Approved by: michaelwoerister
Pushing d28560e to master...

@bors bors added the merged-by-bors label Feb 27, 2020
@bors bors merged commit d28560e into rust-lang:master Feb 27, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200226.34 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@matthewjasper matthewjasper deleted the matthewjasper:drop-in-place-cgus branch Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.