Add scoped thread-local encoding and decoding contexts to cstore. #30140
Conversation
|
AST autoserialization causes a ton of metadata bloat and a decent amount of compile-time bloat. I am not sure I would like to stay with MIR autoserialization if that would be a problem. I am not aware of the current astencode manual serialization causing problems (I looked at that topic and it looks like the most annoying part in serializing MIR is dealing with |
|
@arielb1: I'm improving |
|
Sorry for not responding earlier, was ill last week. Some thoughts:
|
|
I don't recall having maintainability problems with the old astencode manual-decoding code - it mostly just works. Our MIR format is not really supposed to rapidly change.
|
michaelwoerister
force-pushed
the
tls-encoding
branch
2 times, most recently
from
0b1e599 to
e254867
Compare
|
I'm pretty happy with this PR. Writing serialization code is painful --- not so much a source of bugs (though I've had a few from tyencode), but really annoying, and to little purpose. I don't think autogenerated serialization needs to be inefficienct. Any inefficiences we see can easily be overcome by implementing serialization by hand for a particular type -- you can then use the autogenerated code as a building block, by writing things like // serialize:
serialize((&self.foo, &self.bar)); // serialize first 2 fields, but not self.span
// deserialize:
let (foo, bar) = deserialize();
MyType { foo: foo, bar: bar, span: DUMMY_SP } |
|
@bors r+ |
|
📌 Commit e254867 has been approved by |
|
⌛ Testing commit e254867 with merge 56670b1... |
|
💔 Test failed - auto-win-msvc-64-opt |
With this commit, metadata encoding and decoding can make use of thread-local encoding and decoding contexts. These allow implementers of serialize::Encodable and Decodable to access information and datastructures that would otherwise not be available to them. For example, we can automatically translate def-id and span information during decoding because the decoding context knows which crate the data is decoded from. Or it allows to make ty::Ty decodable because the context has access to the ty::ctxt that is needed for creating ty::Ty instances.
michaelwoerister
force-pushed
the
tls-encoding
branch
from
e254867 to
f65823e
Compare
|
@bors r=nikomatsakis |
|
📌 Commit f65823e has been approved by |
With this commit, metadata encoding and decoding can make use of thread-local encoding and decoding contexts. These allow implementers of `serialize::Encodable` and `Decodable` to access information and datastructures that would otherwise not be available to them. For example, we can automatically translate def-id and span information during decoding because the decoding context knows which crate the data is decoded from. Or it allows to make `ty::Ty` decodable because the context has access to the `ty::ctxt` that is needed for creating `ty::Ty` instances. Some notes: - `tls::with_encoding_context()` and `tls::with_decoding_context()` (as opposed to their unsafe versions) try to prevent the TLS data getting out-of-sync by making sure that the encoder/decoder passed in is actually the same as the one stored in the context. This should prevent accidentally reading from the wrong decoder. - There are no real tests in this PR. I had a unit tests for some of the core aspects of the TLS implementation but it was kind of brittle, a lot of code for mocking `ty::ctxt`, `crate_metadata`, etc and did actually test not so much. The code will soon be tested by the first incremental compilation auto-tests that rely on MIR being properly serialized. However, if people think that some tests should be added before this can land, I'll try to provide some that make sense. r? @nikomatsakis
With this commit, metadata encoding and decoding can make use of thread-local encoding and decoding contexts. These allow implementers of
serialize::EncodableandDecodableto access information anddatastructures that would otherwise not be available to them. For example, we can automatically translate def-id and span information during decoding because the decoding context knows which crate the data is decoded from. Or it allows to make
ty::Tydecodable because the context has access to thety::ctxtthat is needed for creatingty::Tyinstances.Some notes:
tls::with_encoding_context()andtls::with_decoding_context()(as opposed to their unsafe versions) try to prevent the TLS data getting out-of-sync by making sure that the encoder/decoder passed in is actually the same as the one stored in the context. This should prevent accidentally reading from the wrong decoder.ty::ctxt,crate_metadata, etc and did actually test not so much. The code will soon be tested by the first incremental compilation auto-tests that rely on MIR being properly serialized. However, if people think that some tests should be added before this can land, I'll try to provide some that make sense.r? @nikomatsakis