Skip to content

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@scottmcm@rust-timer@bors@the8472@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Simplify `Vec` using `iter::repeat_n` by scottmcm · Pull Request #104596 · rust-lang/rust · GitHub
Skip to content

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@scottmcm@rust-timer@bors@the8472@rustbot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Simplify `Vec` using `iter::repeat_n` by scottmcm · Pull Request #104596 · rust-lang/rust · GitHub
Skip to content

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Simplify Vec using iter::repeat_n - #104596

Closed
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals
Closed

Simplify Vec using iter::repeat_n#104596
scottmcm wants to merge 4 commits into
rust-lang:masterfrom
scottmcm:simplify-vec-internals

Conversation

@scottmcm

@scottmcmscottmcm commented Nov 19, 2022

Copy link
Copy Markdown
Member

By using repeat_n instead of ExtendElement, we can delete a bunch more unsafe code from Vec.

All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.

r? @ghost

Built atop #104435; will leave draft until I can rebase atop that.

Today it always copies it for *every* appended element, but one of those clones is avoidable.
By using `repeat_n` instead of `ExtendElement`, we can delete a bunch more `unsafe` code from `Vec.
All the codegen tests are passing (and I made one stricter to help be sure), so let's see if perf is also happy with it.
@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 19, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 50b394a with merge 30c97e84bdbff017290f6d12db34df2f225359a8...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (30c97e84bdbff017290f6d12db34df2f225359a8): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.4%[0.2%, 0.7%]24
Regressions ❌
(secondary)
2.3%[0.2%, 5.5%]15
Improvements ✅
(primary)
-0.8%[-1.0%, -0.6%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.3%[-1.0%, 0.7%]26

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)
1.8%[0.4%, 3.8%]5
Regressions ❌
(secondary)
1.7%[1.2%, 2.5%]3
Improvements ✅
(primary)
-4.6%[-4.6%, -4.6%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-4.6%, 3.8%]6

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)
0.8%[0.7%, 0.9%]3
Regressions ❌
(secondary)
5.0%[4.5%, 5.8%]6
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-5.3%[-5.3%, -5.3%]1
All ❌✅ (primary)0.8%[0.7%, 0.9%]3

@rustbotrustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 19, 2022
@the8472

Copy link
Copy Markdown
Member

I'm not sure if it's a good idea to remove this. There's untapped optimization potential in the current implementation. E.g. it could use the zero-value check that vec initialization already has and then use Allocator::grow_zeroed.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@the8472 I agree that's a good thing to add, but I don't think it needs the ExtendWithtrait, since we can't do that for ExtendFunc anyway. I could leave an private extend_element function without the trait and type, though, if you think that'd be helpful.

@scottmcm

Copy link
Copy Markdown
MemberAuthor

@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 Nov 19, 2022
@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 78f2356 with merge d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa...

@bors

bors commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d6ba54d913266c13b4ddf2eaf8b1b9b023df4caa): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.5%[0.2%, 1.2%]76
Regressions ❌
(secondary)
1.8%[0.2%, 6.6%]30
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-0.2%[-0.2%, -0.2%]1
All ❌✅ (primary)0.5%[0.2%, 1.2%]76

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)
1.1%[0.4%, 2.4%]6
Regressions ❌
(secondary)
1.5%[1.5%, 1.5%]1
Improvements ✅
(primary)
-4.8%[-4.8%, -4.8%]1
Improvements ✅
(secondary)
-3.7%[-5.3%, -2.1%]2
All ❌✅ (primary)0.3%[-4.8%, 2.4%]7

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)
0.9%[0.8%, 1.0%]6
Regressions ❌
(secondary)
2.5%[1.3%, 3.7%]8
Improvements ✅
(primary)
-1.4%[-1.9%, -0.9%]9
Improvements ✅
(secondary)
-2.8%[-5.2%, -2.1%]13
All ❌✅ (primary)-0.5%[-1.9%, 1.0%]15

@rustbotrustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Curious -- arguably good on cycles, but clearly bad on instructions.

I guess I should do this in smaller pieces, if at all.

@scottmcm
scottmcm deleted the simplify-vec-internals branch November 20, 2022 01:02
scottmcm added a commit to scottmcm/rust that referenced this pull request Nov 20, 2022
Repeating is `TrustedLen`, so we don't need another copy of [the `TrustedLen` specialization for `extend`](https://github.com/rust-lang/rust/blob/c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b/library/alloc/src/vec/spec_extend.rs#L27).
(This is a simpler version of rust-lang#104596, which tried to do too many things at once.)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regressionPerformance regression.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@scottmcm@rust-timer@bors@the8472@rustbot