Do not re-hash already hashed keys disambiguator and struct identity maps #647

Veykril · 2025-01-05T11:22:54Z

Additionally newtypes these maps.

We got no reason to hash what we already have hashed! (This does need hashbrown as std doesn't stably expose the required API yet)

netlify · 2025-01-05T11:23:15Z

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	`049cb14`
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/67935295b7da8a00088996f5

codspeed-hq · 2025-01-05T11:26:24Z

CodSpeed Performance Report

Merging #647 will not alter performance

_{Comparing Veykril:veykril/push-tqxotywkkxky (049cb14) with master (855a31a)}

Summary

✅ 9 untouched benchmarks

MichaReiser · 2025-01-05T17:22:07Z

Did you do any perf measurements on ra to verify that the extra complexity and memory usage is worth it?

Veykril · 2025-01-05T17:33:36Z

Not yet, until r-a can use the master branch salsa, checking anything against r-a is kind of a pain as I'd need to work against david's fork I believe.

Either way I'd consider the complexity here rather minor and self-contained (I can clean it up a bit and make it more descriptive if desired) while omitting unnecessary work. So even with perf measurement it should be clear that this skips work imo

…maps Additionally newtypes these maps

davidbarsky · 2025-01-23T16:49:26Z

src/tracked_struct.rs

+    // conceptually, this contains an `IdentityHash`, but using the type directly will grow the size
+    // of if this struct by a word due to unusable padding, so we store the fields directly instead.
+    /// Index of the tracked struct ingredient.


two nits:

Phrasing

Considering moving this comment up to Identity's doc comment?

Suggested change

// conceptually, this contains an `IdentityHash`, but using the type directly will grow the size

// of if this struct by a word due to unusable padding, so we store the fields directly instead.

/// Index of the tracked struct ingredient.

/// Index of the tracked struct ingredient.

///

/// Conceptually, this contains an `IdentityHash`, but using `IdentityHash` directly will grow the size

/// of this struct struct by a `std::mem::size_of::<usize>()` due to unusable padding. To avoid this increase

/// in size, `Identity` stores the fields directly instead.

I put it outside of the doc comment as this is just an implementation detail, it is not relevant to the ingredient_index field

davidbarsky

looks good to me, modulo documentation and style nits.

src/tracked_struct.rs

davidbarsky · 2025-01-23T16:55:13Z

src/tracked_struct.rs

+        let eq_module_hash = |k: &Identity| {
+            k.ingredient_index == key.ingredient_index && k.disambiguator == key.disambiguator
+        };
+        let entry = self.map.raw_entry_mut().from_hash(key.hash, eq_module_hash);


consider inlining eq_module_hash?

Oh, that was supposed to be eq_modulo_hash. I prefer keeping it outside as it documents what we check (also the formatting like this is more readable imo)

davidbarsky · 2025-01-23T16:55:42Z

src/tracked_struct.rs

+        let eq_module_hash = |k: &Identity| {
+            k.ingredient_index == key.ingredient_index && k.disambiguator == key.disambiguator
+        };


consider inlining eq_module_hash?

davidbarsky · 2025-01-23T16:58:51Z

src/tracked_struct.rs

+        let eq_module_hash = |k: &IdentityHash| k.ingredient_index == key.ingredient_index;
+        let entry = self.map.raw_entry_mut().from_hash(key.hash, eq_module_hash);


consider inlining eq_module_hash?

src/zalsa_local.rs

MichaReiser · 2025-02-08T21:12:41Z

Backing out of this PR fixes the coarse grained dependency upgrade in Ruff. I've to take a look at this more closely tomorrow

Veykril · 2025-02-08T21:59:16Z

That sounds surprising to me as this PR shouldn't have changed anything.

MichaReiser · 2025-02-08T22:07:42Z

src/tracked_struct.rs

+    pub(crate) fn disambiguate(&mut self, key: IdentityHash) -> Disambiguator {
+        use hashbrown::hash_map::RawEntryMut;
+
+        let eq_modulo_hash = |k: &IdentityHash| k.ingredient_index == key.ingredient_index;


It seems that we also need to include k.hash == key.hash.... I assume that is because the hash map might map multiple hashes to the same bucket and it then uses the equality function to disambiguate. This applies here and at the other from_hash call sites.

What's good about it is that it simplifies the eq implementation a lot :)

To add a little context, hashbrown doesn't store a full hash of each key, and only compares partially. The equality function cannot rely on the hash being equal, it must uniquely identify an entry regardless (e.g. hashbrown could legally do a linear search for equality).

Ah I see, well that would do it. I completely forgot that hashmaps tend to trim the hash to pick a bucket to put the values into... I hope that didn't waste too much time sorry.

Veykril force-pushed the veykril/push-tqxotywkkxky branch from 51cdd70 to 67e8cab Compare January 5, 2025 11:44

Veykril added 2 commits January 23, 2025 13:32

Do not re-hash already hashed keys disambiguator and struct identity …

ce1a5ca

…maps Additionally newtypes these maps

Shrink Identity by a word

5ac3490

Veykril force-pushed the veykril/push-tqxotywkkxky branch from 1637c61 to d717cf8 Compare January 23, 2025 12:32

davidbarsky reviewed Jan 23, 2025

View reviewed changes

davidbarsky approved these changes Jan 23, 2025

View reviewed changes

Clarify manual hashmap usage

049cb14

Veykril force-pushed the veykril/push-tqxotywkkxky branch from d717cf8 to 049cb14 Compare January 24, 2025 08:42

Veykril added this pull request to the merge queue Jan 24, 2025

Merged via the queue into salsa-rs:master with commit cde9fbd Jan 24, 2025
10 checks passed

MichaReiser mentioned this pull request Feb 8, 2025

Transition to salsa coarse-grained tracked structs astral-sh/ruff#15763

Merged

Veykril deleted the veykril/push-tqxotywkkxky branch February 8, 2025 21:35

MichaReiser reviewed Feb 8, 2025

View reviewed changes

Veykril mentioned this pull request Feb 9, 2025

Fix Disambiguator- and IdentityMap hashing #682

Merged

		let eq_module_hash = \|k: &IdentityHash\| k.ingredient_index == key.ingredient_index;
		let entry = self.map.raw_entry_mut().from_hash(key.hash, eq_module_hash);

Do not re-hash already hashed keys disambiguator and struct identity maps #647

Do not re-hash already hashed keys disambiguator and struct identity maps #647

Uh oh!

Conversation

Veykril commented Jan 5, 2025

Uh oh!

netlify bot commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for salsa-rs canceled.

Uh oh!

codspeed-hq bot commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #647 will not alter performance

Summary

Uh oh!

MichaReiser commented Jan 5, 2025

Uh oh!

Veykril commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidbarsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Feb 8, 2025

Uh oh!

Veykril commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veykril Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Jan 5, 2025 •

edited

Loading

codspeed-hq bot commented Jan 5, 2025 •

edited

Loading

Veykril commented Jan 5, 2025 •

edited

Loading

Veykril commented Feb 8, 2025 •

edited

Loading

MichaReiser Feb 8, 2025 •

edited

Loading

Veykril Feb 9, 2025 •

edited

Loading