feat: add feature to use rustc-hash

[jalil-salame]

We use rustc-hash::FxHashMap/Set on src/visibility_tracker.rs this is an implementation detail as AFAIK all types there are pub(crate) so we never return an FxHashMap or FxHashSet on a public interface.

This bring a nice perf increase:

❯ cargo criterion --bench indexed_crate
IndexedCrate/new(aws-sdk-ec2)
                        time:   [1.3474 s 1.3506 s 1.3537 s]
                        change: [+4.4411% +4.8278% +5.2381%] (p = 0.00 < 0.05)
                        Performance has regressed.

❯ cargo criterion --bench indexed_crate --features rustc-hash
IndexedCrate/new(aws-sdk-ec2)
                        time:   [1.1555 s 1.1561 s 1.1567 s]
                        change: [-14.603% -14.405% -14.192%] (p = 0.00 < 0.05)
                        Performance has improved.

❯ cargo criterion --bench indexed_crate --features rayon
IndexedCrate/new(aws-sdk-ec2)
                        time:   [577.26 ms 578.45 ms 579.70 ms]
                        change: [-50.083% -49.964% -49.852%] (p = 0.00 < 0.05)
                        Performance has improved.

❯ cargo criterion --bench indexed_crate --features rayon,rustc-hash
IndexedCrate/new(aws-sdk-ec2)
                        time:   [486.94 ms 488.49 ms 490.13 ms]
                        change: [-15.862% -15.552% -15.212%] (p = 0.00 < 0.05)
                        Performance has improved.

I set it as an off by default feature, but seeing as it comes with not public interface changes AFAIK we might want it to be an on by default feature, or just be a direct dependency. rustc-hash is a well maintained small crate so I feel good about turning it on by default.

[obi1kenobi]

Interesting idea! I'll have to read up a bit on rustc-hash, since I want to be more familiar with the guarantees it offers before committing to this change.

Thanks for opening the PR. Leave it with me, I've got the ball and I'll carry it forward from here.

[jalil-salame]

The ahash comparison contains a good overview of fxhash, the main takeaways are:

• Fast
• Not high quality
• Not DOS resistant

ahash claims better perf on strings and seeing that Ids are basically strings, I might have to create a different PR with ahash.

I have issues with ahash (and any crate that performs rustc version detection) as older version tend to break in newer nightly toolchains, and I dislike the practice of auto enabling features.

Another reason I dislike ahash is their claims of good quality hashes are not theoretically proven, just experimentally, so I wouldn't rely on it for DOS resistance.

But I don't thin either of the previous issues apply to this case.

[obi1kenobi]

I agree on version detection being an issue. The perf boost is nice but this being a FOSS project, it's really sensitive to maintainability problems and I don't think we should knowingly add something that creates substantial new maintenance burden. So I wouldn't expose an ahash feature based on that.

You mentioned looking into switching to an IdRef<'a> to avoid the pointer-to-pointer situation we currently have with &'a Id. Would you be open to looking into that before we switch hashers? I suspect that will also produce a perf improvement we want, while also affecting the numbers measured here. Based on what improvements that work shows, I might ask the rustdoc-types maintainers to include an IdRef<'a> type as a built-in — that conversation would be simpler if our perf numbers are easier to understand by not depending on an alternative hasher.

[jalil-salame]

Of course! I'll check it out!

[jalil-salame]

It does give an improvement, but I had to use unsafe to transmute &str into &IdRef:

(numbers from a different machine than the usual one)

IndexedCrate/new(aws-sdk-ec2)
                        time:   [1.4522 s 1.4566 s 1.4617 s]
                        change: [-7.6545% -7.2008% -6.7255%] (p = 0.00 < 0.05)
                        Performance has improved.

I had to use unsafe for the std::borrow::Borrow trait because like Deref it returns &Borrowed so IdRef has to be struct IdRef(str) instead of struct IdRef<'a>(&'a str). I don't know how to work around this issue:

#[repr(transparent)]
struct IdRef(str);

impl std::borrow::Borrow<IdRef> for Id {
    fn borrow(&self) -> &IdRef {
        // Safety: &IdRef has the same layout and lifetime as &str (because it is repr(transparent))
        unsafe { std::mem::transmute(self.0.as_str()) }
    }
}

From a long rabbit hole, I think redefining Id might solve this: (I was wrong, this still requires unsafe to transmute &str to &IdRef :c)

struct IdType<T: ?Sized>(pub T);

type Id = IdType<Box<str>>;
type IdRef = IdType<str>;

fn coerce() {
    let id: Id = IdType("type".into());
    let id_ref: &IdRef = &id;
}

This has the added benefit of Id being two words instead of three (16 instead of 24 bytes in 64-bit systems). I don't see why String should be the inner type of Id instead of Box<str>. There is no need to modify the Id once created.

[obi1kenobi]

I'll ask the rustdoc-types maintainers about the Id type's definition, thank you!

If you have a second, another set of numbers that would be super useful is whether/how the runtime of the entire cargo-semver-checks run over that AWS crate changes as a result of using IdRef.

[obi1kenobi]

I've done some reading, and I think this is a good change — let's do it!