-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZeroCow: a one-element, owned-or-borrowed VarULE #5561
Comments
The owned type for |
Actually, I'm a bit confused as to the use case: where are we even putting |
I totally support this for serde borrowing, which is broken for
I don't think this is possible. The |
I basically don't think this makes sense in the |
icu4x/components/plurals/src/provider.rs Line 371 in 3694bfe
|
Yeah, I see. I think optimizing this is an ICU4X concern, not a zerovec concern. I really don't think this fits into the holistic design that well, it's just a slight improvement over |
It's a zerovec concern to create It's a zerovec concern to create I think it is also a zerovec concern to create Another advantage I'll add:
|
About stack size: what I was thinking (the reason I said "might") is that there are some various ways of various levels of desirability that we could get an extra niche. The most likely is that we limit the length of the
Where the u32 has 1 bit for the discriminant (owned/borrowed) and 31 bits for the length. The total size of this type is 8 bytes on wasm32 and 16 bytes on aarch64. I'm comfortable with this length limit because it is the same as ICU4C's length limit: it uses With this representation, a read would These are optimizations we can do with |
... barely. I actually think in retrospect trying to design this crate to contain its own We kind of ended up with this after multiple iterations on the design where ZV and VZV were more "parallel" to each other in behavior. I think it made a bit more sense then, but we've moved past that when we learned that design didn't work well. The cow-iness of the primary types here comes from an assumption about supporting both human readable and binary deserialization, which is true for ICU4X but isn't actually that necessary for a lot of people choosing to use zero-copy deserialization. Cow is commonly useful in zero-copy deserialization, but it's an orthogonal concern. If I were to design these types today, I'd potentially have
At least with ZeroVec/VarZeroVec the types were ones we created and we needed to make owned/borrowed versions. That's not the case with VarULE types, they're often third party and already work with Cow.
I can't see this working, EncodeAsVarULE is too generic to give you a concrete owned type. We would need something like ToOwned, still, for a ZeroCow to work.
Only if we wish to lose convenient human-readable deserialization. I think this issue is proposing adding a type to zerovec to work around a serde concern. I know serde has refused to move forward on this, but I don't think that makes it zerovec's job: I think if anyone is to do this it should be some common ICU4X crate ( |
I used to think this, but my thinking shifted when I thought more about it.
I remain generally opposed to
It's not EncodeAsVarULE, sorry, it's just Your response is primarily focused on the theoretical best home for this type. I still think I agree with you about mutation; I was never super excited about mutation. A design I might have preferred, and could see myself supporting if we wanted to move in this direction, would be You also didn't address the technical benefits that my proposed type can bring above and beyond what |
Okay, but it's for zero-copy deserialization in serde. It is not for fixing every shortcoming that serde has. I think patching the Cow thing is out of scope for this crate. I also think "a stack size optimized Cow" is out of scope for this crate: that is a benefit ICU4X and as far as I can tell basically only ICU4X cares about.
So I'm not convinced of the benefits either, for a general audience. The benefits seem extremely marginal over Cow. The stack size benefit in particular is something ICU4X cares strongly about and is why I would support having this type somewhere (icu_provider seems fine), but Rust programmers by and large care about stack size extremely rarely. The branchlessness is a premature optimization; we made VZV branchless primarily for stack size, not for the perf, which was fine. Basically: I see this as something that makes some sense for ICU4X. I do not see this as something that is useful for most zerovec consumers, and even if it were I'm not convinced zerovec is the home for it, which makes me strongly oppose having this be in the zerovec crate. I also think that we should just use Cow unless Cow is actually proving to be a problem. I don't consider needing to use I've said this before but I'll say it again: I don't like that we keep stuffing the zerovec crate with a grab bag of abstractions that make sense for ICU4X. I want it to be more intentionally designed: we're moving towards it in #5523 which I'm very happy about but I would like for our first reaction to new serialization abstractions to not be to add it to zerovec. I think such things should at least incubate more before we add them.
Those are the bounds we need now. That's not a new benefit as far as I can tell. |
I think I understand your position, and I think we differ one some fundamentals and experiences. More on that later. To break down that middle paragraph:
OK. Glad we agree on the principle there, even if I see it as making more sense than you do.
I very much disagree with this statement. It is absolutely useful for "most zerovec consumers". That audience is using zerovec because of serde, and they want to put VarULEs in serde structs in exactly the same way ICU4X does, and they can't do so right now because of the Serde bugs involving Cow, which can be hard to detect, resulting in clients getting more allocations than they should. We, the ICU4X team, didn't even notice the Serde bug right away when we started using cows in data structs.
I haven't heard a counter-suggestion of a home for this type other than One other thing you said earlier which I didn't respond to:
ZeroCow doesn't need to use human-readable serialization. It can always serialize to bytes if it wants. I'm happy to discuss the predicate used for forking between |
Would you agree that the following would be in scope for the If you agree that such a tutorial is useful, then you agree that this problem space is in scope for |
About We don't need |
I'm trying to break down where we disagree. FundamentalsFundamental: The bar for adding things to
|
Thanks for writing this out, it's helpful. TLDR: let's focus on that last "by experience" point about the complexity of implementing
I'm not sure I fully agree with this distinction, or at least the terms used, but I think my issue here is coming from from a slightly different distinction: "new" and "mature" crates. A "mature" crate is one that may still contain a lot of stuff, but things are added with care, and it has a coherent structure. We've been trying to get ICU4X to this stage over time, and overall we've been succeeding. We are able to do this without being minimal, we're just careful. I'd like I think this sense of maturity is rather important: I've seen this dynamic of "not worth it" play out a couple times for unsafe review at Google where we've told importers of some relatively unknown ball of unsafe abstractions that we cannot review their crate, which gives them the options to either visibility restrict (which means the code will not be allowed in security sensitive environments like prod), or find alternate dependencies. I'd rather not have ICU4X be in this position: most larger codebases are starting to care a lot more about unsafe code now and this is going to be in a bigger and bigger problem. While in general consolidating one-off bits of code into generic code is considered good practice, the tradeoff is trickier with unsafe code since generic unsafe code is a much higher bar. To be clear, One of the reasons I care a lot about doing #5523 is that it hopes to clean up some of the haphazardness that we have built up over time and provides a more holistically thought out interface. I'm hoping to convince someone to do a proper safety audit of zerovec soon, and this helps make that an easier sell. So the main aspect of maturity here is just how well thought out the overall œuvre of abstractions are in this crate. This is why I tend to place a high value towards incubating these abstractions more, either as private zerovec abstractions or as ICU4X shared abstractions. We've already come a long way and realized what the holistic model looks like, and it's different than what things looked like when we added them, like in #5523. Another aspect of maturity of a crate is that the crate has a clear target audience, and this is also why I tend to be wary of strongly coupling zerovec to ICU4X's needs. I've expressed similar opinions when it comes to Diplomat before. In particular, Google-run open source projects have an especially bad reputation for having this problem, and it leads to people . ICU4X is not Google-run, but zerovec is effectively maintained only by Googlers, and I can see this reputation easily harming us when it comes to more adoption. I feel like Diplomat has benefited quite a bit from this, there have been a bunch of places where we could have been far more streamlined for ICU4X4 but we went generic and have benefitted from more users, free backends, and more compelling intern projects. To me, zerovec's target audience is congruent to that of This brings us to the next thing:
I think ICU4X is close to a typical zerovec client in many ways. I think the need for (And as you note: the need for optimizing stack size is also definitely atypical)
FWIW I think optimizing for binary size is absolutely not special and a standard zerovec user need, as is heap memory use. Less so on stack memory use. I do sometimes feel that some optimizations aren't "worth it" when it comes to the complexity that they bring us, either for ICU4X or for zerovec. This is what led to me pushing for #5523, where we can get a bunch of optimizations whilst reducing complexity instead of growing it. Hopefully. I'm still working on it and as I previously noted some of the wins I was hoping to get aren't as easily possible, but it's still a net win in my prediction.
Agreed, however as I note above I think borrowing Cow-like serde-impls are only somewhat relevant. Which is still something to care about, but just registering that I care somewhat less than borrowing in general.
I am not convinced of this. I could be convinced, but I'm not at the moment. I recall the branching showing up in microbenchmarks, but I don't recall it being a huge deal (I could be wrong! I don't know the numbers), and I'm also not convinced that the microbenchmarks are showing the numbers actually relevant to the clients. They've been designed to give a general idea of how the crate works perf-wise ("okay, zerovec is slower than vec, but not too much slower, and varzerovec is noticeably slower than vec, but not onerously so"), but I'm not sure they're actually what matters for clients. I'm not even convinced this matters for ICU4X's usage patterns that much; almost all of our zerovec-using data structs are used in formatter types that have far far more branching all over the place. I'm open to doing the optimization just for ICU4X but I don't find it super relevant. Looking at the microbenchmarks on With ZeroCow the optimization is even more clear to not be optimizing "a ton of reads in a row": with zerovec I can kind of imagine a usage pattern that hits the same vec multiple times in a way that retriggers the branch7, but with ZeroCow it's just one thing; I imagine it would mostly get read once. (I'll also note that ZeroCow replaces a branch with a bitmask which is not as fast as getting rid of the branch entirely. Still should move the needle) So overall with ZeroCow's branchlessness I see an optimization I'm not convinced is useful for ICU4X, and I'm even less convinced is useful for the general zerovec audience. It's an optimization I'm happy to perform prematurely in ICU4X to play around with it, but I feel like it reduces zerovec's overall maturity.
I agree, I just mostly feel that "fixing Cow in serde" feels like scope creep for zerovec.
I don't think this makes sense: it's a problem with serde-derive in the first place. Manual serde impls don't have this problem. It's a bit of a non sequitur to ask for there to be a core serde solution to a problem that is not there in core serde.
This is compelling, though this still matches my read of this being a "serde + Cow" problem regardless of zerovec, that we should patch in a separate util or an ICU4X function stuffed in some ICU4X crate somewhere. I don't think the Fn-vs-trait thing is the primary issue here. I actually think that the Rust compiler corners are just inherent to Deserialize's design in general, I'm not sure this is particularly prone to it. But we can poke at that more.
Okay, this is interesting. I don't think I've experienced this and would like to hear more. Requiring a deep understanding of all three of those is somewhat compelling to me for this to go into zerovec. I also think that understanding this problem may lead to us discovering alternate solutions, for example
I previously couldn't see a ZeroCow design that avoids this. But potentially Footnotes
|
After working a little bit on serde impls for our generic types, and for our custom derives, I think I have a better grasp on the issue with serde impls on VarULE types. Firstly, you need three impls:
Secondly, making sure you correctly handle human and machine readable is tricky: writing If you do all of this correctly, and add a ToOwned impl, then `Cow<'a, Type> will work. As far as I can tell, |
Don't forget about exporting a deserialize_with fn and using it at all the call sites. Then it will work. |
Thought: ZeroCow could have a type parameter that configures the human readable serialization, and it defaults to something that serializes it as a sequence of bytes. Alternatively, it could reference a (potentially private) human readable struct that implements serde and has TryFrom impls. Just trying to come up with a way to avoid needing any custom serde code. Needs some design work. |
Fixes #4437 This doesn't work yet: there's no easy way to get Bake impls for custom VarULE types since `from_byte_slice_unchecked` can't be called in const contexts (since you can't do trait stuff in non-const contexts: in theory this function is very const-capable) I chose to use `make_varule` but VarTupleNULE would work as well (and plus we can give it the relevant Bake impl if we want). I wrote this PR to encapsulate things so that swapping the internals is easier I think I'll make this work by implementing ZeroCow: #5561 <!-- Thank you for your pull request to ICU4X! Reminder: try to use [Conventional Comments](https://conventionalcomments.org/) to make comments clearer. Please see https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for general information on contributing to ICU4X. -->
Related: #5523 and #5531 #1556
It fits into the holistic zerovec design to have a new type
ZeroCow
.It holds onto an owned-or-borrowed VarULE. There is no need to have a
ZeroCowULE
because the ULE will be the VarULE.Crucial differences from
alloc::borrow::Cow
:serde
Box
instead ofVec
for the owned type@Manishearth
The text was updated successfully, but these errors were encountered: