perf(all): experiment: add dedicated identifier string type

[camchenry]

• Related to Create dedicated Ident type for identifier strings backlog#193

This is a proof of concept. Mainly to see how bad the damage is if we add a new Ident type. Will help guide the API and also how we approach implementing this.

[camchenry]

• perf(all): experiment: add dedicated identifier string type #16416 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Performance Report

Merging #16416 will degrade performances by 3.17%

_{Comparing 12-02-perf_all_experiment_add_dedicated_identifier_string_type (38dbc7d) with main (514c724)}

Summary

⚡ 3 improvements
❌ 1 regression
✅ 38 untouched
⏩ 3 skipped¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Mode	Benchmark	BASE	HEAD	Change
⚡	Simulation	isolated-declarations[vue-id.ts]	58 ms	55 ms	+5.57%
⚡	Simulation	semantic[binder.ts]	4.1 ms	3.9 ms	+5.1%
⚡	Simulation	semantic[react.development.js]	1.6 ms	1.5 ms	+3.77%
❌	Simulation	parser[binder.ts]	3.2 ms	3.3 ms	-3.17%

Footnotes

1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[camchenry]

@overlookmotel I ended up making a lot of changes I didn't mean to, so please excuse some of the unnecessary changes. I think I've committed a crime on our codebase. However, the main point is that I converted some of the usage of FxHashMap<Atom, _> to use IdentHashMap<_> instead. I did not include the arena-allocated hash maps as that presents quite a challenge in implementation, though I think that's where some of the most valuable work might be.

Regardless, we are starting to at least some positive benefits that we would expect. It's essentially breakeven once you take into account the additional parsing time, but it's 2-3% faster in semantic already:

[overlookmotel]

Creating a hasher costs nothing. Do "jump to definition" on FxHasher::default() and you'll see.

[overlookmotel]

I just realized something. hasher.finish() is rotating the bits (again, do "jump to definition"). That's not what we want to happen as we want the bits with highest entropy in top 32 bits - we do the rotation ourselves in Hash::hash instead.

So I'd suggest undoing the rotation again here.

let hash = {
    let mut hasher = FxHasher::default();
    s.hash(&mut hasher);
    hasher.finish()
};

// `FxHasher::finish` performs a rotation.
// That's not what we want as we want the highest entropy bits in top 32 bits.
// Undo that rotation here by rotating back in the opposite direction.
// This code is the exact reverse of the code in `FxHasher::finish`,
// so compiler should see that together they make a no-op, and remove both rotations.
#[cfg(target_pointer_width = "64")]
const ROTATE: u32 = 26;
#[cfg(target_pointer_width = "32")]
const ROTATE: u32 = 15;

let hash = hash.rotate_right(ROTATE);

If we have a higher-entropy hash, that should mean less collisions. If so, it might move the semantic benchmarks a bit.

[camchenry]

Ah, good to know that doing FxHasher::default() is actually not doing anything. If we end up porting the fxhash code into oxc, hopefully we can make the rotation simpler (by just not doing it), and not need to rely on this hopefully turning into a no-op.

[overlookmotel]

Yes this is a bit of a hack. We will want to pull FxHasher's code into Oxc as you say. But I'd be very confident compiler will remove it, as long as FxHasher::finish gets inlined (which it really should). Compiler is really good at simple optimizations like this.

https://godbolt.org/z/334EEeKvf

Anyway, it's only 2 ops if it doesn't. The thing that might have an effect on perf is if we get less hash collisions.

[overlookmotel]

This is the place where re-hashing will be happening unnecessarily in semantic when copying from 1 arena to another.

I think this should do the trick - copy string, but don't recalculate hash:

impl<'new_alloc> CloneIn<'new_alloc> for Ident<'_> {
    type Cloned = Ident<'new_alloc>;

    #[inline]
    fn clone_in(&self, allocator: &'new_alloc Allocator) -> Self::Cloned {
        let s = allocator.alloc_str(self.as_str());
        let ptr = NonNull::from(s).cast::<u8>();
        Ident { ptr, len_and_hash: self.len_and_hash, _marker: PhantomData }
    }
}

If there are any other methods which create an Ident from another Ident, they should do the same - but I think this is maybe the only one?

[Boshen]

I'll move this forward.

[Boshen]

closed by the stack #18400