Assorted changes and prepare 0.11.0 release - #305
Conversation
Fixes #224 Signed-off-by: Simon Sapin <simon@igalia.com>
This removes a special case and a trait method Signed-off-by: Simon Sapin <simon@igalia.com>
Signed-off-by: Simon Sapin <simon@igalia.com>
Also align `string-cache-codegen` crate version with corresponding `string-cache` version Signed-off-by: Simon Sapin <simon@igalia.com>
|
This is ready for review, but marked as draft because I’d like to test it in Servo before merging. |
Signed-off-by: Simon Sapin <simon@igalia.com>
A `const` item was considered "unused" if only used in a `const _: () = …;` const assert Signed-off-by: Simon Sapin <simon@igalia.com>
Signed-off-by: Simon Sapin <simon@igalia.com>
|
|
||
| /// Get the hash of the string as it is stored in the set. | ||
| pub fn get_hash(&self) -> u32 { | ||
| pub fn get_hash(&self) -> u64 { |
There was a problem hiding this comment.
@emilio string-cache was the primary motivation to make the precomputed-hash crate, right? should precomputed-hash be changed to return u64 instead of u32?
There was a problem hiding this comment.
I mean, Gecko atoms have a u32. But we could effectively return a u64, see also https://bugzilla.mozilla.org/show_bug.cgi?id=2062315 for example on stylo.
There was a problem hiding this comment.
The Readme examples are still using extern crate, do these need to be updated for using a newer edition?
There was a problem hiding this comment.
It doesn’t need to because the edition used inside the library is independent from the one used by a dependent crate (and extern crate still works in all edition, only it’s usually useless and warns) but yes let’s fix it. Thanks for catching this!
Signed-off-by: Simon Sapin <simon@igalia.com>
With #303 already merged, it’s already the case that the next release has to be semver-incompatible so there’s no need to block merging the version bump. Unmarked as draft |
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
I'm curious if the inline representation is useful tbh.
| /// For inline atoms however (short strings 7 bytes or less), | ||
| /// the returned value is the literal inline representation | ||
| /// with string bytes packed directly in the `u64` value, | ||
| /// which makes it a relatively poor-quality hash if used directly. |
There was a problem hiding this comment.
FWIW this seems unfortunate for servo, because a lot of CSS classes / ids / etc are short.
There was a problem hiding this comment.
I wonder if the previous fold-two-u64-halves-into-a-u32 did provide good quality either, i.e. whether this is actually a change instead of just documenting what was always the case.
Maybe this would benefit from specializing the short-string case of the hash_bytes compression function from rustc-hash (or just using that crate and relying on the compiler throwing away the unused parts)?
There was a problem hiding this comment.
Yeah, yeah, I don't think it's a change in practice.
There was a problem hiding this comment.
Yes these new docs only describe what has been happening for a long time. If you rely on impl Hash for Atom, this u64 is hashed again so it containing the raw bytes for short strings is not a problem.
It’s only Stylo’s PrecomputedHashMap that uses this u64 after only XOR’ing the two halves into a u32, same in this PR as before. The only change is moving the XOR from fn get_hash() to impl PrecomputedHash for Atom
4 participants
Commits:
size_ofandalign_oftestsatom.as_ref()ambiguous in some cases, and becauseatom.get_hash()changed from u32 to u64. Also bumpstring-cache-codegenmultiple versions so its number matches the correspondingstring-cacherelease.