Skip to content

Conversation

@Keno
Copy link
Member

@Keno Keno commented Apr 2, 2025

After:

julia> convert(Core.Binding, GlobalRef(Base, :Intrinsics))
Binding Base.Intrinsics
   617:∞ - implicit `using` resolved to constant Core.Intrinsics
   0:616 - undefined binding - guard entry

julia> convert(Core.Binding, GlobalRef(Base, :Math))
Binding Base.Math
   22128:∞ - constant binding to Base.Math
   0:22127 - backdated constant binding to Base.Math

There is a bit of trickiness here. In particular, the question is, "when do we check" whether the partition next to the one we currently looked at happens to have the same implicit resolution as our current one. The most obvious answer is that we should do it on access, but in practice that would require essentially scanning back and considering every possible world age state at every lookup. This is undesirable - the lookup is not crazy expensive, but it can add up and most world ages we never touch, so it is also wasteful.

This instead implements a different approach where we only perform the resolution for world ages that somebody actually asked about, but can then subsequently merge partitions if we do find that they are identical. The logic for that is a bit involved, since we need to be careful to keep the datastructure valid at every point, but does address the issue.

Fixes #57923

@Keno Keno requested a review from Copilot April 2, 2025 19:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the mechanism for merging adjacent implicit binding partitions to improve lookup efficiency and reduce unnecessary work. Key changes include refactoring the merging logic in module.c, converting the min_world field to atomic in julia.h, and updating binding partition field metadata in jltypes.c.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
src/module.c Refactored atomic operations and merging logic for binding partitions.
src/julia.h Changed min_world to an atomic type to support concurrent access.
src/jltypes.c Updated bitmask configuration for atomic fields and added constant field settings.
Files not reviewed (1)
  • test/rebinding.jl: Language not supported
Comments suppressed due to low confidence (2)

src/jltypes.c:3269

  • [nitpick] The comment describing the atomic fields is inconsistent with the previous version. Please update the comment to accurately reflect that fields 1, 2, 3, and 4 are now set as atomic.
const static uint32_t binding_partition_atomicfields[] = { 0b01111 }; // Set fields 1, 2, 3, 4 as atomic

src/module.c:157

  • [nitpick] Consider adding tests that validate the behavior of this CAS loop under concurrent modifications to ensure the retry logic for merging partitions functions correctly.
if (!jl_atomic_cmpswap(&prev->next, &next, nextnext)) {

@KristofferC KristofferC added the backport 1.12 Change should be backported to release-1.12 label Apr 2, 2025
After:
```
julia> convert(Core.Binding, GlobalRef(Base, :Intrinsics))
Binding Base.Intrinsics
   617:∞ - implicit `using` resolved to constant Core.Intrinsics
   0:616 - undefined binding - guard entry

julia> convert(Core.Binding, GlobalRef(Base, :Math))
Binding Base.Math
   22128:∞ - constant binding to Base.Math
   0:22127 - backdated constant binding to Base.Math
```

There is a bit of trickiness here. In particular, the question is,
"when do we check" whether the partition next to the one we currently
looked at happens to have the same implicit resolution as our current
one. The most obvious answer is that we should do it on access, but in
practice that would require essentially scanning back and considering
every possible world age state at every lookup. This is undesirable -
the lookup is not crazy expensive, but it can add up and most world
ages we never touch, so it is also wasteful.

This instead implements a different approach where we only perform
the resolution for world ages that somebody actually asked about,
but can then subsequently merge partitions if we do find that they
are identical. The logic for that is a bit involved, since we need
to be careful to keep the datastructure valid at every point, but
does address the issue.

Fixes #57923
@Keno Keno merged commit 1c3878c into master Apr 4, 2025
5 of 7 checks passed
@Keno Keno deleted the kf/57923 branch April 4, 2025 01:10
@KristofferC KristofferC mentioned this pull request Apr 4, 2025
51 tasks
KristofferC pushed a commit that referenced this pull request Apr 16, 2025
After:
```
julia> convert(Core.Binding, GlobalRef(Base, :Intrinsics))
Binding Base.Intrinsics
   617:∞ - implicit `using` resolved to constant Core.Intrinsics
   0:616 - undefined binding - guard entry

julia> convert(Core.Binding, GlobalRef(Base, :Math))
Binding Base.Math
   22128:∞ - constant binding to Base.Math
   0:22127 - backdated constant binding to Base.Math
```

There is a bit of trickiness here. In particular, the question is, "when
do we check" whether the partition next to the one we currently looked
at happens to have the same implicit resolution as our current one. The
most obvious answer is that we should do it on access, but in practice
that would require essentially scanning back and considering every
possible world age state at every lookup. This is undesirable - the
lookup is not crazy expensive, but it can add up and most world ages we
never touch, so it is also wasteful.

This instead implements a different approach where we only perform the
resolution for world ages that somebody actually asked about, but can
then subsequently merge partitions if we do find that they are
identical. The logic for that is a bit involved, since we need to be
careful to keep the datastructure valid at every point, but does address
the issue.

Fixes #57923

(cherry picked from commit 1c3878c)
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Apr 25, 2025
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.

Exploding lists of identical binding partitions?

3 participants