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.

Sign up for GitHub

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

Jump to bottom

Box thir::ExprKind::Adt for performance #86266

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Box thir::ExprKind::Adt for performance #86266

merged 1 commit into from
Jun 16, 2021
+43 −26

Conversation

LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Jun 13, 2021 •
edited
Loading

Adt is the biggest variant in the enum and probably isn't used very often compared to the other expr kinds, so boxing it should be beneficial for performance. We need a perf test to be sure.

@LeSeulArtichaut LeSeulArtichaut added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 13, 2021
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 13, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 13, 2021
@bors
Copy link
Contributor

bors commented Jun 13, 2021

⌛ Trying commit 42f11b32fc40cf11cc6f18abd58f0f47bbcb39a4 with merge 2a510203a71897c9c924e66fa49eb444dabc13c3...

@bors
Copy link
Contributor

bors commented Jun 13, 2021

☀️ Try build successful - checks-actions
Build commit: 2a510203a71897c9c924e66fa49eb444dabc13c3 (2a510203a71897c9c924e66fa49eb444dabc13c3)

@rust-timer
Copy link
Collaborator

Queued 2a510203a71897c9c924e66fa49eb444dabc13c3 with parent 6cc5d54, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2a510203a71897c9c924e66fa49eb444dabc13c3): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 13, 2021
@LeSeulArtichaut LeSeulArtichaut changed the title Box ExprKind::Adt Box the largest variants of thir::ExprKind Jun 13, 2021
@LeSeulArtichaut
Copy link
Contributor Author

I'd like to try boxing Literal and Closure which are the two next largest variants.

@jyn514
Copy link
Member

jyn514 commented Jun 13, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 13, 2021
@bors
Copy link
Contributor

bors commented Jun 13, 2021

⌛ Trying commit 7ba16512599d9e491b2035de106185b0f1c338e6 with merge cebaf13eee5bbb1bf05df1339bcddf37ce4288a1...

@bors
Copy link
Contributor

bors commented Jun 13, 2021

☀️ Try build successful - checks-actions
Build commit: cebaf13eee5bbb1bf05df1339bcddf37ce4288a1 (cebaf13eee5bbb1bf05df1339bcddf37ce4288a1)

@rust-timer
Copy link
Collaborator

Queued cebaf13eee5bbb1bf05df1339bcddf37ce4288a1 with parent fb3ea63, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (cebaf13eee5bbb1bf05df1339bcddf37ce4288a1): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 13, 2021
@LeSeulArtichaut LeSeulArtichaut changed the title Box the largest variants of thir::ExprKind Box thir::ExprKind::Adt for performance Jun 13, 2021
davidtwco
davidtwco approved these changes Jun 16, 2021
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a mild improvement that is worth landing, looks good to me.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 16, 2021

📌 Commit 5e802e5 has been approved by davidtwco

@bors 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 Jun 16, 2021
@marmeladema
Copy link
Contributor

Perf were better on the first run no?

@jyn514
Copy link
Member

jyn514 commented Jun 16, 2021

@marmeladema yes, @LeSeulArtichaut reverted to that commit.

@bors
Copy link
Contributor

bors commented Jun 16, 2021

⌛ Testing commit 5e802e5 with merge 5b60db2f4cb8a914e0c1041fc66e6a4096377b75...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMuHVPZqhT/q

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536
---

error: failed to run custom build command for `libz-sys v1.1.2`

Caused by:
  process didn't exit successfully: `/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-tools/release/build/libz-sys-6a8089f3ba64501c/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-env-changed=LIBZ_SYS_STATIC
  cargo:rerun-if-changed=build.rs
  TARGET = Some("x86_64-apple-darwin")
  OPT_LEVEL = Some("3")
  HOST = Some("x86_64-apple-darwin")
  CC_x86_64-apple-darwin = Some("sccache /Users/runner/work/rust/rust/clang+llvm-12.0.0-x86_64-apple-darwin/bin/clang")
  CFLAGS_x86_64-apple-darwin = Some("-ffunction-sections -fdata-sections -fPIC --target=x86_64-apple-darwin -stdlib=libc++ -fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/5b60db2f4cb8a914e0c1041fc66e6a4096377b75")
  CRATE_CC_NO_DEFAULTS = None
  DEBUG = Some("false")
  running: "sccache" "/Users/runner/work/rust/rust/clang+llvm-12.0.0-x86_64-apple-darwin/bin/clang" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-stdlib=libc++" "-fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/5b60db2f4cb8a914e0c1041fc66e6a4096377b75" "-I" "src/zlib" "-fvisibility=hidden" "-DSTDC" "-D_LARGEFILE64_SOURCE" "-D_POSIX_SOURCE" "-D_C99_SOURCE" "-o" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-tools/x86_64-apple-darwin/release/build/libz-sys-9be647c09c509bd3/out/build/src/zlib/adler32.o" "-c" "src/zlib/adler32.c"
  cargo:warning=error: Connection to server timed out
  exit status: 2
  running: "sccache" "/Users/runner/work/rust/rust/clang+llvm-12.0.0-x86_64-apple-darwin/bin/clang" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-stdlib=libc++" "-fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/5b60db2f4cb8a914e0c1041fc66e6a4096377b75" "-I" "src/zlib" "-fvisibility=hidden" "-DSTDC" "-D_LARGEFILE64_SOURCE" "-D_POSIX_SOURCE" "-D_C99_SOURCE" "-o" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-tools/x86_64-apple-darwin/release/build/libz-sys-9be647c09c509bd3/out/build/src/zlib/compress.o" "-c" "src/zlib/compress.c"

  --- stderr



  error occurred: Command "sccache" "/Users/runner/work/rust/rust/clang+llvm-12.0.0-x86_64-apple-darwin/bin/clang" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-stdlib=libc++" "-fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/5b60db2f4cb8a914e0c1041fc66e6a4096377b75" "-I" "src/zlib" "-fvisibility=hidden" "-DSTDC" "-D_LARGEFILE64_SOURCE" "-D_POSIX_SOURCE" "-D_C99_SOURCE" "-o" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage1-tools/x86_64-apple-darwin/release/build/libz-sys-9be647c09c509bd3/out/build/src/zlib/adler32.o" "-c" "src/zlib/adler32.c" with args "clang" did not execute successfully (status code exit status: 2).

warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] syn test:false 85.643
[RUSTC-TIMING] serde test:false 8.814

@bors
Copy link
Contributor

bors commented Jun 16, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 16, 2021
@LeSeulArtichaut
Copy link
Contributor Author

Looks spurious. @bors retry

@bors 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 Jun 16, 2021
@bors
Copy link
Contributor

bors commented Jun 16, 2021

⌛ Testing commit 5e802e5 with merge a85f584...

@bors
Copy link
Contributor

bors commented Jun 16, 2021

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing a85f584 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2021
@bors bors merged commit a85f584 into rust-lang:master Jun 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 16, 2021
@bors bors mentioned this pull request Jun 16, 2021
Merged
@LeSeulArtichaut LeSeulArtichaut deleted the box-thir-adt branch June 17, 2021 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
10 participants
@LeSeulArtichaut @rust-highfive @jyn514 @rust-timer @bors @davidtwco @marmeladema @rust-log-analyzer @bjorn3 @rustbot