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

Reduce size of hir::Expr by boxing more of hir::InlineAsm #66515

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 18, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2019
@Centril
Copy link
Contributor Author

Centril commented Nov 18, 2019

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 18, 2019

⌛ Trying commit 9b7bed080fc3381be02de27bb7167be3a7a74221 with merge 03d05b28688feb26964e046893ba71f5bf7fd69b...

@bors
Copy link
Contributor

bors commented Nov 18, 2019

☀️ Try build successful - checks-azure
Build commit: 03d05b28688feb26964e046893ba71f5bf7fd69b (03d05b28688feb26964e046893ba71f5bf7fd69b)

@rust-timer
Copy link
Collaborator

Queued 03d05b28688feb26964e046893ba71f5bf7fd69b with parent a0d40f8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 03d05b28688feb26964e046893ba71f5bf7fd69b, comparison URL.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 19, 2019

Unsure if this has had an effect on max-rss, but if it has, it's an improvement (loads of green in https://perf.rust-lang.org/compare.html?start=a0d40f8bdfcc3c28355467973f97fd4c45ac5876&end=03d05b28688feb26964e046893ba71f5bf7fd69b&stat=max-rss)

@Centril
Copy link
Contributor Author

Centril commented Nov 19, 2019

Yeah that's my thinking also; cc @nnethercote

@@ -2074,6 +2072,13 @@ pub struct InlineAsm {
pub dialect: AsmDialect,
}

#[derive(RustcEncodable, RustcDecodable, Debug, HashStable)]
pub struct InlineAsm {
pub inner: InlineAsmInner,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any need to have InlineAsmInner? Could we just have asm, asm_str_style, outputs, and dialect fields directly in InlineAsm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but wasn't able to do that; Expr is not Clone and InlineAsmInner needs to be Clone.

@nnethercote
Copy link
Contributor

max-rss is very noisy so it's hard to know if this effect is real. The change certainly can't hurt, though, especially given that the InlineAsm variant is so rarely used.

@Centril
Copy link
Contributor Author

Centril commented Nov 21, 2019

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Nov 21, 2019

📌 Commit 44cebe5 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Nov 21, 2019

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

@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 Nov 21, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 21, 2019
Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Nov 21, 2019
Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Nov 21, 2019
Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`

r? @oli-obk
bors added a commit that referenced this pull request Nov 21, 2019
Rollup of 5 pull requests

Successful merges:

 - #65355 (Stabilize `!` in Rust 1.41.0)
 - #65730 (Suggest to add lifetime constraint at explicit ouput of functions)
 - #66468 (Cleanup Miri SIMD intrinsics)
 - #66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
 - #66602 (Revert "Update Source Code Pro and include italics")

Failed merges:

r? @ghost
@bors bors merged commit 44cebe5 into rust-lang:master Nov 21, 2019
@Centril Centril deleted the cheaper-inline-asm branch November 21, 2019 17:53
flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request Nov 22, 2019
flip1995 added a commit to Manishearth/rust-clippy that referenced this pull request Nov 22, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants