[ty] Move Salsa caching "down a layer" in `tuple.rs` #19877

AlexWaygood · 2025-08-12T12:56:50Z

Summary

This PR moves the Salsa caching "down a layer" in tuple.rs:

TupleType is no longer a Salsa-tracked struct; instead, it's just a thin wrapper around a Salsa-tracked struct, similar to NominalInstanceType, ProtocolInstanceType and others.
TupleSpec (the type wrapped by TupleType) is no longer a type alias; instead, it's now a Salsa-tracked struct.

The benefits of this are that we need to use many fewer Cows across the ty codebase (which were previously required due to TupleSpec not being Copy), and we can avoid some very complicated trait bounds that we currently have in the signature of TupleType::new() on main.

Test Plan

Existing tests

github-actions · 2025-08-12T12:59:30Z

Diagnostic diff on typing conformance tests

Changes were detected when running ty on typing conformance tests

--- old-output.txt	2025-08-12 16:21:25.297441346 +0000
+++ new-output.txt	2025-08-12 16:21:25.364441662 +0000
@@ -1,5 +1,5 @@
 WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/918d35d/src/function/execute.rs:215:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1f113)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/918d35d/src/function/execute.rs:215:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(5f5e)): execute: too many cycle iterations`
 _directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
 _directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
 _directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__`

github-actions · 2025-08-12T13:00:41Z

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

AlexWaygood · 2025-08-12T13:22:20Z

~~Performance is in the noise: some microbenchmarks are showing a 2% slowdown, but some walltime benchmarks are showing a 2% speedup.~~

The latest version of the PR shows small but consistent improvements of 1-2% on quite a few benchmarks.

crates/ty_python_semantic/src/types/tuple.rs

AlexWaygood · 2025-08-12T14:30:20Z

crates/ty_python_semantic/src/types/tuple.rs

-impl<'db> Type<'db> {
-    pub(crate) fn homogeneous_tuple(db: &'db dyn Db, element: Type<'db>) -> Self {
-        Type::tuple(TupleType::homogeneous(db, element))
-    }
-
-    pub(crate) fn heterogeneous_tuple<I, T>(db: &'db dyn Db, elements: I) -> Self
-    where
-        I: IntoIterator<Item = T>,
-        T: Into<Type<'db>>,
-    {
-        Type::tuple(TupleType::from_elements(
-            db,
-            elements.into_iter().map(Into::into),
-        ))
-    }
-
-    pub(crate) fn empty_tuple(db: &'db dyn Db) -> Self {
-        Type::tuple(Some(TupleType::empty(db)))
-    }
-}


These are moved to instance.rs because we can do the same things with less indirection from that module (we can access private implementation details of NominalInstanceType from that module)

AlexWaygood · 2025-08-12T15:00:16Z

(CI failures are all due to the GitHub outage)

dcreager

As an alternative, we might consider just making Tuple (and therefore TupleSpec in the world where it's still an alias) easier/cheaper to clone. On the assumption that most tuple specs are small, we could use SmallVecs inside of Tuple and friends, and just clone the TupleSpecs where we need to. Then you'd get rid of the Cows by returning owned TupleSpecs everywhere.

we can avoid some very complicated trait bounds that we currently have in the signature of TupleType::new() on main.

See below; this was only there to let us call the salsa interning logic with a reference, but if TupleSpec becomes cheap to clone, that's no longer needed.

dcreager · 2025-08-12T14:54:05Z

crates/ty_python_semantic/src/types/tuple.rs

-        T: Borrow<TupleSpec<'db>> + Hash + salsa::plumbing::interned::Lookup<TupleSpec<'db>>,
-        TupleSpec<'db>: salsa::plumbing::interned::HashEqLike<T>,


These bounds were only here to allow callers to pass in a reference to a TupleSpec or an owned one. If you pass in a reference, we could avoid the clone if the result ended up being None, or if there was already an interned TupleType with an equivalent TupleSpec.

When I first added this code, there were some hot path calls where that seemed to make a noticeable difference in performance. But looking again now, that no longer seems to be the case — in most places we're already calling with an owned TupleSpec. So, given that, you could also get rid of these bounds by just taking in an owned TupleSpec, and requiring the caller to clone before calling if they have a reference.

AlexWaygood · 2025-08-12T15:39:30Z

As an alternative, we might consider just making Tuple (and therefore TupleSpec in the world where it's still an alias) easier/cheaper to clone. On the assumption that most tuple specs are small, we could use SmallVecs inside of Tuple and friends, and just clone the TupleSpecs where we need to. Then you'd get rid of the Cows by returning owned TupleSpecs everywhere.

Hmm... we could, but I'm not sure I see the advantages of doing it that way. If we did this we'd have to keep TupleType as a Salsa-interned struct, or NominalInstanceType would no longer be Copy (and if NominalInstanceType were no longer Copy, Type would no longer be Copy). Our size constraints mean we have to intern either one or both of TupleType and TupleSpec.

What are the benefits of your proposal relative to my current PR?

dcreager · 2025-08-12T18:45:45Z

Hmm... we could, but I'm not sure I see the advantages of doing it that way. If we did this we'd have to keep TupleType as a Salsa-interned struct, or NominalInstanceType would no longer be Copy (and if NominalInstanceType were no longer Copy, Type would no longer be Copy). Our size constraints mean we have to intern either one or both of TupleType and TupleSpec.

What are the benefits of your proposal relative to my current PR?

Yep, with my suggestion, TupleType would remain interned, like it was before. We've found (e.g. in #19867) that interning fewer things is generally a good tradeoff, since interning requires cross-thread synchronization. My hunch is that we'll intern more values if we do the caching at the TupleSpec level. try_iterate is an example: it returns a TupleSpec, and there are call sites where we do not wrap that in a TupleType. So with this PR, we now have to intern the result of that function in places where we didn't have to before.

AlexWaygood · 2025-08-12T19:15:44Z

Yep, with my suggestion, TupleType would remain interned, like it was before. We've found (e.g. in #19867) that interning fewer things is generally a good tradeoff, since interning requires cross-thread synchronization. My hunch is that we'll intern more values if we do the caching at the TupleSpec level. try_iterate is an example: it returns a TupleSpec, and there are call sites where we do not wrap that in a TupleType. So with this PR, we now have to intern the result of that function in places where we didn't have to before.

Hmmmmmm, I see, fair enough. I'm slightly sceptical that #19867 is indicative of a broader problem when it comes to lock contention -- my instinct is that that issue is quite sui generis. But I do agree that we should probably try to intern the minimum number of objects in the Salsa cache, since it leads to higher memory usage overall; and I think you're right that this PR in its current form probably leads to us interning a number of TupleSpecs that don't really need to be interned at the end of the day.

MichaReiser · 2025-08-13T06:53:27Z

Hmmmmmm, I see, fair enough. I'm slightly sceptical that #19867 is indicative of a broader problem when it comes to lock contention -- my instinct is that that issue is quite sui generis.

I guess that depends on the perspective :) It is specific in the sense that it is only an issue when the same interned values (of a specific interned struct) are used in a very hot code path. It isn't an issue if many threads intern many different interned structs because Salsa shards the locks by value, meaning most threads will end up acquiring different locks.

I don't understand try_iterate well enough to understand if this falls into the former (we often call it on very few tuples) or the latter (we call it on many different tuples) bucket.

The other part that makes #19867 somewhat unique is that it's a very hot methods. I'm not sure if try_iterate is equally hot. Either way. It's something you can test easily yourself. Build ty with the profiling profile (cargo build --bin ty --profile profiling) and run samply record ./ruff/target/profiling/ty check ... on a large repository. Then invert the stack frames and verify if there are now more frames attributed to the mutex lock cold path (see the PR for an example)

AlexWaygood · 2025-08-13T09:28:30Z

I find @dcreager's point pretty convincing: most of the paths in Type::try_iterate end up creating a fresh TupleSpec rather than reusing an (already borrowed one), because it returns a TupleSpec for all iterable types (not just tuples) in order to describe the behaviour these types have when they're unpacked. That means we'll end up interning (possibly many) TupleSpecs unnecessarily in many cases with the design proposed in this PR.

I don't love how viral our current design makes all the Cow types, but I'm persuaded that it's probably the least-bad option for now. I'll split out some of the other simplifications/improvements in this PR into a standalone PR.

Thanks @dcreager!

dcreager · 2025-08-13T15:13:36Z

I don't love how viral our current design makes all the Cow types

We can still try to get rid of the Cows, if we want — we'd just have to make Tuple (and therefore TupleSpec) cheap to clone, so that we can return owned values all the time. They're just a couple of boxed slices of Types, so they might be cheap enough to copy already!

AlexWaygood · 2025-08-13T15:17:08Z

Right! But I consider the price of a few ugly 🐄s worth it to avoid some unnecessary clones, even if those clones wouldn't actually be that expensive 😆

AlexWaygood added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Aug 12, 2025

AlexWaygood force-pushed the alex/tuple-salsa-cache branch from 3cd18a2 to aea07ae Compare August 12, 2025 13:07

MichaReiser reviewed Aug 12, 2025

View reviewed changes

crates/ty_python_semantic/src/types/tuple.rs Outdated Show resolved Hide resolved

AlexWaygood force-pushed the alex/tuple-salsa-cache branch 3 times, most recently from bd517f2 to 4676953 Compare August 12, 2025 14:24

AlexWaygood marked this pull request as ready for review August 12, 2025 14:29

AlexWaygood requested review from carljm, dcreager and sharkdp as code owners August 12, 2025 14:29

AlexWaygood commented Aug 12, 2025

View reviewed changes

AlexWaygood requested a review from MichaReiser August 12, 2025 14:30

AlexWaygood force-pushed the alex/tuple-salsa-cache branch 2 times, most recently from cb8fe72 to 2f573e5 Compare August 12, 2025 14:54

dcreager reviewed Aug 12, 2025

View reviewed changes

[ty] Move Salsa caching "down a layer" in tuple.rs

df2a43a

AlexWaygood force-pushed the alex/tuple-salsa-cache branch from 2f573e5 to df2a43a Compare August 12, 2025 16:19

AlexWaygood closed this Aug 13, 2025

AlexWaygood deleted the alex/tuple-salsa-cache branch August 13, 2025 09:28

AlexWaygood mentioned this pull request Aug 13, 2025

[ty] Various minor cleanups to tuple internals #19891

Merged

		T: Borrow<TupleSpec<'db>> + Hash + salsa::plumbing::interned::Lookup<TupleSpec<'db>>,
		TupleSpec<'db>: salsa::plumbing::interned::HashEqLike<T>,

[ty] Move Salsa caching "down a layer" in tuple.rs #19877

[ty] Move Salsa caching "down a layer" in tuple.rs #19877

Uh oh!

Conversation

AlexWaygood commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Diagnostic diff on typing conformance tests

Uh oh!

github-actions bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

AlexWaygood commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlexWaygood Aug 12, 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!

AlexWaygood commented Aug 12, 2025

Uh oh!

dcreager left a comment

Choose a reason for hiding this comment

Uh oh!

dcreager Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcreager commented Aug 12, 2025

Uh oh!

AlexWaygood commented Aug 12, 2025

Uh oh!

MichaReiser commented Aug 13, 2025

Uh oh!

AlexWaygood commented Aug 13, 2025

Uh oh!

dcreager commented Aug 13, 2025

Uh oh!

AlexWaygood commented Aug 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[ty] Move Salsa caching "down a layer" in `tuple.rs` #19877

[ty] Move Salsa caching "down a layer" in `tuple.rs` #19877

AlexWaygood commented Aug 12, 2025 •

edited

Loading

github-actions bot commented Aug 12, 2025 •

edited

Loading

github-actions bot commented Aug 12, 2025 •

edited

Loading

`mypy_primer` results

AlexWaygood commented Aug 12, 2025 •

edited

Loading

AlexWaygood Aug 12, 2025 •

edited

Loading

AlexWaygood commented Aug 12, 2025 •

edited

Loading