Use `inventory` for static ingredient registration by ibraheemdev · Pull Request #934 · salsa-rs/salsa

ibraheemdev · 2025-07-09T20:16:38Z

Salsa currently registers ingredients dynamically the first time they are accessed, but persistent caching will require that all serializable ingredients are registered up-front. It's possible that we may switch to a static ingredient registration API, but as a first step, we can use inventory to ensure all ingredients have stable indices. This should enable persistent caching and also give us the same performance benefits of static ingredient registration without a large breaking API change.

Inventory has pretty good platform support, I believe it includes all the platforms we care about. There is a caveat on WebAssembly, but I don't think it's a dealbreaker.

netlify · 2025-07-09T20:16:43Z

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	`21838e4`
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/6879c9d4c190190008dd0059

ibraheemdev · 2025-07-09T20:17:22Z


        input.items.push(parse_quote! {
+            #[cold]
+            #[inline(never)]


This was previously marked as inline(always), but it's only called in the slow path for both single and multi-database use cases.

codspeed-hq · 2025-07-09T20:19:25Z

CodSpeed Performance Report

Merging #934 will improve performances by 29.04%

_{Comparing ibraheemdev:ibraheem/inventory (21838e4) with master (d28d66b)}

Summary

⚡ 9 improvements
✅ 3 untouched benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`accumulator`	3.8 ms	3.4 ms	+12.68%
⚡	`amortized[Input]`	3.5 µs	2.7 µs	+29.04%
⚡	`amortized[InternedInput]`	3.4 µs	2.7 µs	+25.6%
⚡	`amortized[SupertypeInput]`	4.1 µs	3.3 µs	+22.59%
⚡	`new[Input]`	10.5 µs	9.3 µs	+13.56%
⚡	`new[InternedInput]`	5.8 µs	4.7 µs	+23.58%
⚡	`new[SupertypeInput]`	17.6 µs	15.4 µs	+14.2%
⚡	`converge_diverge`	141.6 µs	131.7 µs	+7.49%
⚡	`many_tracked_structs`	37.2 µs	32.4 µs	+14.65%

ibraheemdev · 2025-07-09T20:20:11Z

+            IngredientCache::<I>::UNINITIALIZED,
+            packed,
+            Ordering::Release,
+            Ordering::Relaxed,


I relaxed the failure ordering here because we don't synchronize on failure. FWIW this compare_exchange could even just be a store because it encapsulates all the cached data, but it doesn't really matter.

ibraheemdev · 2025-07-09T20:22:53Z

I believe the benchmark results are somewhat misleading here because they use multiple databases. The fast path for accessing an ingredient is affected less (it doesn't include the HashMap lookup, only the Vec). That said this should be a clear win.

ibraheemdev · 2025-07-09T20:38:12Z

Hmm looks like Miri doesn't support this yet rust-lang/miri#450. That might be the dealbreaker I was anticipating.

Darksonn · 2025-07-10T11:01:56Z

There are also many projects out there that ban life-before-main including inventory for various reasons including linker issues. For example, if salsa used inventory, we would never be able to import it into the Android codebase.

ibraheemdev · 2025-07-10T11:03:43Z

                    ) -> &$zalsa::interned::IngredientImpl<$Configuration> {
                        let zalsa = db.zalsa();
                        $INTERN_CACHE.get_or_create(zalsa, || {
-                            <dyn $Db as $Db>::zalsa_register_downcaster(db);


I'm not exactly sure why this was here before, I don't think it's necessary. It's clearer now because we register the downcaster on the function ingredient directly.

ibraheemdev · 2025-07-10T11:08:33Z

There are also many projects out there that ban life-before-main including inventory for various reasons including linker issues. For example, if salsa used inventory, we would never be able to import it into the Android codebase.

I think the ideal approach would be to make this an optional feature, and users who can't use inventory can register ingredients manually instead.

Out of curiosity, does the Android codebase support linkme?

ibraheemdev · 2025-07-15T05:14:02Z

I added a simple API for manual ingredient registration. This is just blocked on rust-lang/miri#4459 for Miri support (to avoid rewriting all of our tests to use manual registration).

MichaReiser · 2025-07-15T06:38:03Z

+        // It doesn't matter if we overwrite any stores, as `create_index` should
+        // always return the same index.
+        self.ingredient_index
+            .store(ingredient_index.as_u32(), Ordering::Release);


I'm not sure this is guaranteed when using manual registration because different databases can have different registered ingredients.

Hmm you're right. I guess we still need the nonce check when the inventory feature is disabled? You shouldn't be able to register any ingredients manually that weren't already registered when inventory is enabled.

Hmm, this does mean that our inventory feature isn't strictly addeditve because you won't be able to use the manual registration functions when using inventory (or you can, they're just ignored)

With the inventory feature you can't create an ingredient except that it's automatically registered.

…is in use

MichaReiser

Nice. This is awesome.

I wonder if this also fixes the non-determinism when replaying multithreaded tests.

Requesting review from David/Lukas to get a thumbs up/down from the r-a side

Do you know how far off the miri changes are? Also, have you tested if the ty playground wroks with this new salsa version?

davidbarsky

given that miri supports this + this is a substantial speedup, i think we're okay with landing this. if we run into issues, well, we know where to report.

## Summary Pulls in salsa-rs/salsa#934.

ibraheemdev commented Jul 9, 2025

View reviewed changes

use inventory for static ingredient registration

fab2734

ibraheemdev force-pushed the ibraheem/inventory branch from 502e237 to fab2734 Compare July 9, 2025 20:21

remove unnecessary synchronization from memo tables

823d1c2

use global ingredient caches for database-independent ingredients

ab5744f

ibraheemdev commented Jul 10, 2025

View reviewed changes

ibraheemdev mentioned this pull request Jul 10, 2025

Update salsa astral-sh/ruff#19258

Merged

ibraheemdev force-pushed the ibraheem/inventory branch from 2fff0f3 to 407f43b Compare July 15, 2025 05:13

add manual ingredient registration API

842d7c2

ibraheemdev force-pushed the ibraheem/inventory branch from 407f43b to 842d7c2 Compare July 15, 2025 05:16

MichaReiser reviewed Jul 15, 2025

View reviewed changes

Comment thread src/database_impl.rs Outdated

MichaReiser reviewed Jul 15, 2025

View reviewed changes

ibraheemdev force-pushed the ibraheem/inventory branch from 9cd1cc8 to 678609e Compare July 15, 2025 21:57

remove static ingredient index optimization when manual registration …

901868e

…is in use

ibraheemdev force-pushed the ibraheem/inventory branch from 678609e to 901868e Compare July 15, 2025 21:58

MichaReiser approved these changes Jul 16, 2025

View reviewed changes

Comment thread src/ingredient_cache.rs Outdated

MichaReiser requested review from Veykril and davidbarsky July 16, 2025 06:50

fix atomic imports

6fcdad5

davidbarsky approved these changes Jul 17, 2025

View reviewed changes

simplify ingredient caches

21838e4

MichaReiser added this pull request to the merge queue Jul 18, 2025

Merged via the queue into salsa-rs:master with commit dba66f1 Jul 18, 2025
12 checks passed

github-actions Bot mentioned this pull request Jul 18, 2025

chore: release v0.24.0 #929

Merged

ibraheemdev added a commit to astral-sh/ruff that referenced this pull request Jul 18, 2025

Update salsa (#19258)

e6e029a

## Summary Pulls in salsa-rs/salsa#934.

github-actions Bot mentioned this pull request Oct 9, 2025

chore: release v0.25.0 #1002

Merged

MichaReiser mentioned this pull request Oct 13, 2025

Reduce size of WASM builds astral-sh/ruff#20845

Open

KotlinIsland pushed a commit to KotlinIsland/basedpython that referenced this pull request May 1, 2026

Update salsa (#19258)

9fa20e2

## Summary Pulls in salsa-rs/salsa#934.

Conversation

ibraheemdev commented Jul 9, 2025

Uh oh!

netlify Bot commented Jul 9, 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!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq Bot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #934 will improve performances by 29.04%

Summary

Benchmarks breakdown

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ibraheemdev commented Jul 9, 2025

Uh oh!

Darksonn commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibraheemdev commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibraheemdev commented Jul 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

MichaReiser left a comment • 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!

Uh oh!

davidbarsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented Jul 9, 2025 •

edited

Loading

codspeed-hq Bot commented Jul 9, 2025 •

edited

Loading

ibraheemdev commented Jul 9, 2025 •

edited

Loading

ibraheemdev commented Jul 10, 2025 •

edited

Loading

MichaReiser left a comment •

edited

Loading