Use min_specialization in the remaining rustc crates #72707
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matthewjasper
force-pushed
the
rustc_min_spec
branch
from
9217821
to
dc4d551
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matthewjasper
force-pushed
the
rustc_min_spec
branch
from
dc4d551
to
21eab5b
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matthewjasper
force-pushed
the
rustc_min_spec
branch
from
21eab5b
to
cefb703
Compare
|
☔ The latest upstream changes (presumably #72778) made this pull request unmergeable. Please resolve the merge conflicts. |
matthewjasper
force-pushed
the
rustc_min_spec
branch
from
cefb703
to
596afe0
Compare
|
☔ The latest upstream changes (presumably #72927) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Is there anyone in @rust-lang/compiler willing to review this? |
oli-obk
reviewed
Jun 9, 2020
| @@ -228,8 +228,13 @@ impl<'tcx> SpecializedEncoder<LocalDefId> for EncodeContext<'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'tcx> SpecializedEncoder<Ty<'tcx>> for EncodeContext<'tcx> { | |||
| fn specialized_encode(&mut self, ty: &Ty<'tcx>) -> Result<(), Self::Error> { | |||
| impl<'a, 'b, 'tcx> SpecializedEncoder<&'a ty::TyS<'b>> for EncodeContext<'tcx> | |||
There was a problem hiding this comment.
are the two separate lifetimes 'a and 'b really necessary here? Couldn't they just be one and use Ty<'a>?
There was a problem hiding this comment.
Unfortunately not. Specializing impls can't repeat type/lifetime parameters.
oli-obk
reviewed
Jun 9, 2020
| &'a ty::TyS<'b>: UseSpecializedEncodable, | ||
| { | ||
| fn specialized_encode(&mut self, ty: &&'a ty::TyS<'b>) -> Result<(), Self::Error> { | ||
| debug_assert!(self.tcx.lift(ty).is_some()); |
There was a problem hiding this comment.
Is there a measurable perf loss to just doing lift and unwrap and skipping the transmute?
There was a problem hiding this comment.
Ok, looks like there are some serious losses here. Let's benchmark against the transmute variant and if that has no perf losses, let's merge it.
oli-obk
reviewed
Jun 9, 2020
| ) -> Result<(), Self::Error> { | ||
| ty_codec::encode_spanned_predicates(self, predicates, |ecx| &mut ecx.predicate_shorthands) | ||
| debug_assert!(predicates.iter().all(|(p, _)| self.tcx.lift(p.kind()).is_some())); |
There was a problem hiding this comment.
I think you can just lift the entire borrowed slice instead of each element
I agree. r? @oli-obk r=me with the comments resolved in some way |
matthewjasper
force-pushed
the
rustc_min_spec
branch
from
596afe0
to
09a2e80
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 09a2e80cc4266f81c7d60516f8eff5eb92093b7f with merge 70c770a6c3a5241ab4f65e6841ecd641c983a4fb... |
|
☀️ Try build successful - checks-azure |
|
Queued 70c770a6c3a5241ab4f65e6841ecd641c983a4fb with parent feb3536, future comparison URL. |
|
Finished benchmarking try commit (70c770a6c3a5241ab4f65e6841ecd641c983a4fb): comparison url. |
|
Awaiting bors try build completion |
|
⌛ Trying commit 88ea7e5 with merge 3b49dc19913ecb228b5ce64932615370ac1ebc35... |
|
One thing I want to mention: The specialization hack that we use was meant to be replaced with a custom trait for serialization, but that was blocked on the ability to use custom derive -- now that we have custom derive, couldn't we make the encodable trait carry a Also, how much is any of this documented? I cast my eye over the diff but I admit I've always found the encoding system rather confusing. |
|
💥 Test timed out |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 88ea7e5 with merge d5dd610915fff92af707dd35f4a864570eaa8520... |
|
I've started working on safe serialization traits, but it's going to be a large change affecting a lot of code, so I think it makes sense to have it separated. |
|
☀️ Try build successful - checks-azure |
|
Queued d5dd610915fff92af707dd35f4a864570eaa8520 with parent bb86748, future comparison URL. |
|
Finished benchmarking try commit (d5dd610915fff92af707dd35f4a864570eaa8520): comparison url. |
|
@bors r=oli-obk |
|
📌 Commit 88ea7e5 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- r=oli-obk This wasn't showing in the queue at https://buildbot2.rust-lang.org/homu/queue/rust |
bors
added
S-waiting-on-author
|
📌 Commit 88ea7e5 has been approved by |
bors
added
S-waiting-on-bors
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Jun 15, 2020
…-obk Use min_specialization in the remaining rustc crates This adds a lot of `transmute` calls to replace the unsound uses of specialization. It's ugly, but at least it's honest about what's going on. cc rust-lang#71420, @RalfJung
This was referenced Jun 15, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 15, 2020
Rollup of 10 pull requests Successful merges: - rust-lang#72707 (Use min_specialization in the remaining rustc crates) - rust-lang#72740 (On recursive ADT, provide indirection structured suggestion) - rust-lang#72879 (Miri: avoid tracking current location three times) - rust-lang#72938 (Stabilize Option::zip) - rust-lang#73086 (Rename "cyclone" to "apple-a7" per changes in upstream LLVM) - rust-lang#73104 (Example about explicit mutex dropping) - rust-lang#73139 (Add methods to go from a nul-terminated Vec<u8> to a CString) - rust-lang#73296 (Remove vestigial CI job msvc-aux.) - rust-lang#73304 (Revert heterogeneous SocketAddr PartialEq impls) - rust-lang#73331 (extend network support for HermitCore) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a lot of
transmutecalls to replace the unsound uses of specialization.It's ugly, but at least it's honest about what's going on.
cc #71420, @RalfJung