Use Zero-Copy in Reflection Database

[krakow10]

This replaces all occurences of Cow<'a, str> and String in rbx_reflection with &'a str and a #[serde(borrow)] annotation. The rbx_reflector implementation is rather lazy. No performance impact after the database is initialized, but will have somewhat reduced memory usage for the in-memory database as well as reduced initialization time. There could be little fix-ups I missed because of how coerce-able &str is.

Note:

• The migration field of PropertyMigration is made pub

[Dekkonot]

I measured the impact by swapping out the global allocator to measure the memory used by the database, and it looks like this would save us 685kb on startup, which amounts to 24% of the memory footprint of the reflection database. At least on startup; no promises that none of the other code involved isn't causing any noise.

Unfortunately though I'm not sure if I can merge this right now. It's a good change, but it's a breaking one for rbx_reflection and I'm not sure it's worth the performance gain. I'm not going to close this, but a breaking change for a 685kb change in memory feels not worth it. I doubt anyone is running rbx-dom in an environment where they need that memory back urgently.

In terms of implementation, this is mostly fine but I notice a trend with using &Enum::Variant when pattern matching. Presumably this is to avoid ownership trouble, but probably the right way to do this is either borrowing the value you're matching or using the ref keyword.

e.g. instead of

match value {
  &Enum::Variant(value) => {}
}

you should probably be doing

match &value {
  Enum::Variant(value) => {}
}

or Enum::Variant(ref value) => {} if necessary.

[krakow10]

Unfortunately though I'm not sure if I can merge this right now. It's a good change, but it's a breaking one for rbx_reflection and I'm not sure it's worth the performance gain. I'm not going to close this, but a breaking change for a 685kb change in memory feels not worth it. I doubt anyone is running rbx-dom in an environment where they need that memory back urgently.

That's fine, you're right that I'm not hurting for 1MB of memory here. I put this together to implement the desired "lifetime borrowing" semantics described in #515 (comment) after I realized I had seen an implementation in another project. If it can be eventually combined with other breaking changes to make a worthwhile upgrade then that's great.

I notice a trend with using &Enum::Variant when pattern matching. Presumably this is to avoid ownership trouble, but probably the right way to do this is either borrowing the value you're matching or using the ref keyword.

I had always just assumed it would generate the same assembly whether you dereference or bind by reference. I was using the &Enum::Variant(value) => {} syntax simply because I prefer the look of deconstruction using & to the dereference operator *. I haven't thought deeply about it, I just like the type to be in the correct form to be used directly after without dereference or coercion. For example here:

rbx-dom/rbx_binary/src/core.rs

Line 386 in 73cc7c3

&PropertyKind::Alias { alias_for } => {

Without prefixing the match arm with &, the type of alias_for is &&str. If there's a performance reason not to do this that I don't understand then I would love to learn about it. I checked it out in Compiler Explorer and it seems like there is actually a difference: https://godbolt.org/z/5E8KxY1xh I don't really understand what the assembly difference means other than there is one extra instruction using &.

I suspect there could be a misunderstanding, so to be clear, the & in this code is dereferencing, not borrowing:

match value {
  &Enum::Variant(variant) => variant
}

Equivalent code would be:

match value {
  Enum::Variant(variant) => *variant
}

[Dekkonot]

Without prefixing the match arm with &, the type of alias_for is &&str. If there's a performance reason not to do this that I don't understand then I would love to learn about it. I checked it out in Compiler Explorer and it seems like there is actually a difference: https://godbolt.org/z/5E8KxY1xh I don't really understand what the assembly difference means other than there is one extra instruction using &.

I'll be honest: I don't know either. My preference is entirely just because I consider &Enum::Variant(value) => value to be worse than Enum::Variant(value) => *value visually. The fact that you're using & for dereferencing is horrifying to me, haha.

[krakow10]

I updated the dereferencing deconstructions I found according to your preference. By the way, what do you think about Into<&'a str>? I actually specifically needed to do this instead of AsRef<str> for hash_str because AsRef has no lifetime information from the source value. It looks like it's also applicable here (as in AsRef<str> will not work), although simply accepting &'a str would definitely make more sense. Notably &String does not implement Into<&'a str> so it's instantly worse.

[krakow10]

@Dekkonot While cutting a new release on my fork, I noticed that #376 is a breaking change and will require a new database release. Perhaps this change can be included in the next major database version?

[Dekkonot]

I'm not opposed to that, though the breaking change in rbx-reflection-database doesn't necessarily correspond to one in rbx-reflection.