dist: Support a bincoded manifest file for performance reasons #2627
Conversation
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
kinnison
changed the title
|
I've rewritten this as a serialisation of the parsed manifest as a bincoded file. This is basically the same performance as toml parsing the trimmed manifest, but doesn't involve trimming which was debateable as to its correctness. We need to introduce a version indicator for this so that we can detect if we should fall back to reading the toml and rewriting the bincode in case of changing our manifest structures. |
|
Lets use flatbuf not bincode. https://www.reddit.com/r/rust/comments/cmq6k9/bincode_for_other_languages/ vs https://google.github.io/flatbuffers/flatbuffers_support.html |
|
Is there a flatbuffers crate with serde support? |
|
|
||
| file.sync_data()?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn write_file(path: &Path, contents: &str) -> io::Result<()> { |
There was a problem hiding this comment.
I don't think this pays for itself vs write_file(path, contents.as_bytes())?;
There was a problem hiding this comment.
Fair enough, I'll sort out a refactor commit alongside this which pushes that up to the call sites.
| @@ -52,20 +52,24 @@ pub fn if_not_empty<S: PartialEq<str>>(s: S) -> Option<S> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn write_file(path: &Path, contents: &str) -> io::Result<()> { | |||
| pub fn write_file_bytes(path: &Path, contents: &[u8]) -> io::Result<()> { | |||
There was a problem hiding this comment.
I note this is doing a sync_data - this is an important part of the contract of the function; if we're renaming it perhaps consider exposing that at the same time - e.g. namespacing it or adding _synced or something.
There was a problem hiding this comment.
I agree with this idea and will sort it out
|
There is for flexbuffers - https://github.com/google/flatbuffers/tree/master/rust/flexbuffers - but I'm not sure of the story for flatfbuffers. |
|
Okay so flexbuffers look plausible vs. bincode, though as it's an internal cache implementation detail why are you adamant we shouldn't use bincode? |
|
If we need to debug it or introspect it, flatbuffers has more tooling available as it isn't rust-only with relatively few users. ditto flexbuffers; flatbuffers is the schemad version, I don't think the lack of serde support should be an issue though I haven't looked into it closely - an alternative would be protobuf, the tower protobuf glue is pretty nice |
|
I'm concerned about minimising the impact of the effort if we're do this soon. I was thinking of treating the binary as a cache and if it failed to load falling back to the toml. The serde capability just means it's much less effort for us in terms of implementation. Debuggability is a good argument against bincode though. Flexbuffers look plausible if a bit more awkward to implement than bincode, yaml, json, etc. |
|
Cargo also uses bincode for certain caches like fingerprints. Flatbuffers having a schema would make the caches a bit bigger I think and will likely encourage others to inspect this implementation detail. |
This goes some of the way to mitigating #2626 but isn't a "fix" per-se.
Not least, we need to be sure of whether this is valid.