Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow freezing of Core.MethodTables #56143

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

fatteneder
Copy link
Member

Implements the idea from #54138 (comment).

A follow-up PR would apply it to some relevant functions in Base.

src/gf.c Outdated Show resolved Hide resolved
base/reflection.jl Outdated Show resolved Hide resolved
@nsajko nsajko added the feature Indicates new feature / enhancement requests label Oct 15, 2024
Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

It could be nice to have analogous functionality for unsealing a method table as well.

base/reflection.jl Outdated Show resolved Hide resolved
base/reflection.jl Outdated Show resolved Hide resolved
test/reflection.jl Outdated Show resolved Hide resolved
test/reflection.jl Show resolved Hide resolved
@fatteneder fatteneder changed the title Allow sealing of Core.MethodTables Allow freezing of Core.MethodTables Oct 17, 2024
src/gf.c Outdated Show resolved Hide resolved
base/runtime_internals.jl Outdated Show resolved Hide resolved
base/runtime_internals.jl Outdated Show resolved Hide resolved
base/runtime_internals.jl Outdated Show resolved Hide resolved
base/runtime_internals.jl Outdated Show resolved Hide resolved
test/reflection.jl Outdated Show resolved Hide resolved
@fatteneder
Copy link
Member Author

i guess the analyzegc failure is due to me not being familiar with atomics ordering, will check tomorrow

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM, but would probably be good to hear from @JeffBezanson or @Keno or triage if they have thoughts about the API here? The gc-analyzer looks like you can fix it by adding assert(mt != NULL) there. I don't really know why it is triggering from this, but that should make it happy

There is also a core test that needs to be fixed (adding this field to the list of atomic fields)

@fatteneder
Copy link
Member Author

The gc-analyzer looks like you can fix it by adding assert(mt != NULL) there. I don't really know why it is triggering from this, but that should make it happy

Thanks, that fixed it it locally. I could not make sense out of this either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants