Zero-Copy ReadSlice Trait

[krakow10]

The point of these changes is to reduce the number of copies where possible. To achieve this, we split off chunks of a byte slice instead of allocating a String or Vec where applicable.

Performance

5-8% faster on the deserialize microbenchmarks according to my testing. This mostly affects performance on a per-prop-chunk basis rather than per-property which explains why it doesn't seem to have a big impact on the Miner's Haven benchmark. This is probably also why #545 didn't see a big performance uplift.

[Dekkonot]

I'm a clever boy and can work out that the new functions aren't a part of RbxReadExt because they operate explicitly on a slice instead of a nebulous impl Read, but it might be worth writing it down for future generations. I'll leave that to your discretion, but I tend to err on the side of writing things down even if they feel obvious because it lowers the bus factor for the project.

Otherwise this seems fine to me.

[krakow10]

the new functions aren't a part of RbxReadExt ... it might be worth writing it down

Done. When I was writing this PR at first I was considering a trait too, but decided this was simpler. If you think you can improve the clarity of the documentation wording, suggestions or changes are welcome.

[krakow10]

The read_string and read_binary_string functions are now removed from the RbxReadExt trait entirely, similar to #561. There is now only one implementation of the read_string and read_binary_string functions. I previously included both implementations due to some usages requiring an owned string, but one can simply call .to_owned() to create an owned copy. The functions will also check the length of the input slice and return an UnexpectedEof error in case they are too short as mentioned in #561. There will be conflicts with #561 since I cannot create stacked PRs, but I will handle it if either is merged.

[krakow10]

This now combines changes from #561 to create a RbxReadZeroCopy trait. This way the functions are all grouped together and don't make a mess of the imports, and no longer drastically change the call sites. In all honesty I couldn't think of how to make a trait extend &[u8] when I first opened this PR.

[krakow10]

The compiler keeps whispering ways to make the code better. Now I can put all the functions back into the RbxReadExt trait and use a trait bound to access the zero-copy read_slice method. It's shrunk the core.rs diff considerably.

[krakow10]

The complete vision of changes adding to this PR:

• Add read_array method to ReadSlice trait #575
• RbxWriteInterleaved Trait #573
• RbxReadInterleaved Trait #592

I would like to open stacked PRs, but they need to be created with a branch within the repository itself, which I don't have access to.

[Dekkonot]

I would like to open stacked PRs, but they need to be created with a branch within the repository itself, which I don't have access to.

If you are too keen, you can make PRs on your own fork of rbx-dom and link them. I'd probably just merge them with a squash though so you can theoretically rebase after they're merged.

[krakow10]

Hmm that sounds like two pull requests for the price of two, and without the merge target update of a stacked PR. Speaking of which, a stacked PR will only update the merge target, not automatically rebase, so maybe it's not as useful as I imagined when PRs are squashed to master. Seems like it would still need maintenance from me regardless. I'll just continue to be patient and resolve conflicts as they arise.

[krakow10]

I've split out the RbxReadInterleaved change into its own PR #592. This is just the ReadSlice trait now.

[kennethloeffler]

Nice, this looks to be a significant optimization for light workloads: I saw about +10% on the small benchmarks. It makes sense, since we have proportionally more strings to copy in larger files. Code looks great to me, nice work!