methods: add a lock around method table changes #52997
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vtjnash
force-pushed
the
jn/lock-method-add
branch
from
f5e0c2e to
b060560
Compare
gbaraldi
reviewed
Jan 24, 2024
gbaraldi
reviewed
Jan 24, 2024
vtjnash
force-pushed
the
jn/lock-method-add
branch
from
b060560 to
7cc03c5
Compare
vchuravy
reviewed
Jan 24, 2024
vchuravy
reviewed
Jan 24, 2024
vchuravy
reviewed
Jan 24, 2024
vtjnash
force-pushed
the
jn/lock-method-add
branch
from
7cc03c5 to
bbe0c93
Compare
Ensures that only one update can occur simultaneously, and that the acquire-release on the world-age will be sequenced after all of the invalidations to the caches. It should now be safe to add methods in parallel, concurrently with running code. However, there are still no locks here to ensure that only one module is deserialized at a time, which means that is still unsafe. As future work, all of the method inserting and (separately) the invalidation work could run in parallel after loading an image, since there is a fine-grained lock on each individual part of that, as well as the big lock so that only one module at a time can load methods. The min/max are always written behind a lock, but they are not usually read from behind a lock, so add relaxed atomic markers to make any analyzers happy.
vtjnash
force-pushed
the
jn/lock-method-add
branch
from
bbe0c93 to
98a1269
Compare
vchuravy
approved these changes
Jan 27, 2024
Member
|
Daily PkgEval started failing on GeoParquet.jl and SubpixelRegistration.jl; I haven't bisected or reduced but this PR seems related: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_date/2024-01/27/GeoParquet.primary.log rr traces included |
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
Jan 29, 2024
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
Jan 29, 2024
Member
Author
|
adding this assert catches the mistake even earlier: Looks like there are 2, very close to being almost identical, CodeInstance objects for this MethodInstance, which is broken in this part of the code. And if we look this up in |
Member
Author
|
Looks like what happened is during serialization of Parquet2 there were indeed 2 distinct MethodInstance objects But during deserialization of it, these got merged into a single object. So basically, it is version of this bug: Though also possibly a case that deserialization should just accept and ignore this bug. |
Member
|
This causes a test failure in Pluto.jl, I wonder why this was not caught by PkgEval? Running |
vchuravy
added a commit
to JuliaGPU/GPUCompiler.jl
that referenced
this pull request
Feb 8, 2024
Member
|
Does someone know why the failing Pluto tests were not caught by PkgEval? It looks like other packages were checked (#52997 (comment)). |
Member
Author
|
It can be a somewhat stochastic failure, since it depends on the specific order in which hashes are inserted into the dictionary, which can depend on the memory layout |
Member
|
Our case was not stochastic, we did |
Member
Author
|
Mutating that field is undefined behavior because it changes internal state that corrupts the internal invariants regarding the program representation and inference, so it is not expected to be safe |
Member
|
After this PR was merged, the Pluto tests failed with: but this was not caught by PkgEval and I'm wondering why. We fixed it afterwards in fonsp/Pluto.jl#2807 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ensures that only one update can occur simultaneously by adding a lock around it, and that the acquire-release on the world-age will be sequenced after all of the invalidations to the caches, by updating it last.
It should now be safe to add methods in parallel, concurrently with running code. However, there are still no locks here to ensure that only one module is deserialized at a time, which means that parallel require calls are still unsafe. (and also because the loading.jl code is currently a thread-safety disaster zone)
As future work, all of the method inserting and (separately) the validation/insertion work could run in parallel after loading an image, since there is a fine-grained lock on each individual part of that, as well as the big lock so that only one module at a time can load methods.
The min/max fields are always written behind a lock, but they are not usually read from behind a lock, so add relaxed atomic markers to all uses of that field to make any data-race analysis tools happy. These are never used in a comparison with a value greater than
jl_atomic_load_acquire(&jl_world_counter), and therefore the results achieved are consistent.