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

More thread-safe GC (another version) #534

Closed
wants to merge 3 commits into from
Closed

More thread-safe GC (another version) #534

wants to merge 3 commits into from

Conversation

cjdoris
Copy link
Collaborator

@cjdoris cjdoris commented Jul 29, 2024

Another alternative to #520 and #529.

This version just changes the finalizer of Py (and PyObjectArray) objects to:

  • Decref the pointer if the GIL is currently held.
  • Re-attach the finalizer if the GIL is not currently held - so the GC will try to finalize it again in the future, hopefully on the correct thread next time.

It's a lot simpler - the PythonCall.GC module is basically blank now, with enable() and disable() being no-ops.

@ericphanson
Copy link
Contributor

From the PyCall version: JuliaPy/PyCall.jl#883 (comment)

note it is likely to be called again very soon, and probably on the same thread

which could be problematic here. It doesn’t seem nice to have an infinite loop of finalizers, if they keep getting scheduled on the same thread.

To me some kind of queue makes the most sense. However probably anything is better than the segfaults of the status quo.

@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 29, 2024

It doesn’t seem nice to have an infinite loop of finalizers, if they keep getting scheduled on the same thread.

Agreed, although it seems unlikely - I think for this to occur you'd have to be doing all your allocating on one thread (and for that not to be on the thread holding the GIL). I think the other approaches have a similar issue though. Maybe an explicit queue (as in #520 or #529) is indeed nicer.

I'll run some benchmarks and see if that helps decide.

@ericphanson ericphanson mentioned this pull request Aug 1, 2024
@cjdoris
Copy link
Collaborator Author

cjdoris commented Aug 2, 2024

Closing in favour of #529

@cjdoris cjdoris closed this Aug 2, 2024
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.

2 participants