feat!: reduce size of Id and add IdRef #31
Conversation
|
Apparently |
This reduces the size of `Id` from 24 bytes to 16 bytes on 64-bit systems, and from 12 bytes to 8 bytes in 32-bit systems. It also adds an `IdRef` type that we can use instead of a `&Id`, this avoids a double indirection (`&str` vs `&String`). In `trustfall-rustdoc-adapter` we see a [7% perf improvement][1] when using `&IdRef` instead of `&Id`. Sadly, AFAIK, there is no way to safely coerce `Id(Box<str>)` to `&IdRef(str)` so we need to do an unsafe `std::mem::transmute(&str) -> &IdRef`. [1]: obi1kenobi/trustfall-rustdoc-adapter#423 (comment)
jalil-salame
force-pushed
the
id-ref-type
branch
from
f39cf86 to
5bca39e
Compare
There was a problem hiding this comment.
A couple of comments:
-
This repo shouldn't be used for development of the crate source code. See here for why rust-lang/rust is the right place to submit this. (aside: if you did read that section and it wasn't clear to you, please let me know)
-
I'm unclear what the benchmark numbers are showing: The linked PR changes the hashing algorithm, not anything to do with how ID's are represented. I'd like it to be clearer what's being measured.
-
I'm fully in support of changing the definition of
Id. One thing to note is that this makes the inner string not-accessible to users, but I'd like to move towards that. (but this will have complications aslibrustdocneeds to be able to construct anId. Maybe we can add more filtering to theupdate.sh, so the upstream version exposes the inner string but we don't here) -
I'm hesitant about adding
IdRef. The additional complexity is unfortunate. What are the performance numbers like if you don't include it, but do changeIdto be narrower? -
It seems the root of this problem is that our
Id's allocate. This would take much more work, but have you thaught about changing rustdoc to use integer id's instead of strings? This would probably have even better perf implications, and avoid more api complexity. However, it would be much more involved to implement. I'd be happy to landId(but more hesitant forIdRef) changes as an interim measure if you think that'd be worthwhile.
| #[doc(hidden)] | ||
| #[repr(transparent)] | ||
| #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| pub struct IdType<T: ?Sized>(T); |
There was a problem hiding this comment.
What's the purpose of this? I think It'd be better to just duplicate all the derives, rather than using a type alias here.
| pub struct Id(pub String); | ||
| pub type Id = IdType<Box<str>>; | ||
|
|
||
| /// A reference to an [`Id`] |
There was a problem hiding this comment.
But an IdRef isn't a reference to an Id, but an usized typed than can be taken as a reference to. &IdRef is a reference to an Id.
Also, this doc comment should explain the motivations for doing this.
|
Closing as this should be sent to rust-lang/rust |
|
Sorry, I didn't read the contributing guidelines. The comment addresses experimental changes where we switch from The generics are from an experiment on unsized coercion, I thought I might be able to coerce |
This reduces the size of
Idfrom 24 bytes to 16 bytes on 64-bit systems, and from 12 bytes to 8 bytes in 32-bit systems.It also adds an
IdReftype that we can use instead of a&Id, this avoids a double indirection (&strvs&String). Intrustfall-rustdoc-adapterwe see a 7% perf improvement when using&IdRefinstead of&Id.Sadly, AFAIK, there is no way to safely coerce
Id(Box<str>)to&IdRef(str)so we need to do an unsafestd::mem::transmute(&str) -> &IdRef.zulip discussion