Refactor VZV code to center on (and Deref to) VarZeroSlice; make VZVBorrowed private #1418
Conversation
|
I think one of my PRs caused you merge conflicts |
|
Argh, you named get_encoded_slice to as_encoded_bytes, I named it to as_bytes, this is going to be a mess |
|
Do you think we should stick ot as_bytes here or move it over to as_encoded_bytes? |
|
I'm going to go ahead with the rename in this PR because it does more and does it consistently, can undo in a subsequent commit |
| #[inline] | ||
| pub fn as_borrowed<'a>(&'a self) -> VarZeroVecBorrowed<'a, T> { | ||
| pub(crate) fn as_components<'a>(&'a self) -> VarZeroVecComponents<'a, T> { |
There was a problem hiding this comment.
Thought: This is a hot function, and I'm unsure about the code size implications. If everything gets super inlined, the VarZeroVecComponents need not ever actually exist, and overall code size might co down. But if these functions are never inlined, then we can do more code sharing.
There was a problem hiding this comment.
According to CI:
Before:
-rwxr-xr-x 1 runner docker 59664 Dec 20 02:10 optim5.elf
After:
-rwxr-xr-x 1 runner docker 58528 Dec 20 03:16 optim5.elf
So this PR actually reduces code size overall. I'm happy to investigate future improvements later then.
There was a problem hiding this comment.
Yeah, it reduces codesize by sharing all the codepaths better I think
| pub fn len(&self) -> usize { | ||
| self.as_borrowed().len() | ||
| self.as_components().len() |
There was a problem hiding this comment.
Thought: I was thinking that we would write custom code for each of these functions. To get the length, we just read the first 4 bytes. In theory, the compiler should figure that out, iff everything is inlined and DCE works fully. But I don't know if I want to rely on that.
There was a problem hiding this comment.
I kinda like that all the implementation code is in the components module: Perhaps as a followup we can add helper methods there that do partial parsing?
There was a problem hiding this comment.
something like components::len_of_buffer and components::indices_of_buffer(len)
There was a problem hiding this comment.
Yes, that would work for me.
Fine for follow up.
Manishearth
force-pushed
the
varzeroslice
branch
from
9ea1878 to
a3f83b3
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
Merging this on behalf of @Manishearth so it doesn't bitrot again |
Fixes #1367
VZV and VZVOowned now deref to VZSlice, and most functionality is now on VZSlice. This simplifies our code greatly.
VZVBorrowed has been renamed to VZVComponents and is now crate-private; it still needs to exist to hold all of the unsafe parsed representation code.
As mentioned in #1367 (comment) , there was no appreciable difference in the benchmarks.
A thing I didn't do here, but could have, is marking the
varzerovecmodule as private and reexporting VZVOwned up top. Right now we have ZV, VZV, ZS, VZS up top which is nice, but now that there's only one other type invarzerovecit might be worth reexporting it too so that there's no extra module. It doesn't sound like a huge deal either way.