Skip to content

Revert "Assume slice len is bounded by allocation size" - #77771

Merged
bors merged 1 commit into
rust-lang:masterfrom
nagisa:revert-77023
Oct 10, 2020
Merged

Revert "Assume slice len is bounded by allocation size"#77771
bors merged 1 commit into
rust-lang:masterfrom
nagisa:revert-77023

Conversation

@nagisa

@nagisanagisa commented Oct 9, 2020

Copy link
Copy Markdown
Member

#77023 (comment)
suggests that the original PR introduced a significant perf regression.

This reverts commit e44784b / #77023.

cc @HeroicKatora

@rust-highfive

rust-highfive commented Oct 9, 2020

Copy link
Copy Markdown
Contributor

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

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

nagisa commented Oct 9, 2020

Copy link
Copy Markdown
MemberAuthor

r? @Mark-Simulacrum

@Mark-Simulacrum

Mark-Simulacrum commented Oct 9, 2020

Copy link
Copy Markdown
Member

r=me, but looks like other stuff started using the features so we'll need to retain them

@nagisa
Revert "Assume slice len is bounded by allocation size"
54a5608
rust-lang#77023 (comment)
suggests that the original PR introduced a significant perf regression.
This reverts commit e44784b / rust-lang#77023.
@nagisa

nagisa commented Oct 9, 2020

Copy link
Copy Markdown
MemberAuthor

Huh, I force pushed but github is taking its time catching it...

@nagisa

nagisa commented Oct 9, 2020

Copy link
Copy Markdown
MemberAuthor

@bors r=Mark-Simulacrum

@bors

bors commented Oct 9, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 54a5608 has been approved by Mark-Simulacrum

@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 Oct 9, 2020
@camelidcamelid added A-slice Area: `[T]` T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api [DEPRECATED; DO NOT USE] labels Oct 10, 2020
@bors

bors commented Oct 10, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 54a5608 with merge 87b71ed...

@bors

bors commented Oct 10, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 87b71ed to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Oct 10, 2020
@bors
bors merged commit 87b71ed into rust-lang:masterOct 10, 2020
@rustbotrustbot added this to the 1.49.0 milestone Oct 10, 2020
@Mark-Simulacrum

Mark-Simulacrum commented Oct 13, 2020

Copy link
Copy Markdown
Member

perf results on landing are about as expected; a small win. There are some regressions too but nothing too large and on recently-added benchmarks. I would not worry about them right now.

  • Moderate improvement in instruction counts (up to -2.5% on incr-patched: Job builds of regex-opt)
  • Moderate regression in instruction counts (up to 1.3% on full builds of match-stress-enum-check)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-SimulacrumMark-SimulacrumAwaiting requested review from Mark-Simulacrum
@ecstatic-morseecstatic-morseAwaiting requested review from ecstatic-morse

Assignees

@Mark-SimulacrumMark-Simulacrum

Labels

A-sliceArea: `[T]`merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libsRelevant to the library team, which will review and decide on the PR/issue.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Milestone

1.49.0

Development

Successfully merging this pull request may close these issues.

7 participants

@nagisa@rust-highfive@Mark-Simulacrum@bors@kennytm@camelid@rustbot