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.

Sign up for GitHub

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

Jump to bottom

workaround for dict access issue in a finalizer #14456

Merged
merged 1 commit into from
Dec 22, 2015
Merged

workaround for dict access issue in a finalizer #14456

merged 1 commit into from
Dec 22, 2015

Conversation

amitmurthy
Copy link
Contributor

This is a workaround for #14295 and #14445

@amitmurthy amitmurthy mentioned this pull request Dec 21, 2015
Merged
@amitmurthy
Copy link
Contributor Author

@yuyichao can you think of a different/better workaround? I'll merge this and #14457 in a day in the absence of any other approaches.

@yuyichao
Copy link
Contributor

Can't think of a better workaround without fixing our dict implementation

amitmurthy added a commit that referenced this pull request Dec 22, 2015
@amitmurthy
Merge pull request #14456 from JuliaLang/amitm/dictfinalizer
0a1c7f1 
workaround for dict access issue in a finalizer
@amitmurthy amitmurthy merged commit 0a1c7f1 into master Dec 22, 2015
@amitmurthy amitmurthy deleted the amitm/dictfinalizer branch December 22, 2015 03:42
@tkelman
Copy link
Contributor

tkelman commented Dec 22, 2015

Can a test case be added that would fail without this change?

@tkelman tkelman mentioned this pull request Dec 22, 2015
Closed
@tkelman tkelman added the needs tests Unit tests are required for this change label Dec 22, 2015
@amitmurthy
Copy link
Contributor Author

Something like this

julia> @time for n = 1:10^6
          fetch(@spawnat myid() myid())
       end
  8.801960 seconds (50.66 M allocations: 1.747 GB, 16.59% gc time)

But it will add to both the memory pressure as well as elapsed time on already stressed CI infra. It is more of a stress test, rather than unit tests that the test suite is comprised of.

@tkelman
Copy link
Contributor

tkelman commented Dec 22, 2015

Maybe under JULIA_TESTFULL then?

@amitmurthy amitmurthy mentioned this pull request Mar 4, 2016
Merged
This was referenced Apr 18, 2016
Closed
Closed
vtjnash added a commit that referenced this pull request May 4, 2016
@vtjnash
don't corrupt the identity of AbstractRemoteRef in their finalizers
8d1970e 
this made it unreliable for the WeakKeyDict these are typically put in (client_refs)
to have trouble finding them to cleanup the dictionary later
since their hash and identity changed

fixes #15923
reverts workaround #14456 (doesn't break #14295 due to previous commit)
may also fix #16091
amitmurthy added a commit that referenced this pull request May 23, 2016
@amitmurthy
increase loop count in test for #14456 under JULIA_TESTFULL
23b8a7a
@amitmurthy amitmurthy mentioned this pull request May 23, 2016
Merged
vtjnash added a commit that referenced this pull request Jul 26, 2016
@vtjnash
make WeakKeyDict finalizer usage gc-safe
6478db7 
also use this `client_refs.lock` to protect other data-structures from
being interrupted by finalizers, in the multi.jl logic

we may want to start indicating which mutable data-structures are safe
to call from finalizers, since generally that isn't possible

to make a finalizer API gc-safe, that code should observe the standard
thread-safe restrictions (there's no guarantee of which thread it'll run on),

plus, if the data-structures uses locks for synchronization,
use the `islocked` pattern (demonstrated herein) in the `finalizer`
to re-schedule the finalizer when the mutable data-structure is not
available for mutation.
this ensures that the lock cannot be acquired recursively,
and furthermore, this pattern will continue to work if finalizers
get moved to their own separate thread.

close #14445
fix #16550
reverts workaround #14456 (shouldn't break #14295, due to new locks)
should fix #16091 (with #17619)
vtjnash added a commit that referenced this pull request Jul 26, 2016
@vtjnash
make WeakKeyDict finalizer usage gc-safe
7f1cad4 
also use this `client_refs.lock` to protect other data-structures from
being interrupted by finalizers, in the multi.jl logic

we may want to start indicating which mutable data-structures are safe
to call from finalizers, since generally that isn't possible

to make a finalizer API gc-safe, that code should observe the standard
thread-safe restrictions (there's no guarantee of which thread it'll run on),

plus, if the data-structures uses locks for synchronization,
use the `islocked` pattern (demonstrated herein) in the `finalizer`
to re-schedule the finalizer when the mutable data-structure is not
available for mutation.
this ensures that the lock cannot be acquired recursively,
and furthermore, this pattern will continue to work if finalizers
get moved to their own separate thread.

close #14445
fix #16550
reverts workaround #14456 (shouldn't break #14295, due to new locks)
should fix #16091 (with #17619)
vtjnash added a commit that referenced this pull request Aug 4, 2016
@vtjnash
make WeakKeyDict finalizer usage gc-safe
30a6f8b 
also use this `client_refs.lock` to protect other data-structures from
being interrupted by finalizers, in the multi.jl logic

we may want to start indicating which mutable data-structures are safe
to call from finalizers, since generally that isn't possible

to make a finalizer API gc-safe, that code should observe the standard
thread-safe restrictions (there's no guarantee of which thread it'll run on),

plus, if the data-structures uses locks for synchronization,
use the `islocked` pattern (demonstrated herein) in the `finalizer`
to re-schedule the finalizer when the mutable data-structure is not
available for mutation.
this ensures that the lock cannot be acquired recursively,
and furthermore, this pattern will continue to work if finalizers
get moved to their own separate thread.

close #14445
fix #16550
reverts workaround #14456 (shouldn't break #14295, due to new locks)
should fix #16091 (with #17619)
vtjnash added a commit that referenced this pull request Aug 5, 2016
@vtjnash
make WeakKeyDict finalizer usage gc-safe
cd8be65 
also use this `client_refs.lock` to protect other data-structures from
being interrupted by finalizers, in the multi.jl logic

we may want to start indicating which mutable data-structures are safe
to call from finalizers, since generally that isn't possible

to make a finalizer API gc-safe, that code should observe the standard
thread-safe restrictions (there's no guarantee of which thread it'll run on),

plus, if the data-structures uses locks for synchronization,
use the `islocked` pattern (demonstrated herein) in the `finalizer`
to re-schedule the finalizer when the mutable data-structure is not
available for mutation.
this ensures that the lock cannot be acquired recursively,
and furthermore, this pattern will continue to work if finalizers
get moved to their own separate thread.

close #14445
fix #16550
reverts workaround #14456 (shouldn't break #14295, due to new locks)
should fix #16091 (with #17619)
tkelman pushed a commit that referenced this pull request Aug 11, 2016
@vtjnash @tkelman
make WeakKeyDict finalizer usage gc-safe
b673748 
also use this `client_refs.lock` to protect other data-structures from
being interrupted by finalizers, in the multi.jl logic

we may want to start indicating which mutable data-structures are safe
to call from finalizers, since generally that isn't possible

to make a finalizer API gc-safe, that code should observe the standard
thread-safe restrictions (there's no guarantee of which thread it'll run on),

plus, if the data-structures uses locks for synchronization,
use the `islocked` pattern (demonstrated herein) in the `finalizer`
to re-schedule the finalizer when the mutable data-structure is not
available for mutation.
this ensures that the lock cannot be acquired recursively,
and furthermore, this pattern will continue to work if finalizers
get moved to their own separate thread.

close #14445
fix #16550
reverts workaround #14456 (shouldn't break #14295, due to new locks)
should fix #16091 (with #17619)

(cherry picked from commit cd8be65)
ref #16204
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
@vtjnash @mfasi
make WeakKeyDict finalizer usage gc-safe
e9bb7fe 
also use this `client_refs.lock` to protect other data-structures from
being interrupted by finalizers, in the multi.jl logic

we may want to start indicating which mutable data-structures are safe
to call from finalizers, since generally that isn't possible

to make a finalizer API gc-safe, that code should observe the standard
thread-safe restrictions (there's no guarantee of which thread it'll run on),

plus, if the data-structures uses locks for synchronization,
use the `islocked` pattern (demonstrated herein) in the `finalizer`
to re-schedule the finalizer when the mutable data-structure is not
available for mutation.
this ensures that the lock cannot be acquired recursively,
and furthermore, this pattern will continue to work if finalizers
get moved to their own separate thread.

close JuliaLang#14445
fix JuliaLang#16550
reverts workaround JuliaLang#14456 (shouldn't break JuliaLang#14295, due to new locks)
should fix JuliaLang#16091 (with JuliaLang#17619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs tests Unit tests are required for this change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amitmurthy @yuyichao @tkelman