syntax: Fix encoding and decoding spans #31028
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
|
Large numbers are always somewhat sketchy to work with in JSON since they'll so often round trip through a double. Should we maybe encode with the packed number in a string? |
|
@sfackler: True, although |
|
I've seen it hit in systems that use randomly generated 64 bit identifiers, for example. In this case, I think it'd require a high span index greater than 2^21, which is roughly 2 million. It seems not wildly unreasonable to have 2 megabytes of source code for a crate, but I'm not sure. |
|
How bad would it be for JSON to encode spans as structs? Because for the binary encoding that is used for crate metadata, there's no more overhead for these. The main reason for encoding it as two separate values was to allow for the variable-length integer format to kick in properly. |
|
@michaelwoerister: Yeah, serializing them into structs would work. We can save this for another time, but we could also optimize the rbml tuple encoding (and tuple structs) to match that of structs by leaving out the tag and the length encoding. We'd lose some safety, but it might shave some more bytes off our metadata. |
|
@sfackler / @michaelwoerister: I've updated the patch to serialize Spans as a struct. Tests passed locally, and the size of the libraries seems to be identical to before this patch. |
| self.lo.encode(s) | ||
| })); | ||
|
|
||
| s.emit_struct_field("hi", 0, |s| { |
There was a problem hiding this comment.
The field index should be 1 here.
The protocol for `serialize::{En,De}code` doesn't allow for two
integers to be serialized next to each other. This switches the
protocol to serializing `Span`s as a struct. rbml structs don't
have any overhead, so the metadata shouldn't increase in size,
but it allows the json format to be properly generated, albeit
slightly more heavy than when it was just serializing a span as
a u64.
Closes rust-lang#31025.
s
|
r=me if you fix those two errors. |
|
Thanks @michaelwoerister! |
|
@bors: r+ |
|
📌 Commit 1dc7eb8 has been approved by |
The protocol for `serialize::{En,De}code` doesn't allow for two integers to be serialized next to each other.
Closes #31025.
cc @michaelwoerister
The protocol for
serialize::{En,De}codedoesn't allow for two integers to be serialized next to each other.Closes #31025.
cc @michaelwoerister