Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Don't compute inlining status of mono items in advance. - #112128

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance
Jun 1, 2023
Merged

Don't compute inlining status of mono items in advance.#112128
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:no-inlining-advance

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

We record inlining status for mono items in MonoItems, and then transfer it to InliningMap, for later use in
InliningMap::with_inlining_candidates.

But we can just compute inlining status directly in InliningMap::with_inlining_candidates, because the mono item is right there. There's no need to compute it in advance.

This commit changes the code to do that, removing the need for MonoItems and InliningMap::inlines. This does result in more calls to instantiation_mode (one per static occurrence) but the performance effect is negligible.

r? @wesleywiser

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@nnethercote

nnethercote commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

I expect this to have no perf impact, but let's check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 9571d670c36e1e17f3b2351c11fa8cc539260e35 with merge cc638c2ed8ff2a74362f50eb1e9467496484182d...

@nnethercotennethercote mentioned this pull request May 31, 2023
@rust-timer

This comment has been minimized.

@SparrowLii

Copy link
Copy Markdown
Member

This is very nice if it has little perf impact

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

1 similar comment
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (cc638c2ed8ff2a74362f50eb1e9467496484182d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
5.5%[5.5%, 5.5%]1
Regressions ❌
(secondary)
2.0%[2.0%, 2.0%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-4.4%[-4.4%, -4.4%]1
All ❌✅ (primary)5.5%[5.5%, 5.5%]1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
2.5%[2.5%, 2.5%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)2.5%[2.5%, 2.5%]1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.402s -> 642.265s (-0.18%)

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 31, 2023
We record inlining status for mono items in `MonoItems`, and then
transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in
`InliningMap::with_inlining_candidates`, because the mono item is right
there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for
`MonoItems` and `InliningMap::inlines`. This does result in more calls
to `instantiation_mode` (one per static occurrence) but the performance
effect is negligible.
@nnethercote

Copy link
Copy Markdown
ContributorAuthor

No perf impact. @wesleywiser, ready for review, thanks!

@bors rollup

@wesleywiserwesleywiser 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.

Very nice!

@wesleywiser

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit cc21d9a has been approved by wesleywiser

It is now in the queue for this repository.

@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 Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…=wesleywiser
Don't compute inlining status of mono items in advance.
We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in
`InliningMap::with_inlining_candidates`.
But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance.
This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible.
r? `@wesleywiser`
@Dylan-DPCDylan-DPC mentioned this pull request Jun 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests
Successful merges:
- rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
- rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
- rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
- rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
- rust-lang#112024 (Don't suggest break through nested items)
- rust-lang#112128 (Don't compute inlining status of mono items in advance.)
- rust-lang#112141 (remove reference to Into in ? operator core/std docs, fixrust-lang#111655)
Failed merges:
- rust-lang#112071 (Group rfcs tests)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit fe12323 into rust-lang:masterJun 1, 2023
@rustbotrustbot added this to the 1.72.0 milestone Jun 1, 2023
@nnethercote
nnethercote deleted the no-inlining-advance branch June 1, 2023 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@SparrowLii@wesleywiser@rustbot