Skip to content

Conversation

cjgillot
Copy link
Contributor

Extracted from #83036 and #82780.

@rust-highfive
Copy link
Contributor

r? @lcnr

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2021
@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 18, 2021
@bors
Copy link
Collaborator

bors commented Mar 18, 2021

⌛ Trying commit ccef537b0366d1ced16cf212d385aa1707a5794f with merge bf696c7a499ba76acd374ad810be4e05f17460b9...

@bors
Copy link
Collaborator

bors commented Mar 18, 2021

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

@rust-timer
Copy link
Collaborator

Queued bf696c7a499ba76acd374ad810be4e05f17460b9 with parent 9d0446f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (bf696c7a499ba76acd374ad810be4e05f17460b9): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 18, 2021
@joshtriplett
Copy link
Member

This provides a decent improvement on max-rss, but regresses on instructions.

@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 18, 2021
@bors
Copy link
Collaborator

bors commented Mar 18, 2021

⌛ Trying commit 637253f5263e9d8abf2b522553c64ab80d53f358 with merge 0f5649699bc49e9345864d7b99c11697dd8506f3...

@bors
Copy link
Collaborator

bors commented Mar 18, 2021

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

@rust-timer
Copy link
Collaborator

Queued 0f5649699bc49e9345864d7b99c11697dd8506f3 with parent 1705a7d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0f5649699bc49e9345864d7b99c11697dd8506f3): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 19, 2021
@joshtriplett
Copy link
Member

Still appears to be a substantial regression, unfortunately.

@michaelwoerister
Copy link
Member

I've done a few similar things in c49297d (part of #82183). I did not see performance regressions there. Maybe that's of interest.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 19, 2021
@bors
Copy link
Collaborator

bors commented Mar 19, 2021

⌛ Trying commit 11b3409 with merge 58740d89a14c455cd96c58a6dc4a3478d8cb854c...

@bors
Copy link
Collaborator

bors commented Mar 19, 2021

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

@rust-timer
Copy link
Collaborator

Queued 58740d89a14c455cd96c58a6dc4a3478d8cb854c with parent 9f4bc3e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (58740d89a14c455cd96c58a6dc4a3478d8cb854c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@bors
Copy link
Collaborator

bors commented Mar 20, 2021

@rust-timer: 🔑 Insufficient privileges: not in try users

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 20, 2021
@Aaron1011
Copy link
Contributor

cc @rust-lang/infra ^ - it looks like the permissions for rust-timer are messed up somehow.

@bjorn3
Copy link
Member

bjorn3 commented Mar 20, 2021

Improvements up to 1.5%.

@lcnr
Copy link
Contributor

lcnr commented Mar 20, 2021

can someone else take this over, I don't have a lot of capacity rn

r? @michaelwoerister maybe, feel free to reassign

@michaelwoerister
Copy link
Member

Looks good to me. I'm still not a fan of all that unsafe code and most of it can be removed as c49297d demonstrates.

I'd also prefer if read_raw_bytes would just borrow the underlying data instead of copying it to &mut [MaybeUninit<u8>]. That works with the opaque decoder but can't be done if read_raw_bytes is moved to the Decoder trait, as this PR does. I hope we can make that change once we get rid of JSON encoding as rust-lang/compiler-team#418 proposes.

But for now this PR is still a strict improvement.

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 22, 2021

📌 Commit 11b3409 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2021
@bors
Copy link
Collaborator

bors commented Mar 22, 2021

⌛ Testing commit 11b3409 with merge d04c3aa...

@bors
Copy link
Collaborator

bors commented Mar 22, 2021

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing d04c3aa to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2021
@bors bors merged commit d04c3aa into rust-lang:master Mar 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 22, 2021
@cjgillot cjgillot deleted the endecode branch March 22, 2021 17:31
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2021
… r=cjgillot

Allow for reading raw bytes from rustc_serialize::Decoder without unsafe code

The current `read_raw_bytes` method requires using `MaybeUninit` and `unsafe`. I don't think this is necessary. Let's see if a safe interface has any performance drawbacks.

This is a followup to rust-lang#83273 and will make it easier to rebase rust-lang#82183.

r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.