Skip to content

Conversation

@mhauru
Copy link
Member

@mhauru mhauru commented Nov 3, 2025

Closes #202

I tried to come up with a version of the MWE in that issue that could be put into tests, but failed. My hope was to write a slow method for particular CacheKeys, not all. But Base.cache falls back on this:

hash(@nospecialize(x), h::UInt) = hash_uint(3h - objectid(x))  # On 1.12.1
hash(@nospecialize(data), h::UInt) = hash(objectid(data), h)  # On main

which doesn't give me much surface to hold onto. Hence no tests added.

That implementation of hash makes me extra confused as to how we can reliably hit this in Turing.jl tests. That hash should take no time at all.

@mhauru mhauru requested a review from penelopeysm November 3, 2025 18:14
Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! And yeah, this is pretty much impossible to test (much like how I couldn't get an MWE without monkey-patching hash). Quite happy to sign off on it and forget about it.

@penelopeysm
Copy link
Member

penelopeysm commented Nov 3, 2025

That implementation of hash makes me extra confused as to how we can reliably hit this in Turing.jl tests. That hash should take no time at all.

On the original Turing issue I claimed that

I assume that there's some performance regression on hash in 1.12 that causes this error to manifest, and we were lucky enough that 1.11 was fast enough to not error, but 1.12 wasn't.

so I decided to put it to the test:

using Libtask, Chairmarks
f(x) = x
sig = Tuple{typeof(f),Int}
key = Libtask.CacheKey(Base.get_world_counter(), sig)
@be hash(key)

1.11:

julia> @be hash(key)
Benchmark: 3162 samples with 1013 evaluations
 min    27.229 ns (1 allocs: 16 bytes)
 median 27.724 ns (1 allocs: 16 bytes)
 mean   29.536 ns (1 allocs: 16 bytes, 0.03% gc time)
 max    4.827 μs (1 allocs: 16 bytes, 98.61% gc time)

1.12:

julia> @be hash(key)
Benchmark: 3038 samples with 778 evaluations
 min    35.882 ns (1 allocs: 16 bytes)
 median 36.204 ns (1 allocs: 16 bytes)
 mean   39.878 ns (1 allocs: 16 bytes, 0.06% gc time)
 max    5.440 μs (1 allocs: 16 bytes, 98.54% gc time)

Idk, I guess maybe that's enough to cause problems. Also the Base dictionary code hashes multiple keys in a loop so the difference in time will be scaled multiplicatively too (and I suppose it could be performance regressions in other parts of the Dict code, too, although my naive guess is that hashing is the most time-consuming aspect of Dict code(?)).

@mhauru
Copy link
Member Author

mhauru commented Nov 4, 2025

Nice, good check. I thought about checking the performance effects on Libtask too, but then decided that there's little point because this clearly just needs to be done. If we start optimising Libtask there's other fruit hanging way lower. I can tell you that the totality of the test suite runs noticably, but not horribly slower (7.something seconds vs 8.something, I forget now).

@github-actions
Copy link

github-actions bot commented Nov 4, 2025

Libtask.jl documentation for PR #203 is available at:
https://TuringLang.github.io/Libtask.jl/previews/PR203/

@mhauru
Copy link
Member Author

mhauru commented Nov 4, 2025

I changed the docstring of GlobalMCCache to a comment, because Documenter was complaining that something in copyable_task had a docstring that wasn't included in the docs. This feels like a perverse incentive created by Documenter, but I couldn't reproduce the error locally and don't have time to dig into it, so whateva imma merge this thing.

The other two failures are Turing.jl/Mooncake issues and not new.

@mhauru mhauru merged commit c5dd2bc into main Nov 4, 2025
12 of 14 checks passed
@mhauru mhauru deleted the mhauru/fix-concurrency-cache branch November 4, 2025 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Writing to mc_cache is not thread-safe

3 participants