Be more selective when invalidating code instances #57617
Closed
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
Keno
force-pushed
the
kf/nogetpropertyinvalidate
branch
from
0360167 to
ae2914a
Compare
On master, when we invalidate a CodeInstance, we also invalidate the entire associated MethodInstance. However, this is highly problematic, because we have a lot of CodeInstances that are associated with `getproperty(::Module, ::Symbol)` through constant propagation. If one of these CodeInstances gets invalidated (e.g. because the resolution of const-propagated M.s binding changed), it would invalidate essentially the entire world. Prevent this by re-checking the forward edges list to make sure that the code instance we're invalidating is actually in there.
Keno
force-pushed
the
kf/nogetpropertyinvalidate
branch
from
ae2914a to
4ef7c99
Compare
Keno
added a commit
that referenced
this pull request
Mar 3, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in #57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (#57617).
Keno
added a commit
that referenced
this pull request
Mar 3, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in #57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (#57617).
Keno
added a commit
that referenced
this pull request
Mar 3, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in #57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (#57617).
Keno
added a commit
that referenced
this pull request
Mar 3, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in #57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (#57617).
Keno
added a commit
that referenced
this pull request
Mar 4, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in #57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (#57617).
|
Updated version merged in #57625. |
KristofferC
pushed a commit
that referenced
this pull request
Mar 4, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in #57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (#57617). (cherry picked from commit 274d80e)
serenity4
pushed a commit
to serenity4/julia
that referenced
this pull request
May 1, 2025
Our implicit edge tracking for bindings does not explicitly store any edges for bindings in the *current* module. The idea behind this is that this is a good time-space tradeoff for validation, because substantially all binding references in a module will be to its defining module, while the total number of methods within a module is limited and substantially smaller than the total number of methods in the entire system. However, we have an issue where the code that stores these edges and the invalidation code disagree on which module is the *current* one. The edge storing code was using the module in which the method was defined, while the invalidation code was using the one in which the MethodTable is defined. With these being misaligned, we can miss necessary invalidations. Both options are in principle possible, but I think the former is better, because the module in which the method is defined is also the module that we are likely to have a lot of references to (since they get referenced implicitly by just writing symbols in the code). However, this presents a problem: We don't actually have a way to iterate all the methods defined in a particular module, without just doing the brute force thing of scanning all methods and filtering. To address this, build on the deferred scanning code added in JuliaLang#57615 to also add any scanned modules to an explicit list in `Module`. This costs some space, but only proportional to the number of defined methods, (and thus proportional to the written source code). Note that we don't actually observe any issues in the test suite on master due to this bug. However, this is because we are grossly over-invalidating, which hides the missing invalidations from this issue (JuliaLang#57617).
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.
On master, when we invalidate a CodeInstance, we also invalidate the entire associated MethodInstance. However, this is highly problematic, because we have a lot of CodeInstances that are associated with
getproperty(::Module, ::Symbol)through constant propagation. If one of these CodeInstances gets invalidated (e.g. because the resolution of const-propagated M.s binding changed), it would invalidate essentially the entire world.Prevent this by re-checking the forward edges list to make sure that the code instance we're invalidating is actually in there.