Skip to content

Describe why size_align have not been inlined so far - #79827

Merged
bors merged 1 commit into
rust-lang:masterfrom
tmiasko:size-align
Jan 3, 2021
Merged

Describe why size_align have not been inlined so far#79827
bors merged 1 commit into
rust-lang:masterfrom
tmiasko:size-align

Conversation

@tmiasko

@tmiaskotmiasko commented Dec 8, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

although it is used only in one place.

@rust-highfive

rust-highfive commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

r? @kennytm

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

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

Xanewok commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Dec 8, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit dc7325e411910d8f325af84c8ad162408249ffa7 has been approved by Xanewok

@borsbors 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 Dec 8, 2020
@kennytm

kennytm commented Dec 8, 2020

Copy link
Copy Markdown
Member

@bors r-

we tried once before in #72189 and failed, please show what has changed this time.

@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 8, 2020
@kennytm

kennytm commented Dec 8, 2020

Copy link
Copy Markdown
Member

@bors rollup=never

@Xanewok

Xanewok commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

Oh, woops, sorry for the noise. It does look innocuous; might be worth adding a comment for posterity on why it's perf-sensitive/the way it is right now.

@Mark-Simulacrum

Mark-Simulacrum commented Dec 8, 2020

Copy link
Copy Markdown
Member

r=me with a comment added, fwiw

@Mark-Simulacrum

Mark-Simulacrum commented Dec 8, 2020

Copy link
Copy Markdown
Member

(i.e., a comment instead of the current change)

@Xanewok

Xanewok commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

For completeness' sake:

@bors try @rust-timer queue

@rust-timer

rust-timer commented Dec 9, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@bors

bors commented Dec 9, 2020

Copy link
Copy Markdown
Collaborator

⌛ Trying commit dc7325e411910d8f325af84c8ad162408249ffa7 with merge 69c935bbedf41328fef53fd877e4a48df302d2f5...

@bors

bors commented Dec 9, 2020

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Dec 9, 2020

Copy link
Copy Markdown
Collaborator

Queued 69c935bbedf41328fef53fd877e4a48df302d2f5 with parent db85512, future comparison URL.

@rust-timer

rust-timer commented Dec 9, 2020

Copy link
Copy Markdown
Collaborator

Finished benchmarking try commit (69c935bbedf41328fef53fd877e4a48df302d2f5): 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 modify labels: +S-waiting-on-review -S-waiting-on-perf

@rustbotrustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2020
@tmiasko

tmiasko commented Dec 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks for catching this @kennytm. What prompted me to made this change in the first place, was the number of codegen items created for size_align. In retrospect, it is not surprising that it could cause a shift in partitioning. While partitioning is generally stable, when it finally changes, the change tends to be substantial. Looking again at mono-items and their partitioning in a crate I was investigating, this indeed introduced quite a shift.

Given the number of codegen item, the effect on partitioning likely generalizes, although I see no reason to except impact on quality of produced code to be in any particular direction, but since this is not captured by rustc perf benchmarks (except for rustc), leaving this as is seems like a good decision for now.

I added a comment describing why the function wasn't inlined so far.

@tmiaskotmiasko changed the title Inline implementation of size_alignDescribe why size_align have not been inlined so farDec 9, 2020
@kennytm

kennytm commented Dec 9, 2020

Copy link
Copy Markdown
Member

@bors rollup

kennytm
kennytm reviewed Dec 9, 2020
View reviewed changes

@kennytmkennytm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

r=me or Mark-Simulacrum after fixing comment

Comment threadlibrary/core/src/alloc/layout.rs Outdated
@bjorn3

bjorn3 commented Jan 2, 2021

Copy link
Copy Markdown
Member

I wonder if calling the size_of and align_of intrinsics directly instead of the core::mem wrappers would fix the perf regression when inlining size_align.

@kennytm

kennytm commented Jan 2, 2021

Copy link
Copy Markdown
Member

@bors r+ rollup

@bors

bors commented Jan 2, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit cf5bd26 has been approved by kennytm

@borsbors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2021
@bors

bors commented Jan 3, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit cf5bd26 with merge 05dfaba...

@bors

bors commented Jan 3, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 05dfaba to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jan 3, 2021
@bors
bors merged commit 05dfaba into rust-lang:masterJan 3, 2021
@rustbotrustbot added this to the 1.51.0 milestone Jan 3, 2021
@tmiasko
tmiasko deleted the size-align branch January 3, 2021 10:59
@CAD97CAD97 mentioned this pull request Mar 31, 2022
Merged
@scottmcmscottmcm mentioned this pull request Jun 23, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
@bors
Auto merge ofrust-lang#126871- scottmcm:layout-const-for-type, r=<try>
f79e592
Make `Layout::new::<T>()` a constant instead of multiple NullOps
rust-lang#72189 and rust-lang#79827 suggest that this is perf-relevant, so let's see what happens.
r? ghost
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kennytmkennytmkennytm left review comments

Assignees

@kennytmkennytm

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.51.0

Development

Successfully merging this pull request may close these issues.

9 participants

@tmiasko@rust-highfive@Xanewok@bors@kennytm@Mark-Simulacrum@rust-timer@bjorn3@rustbot