Re: Per-generation lists of weak pointers
On Sat, May 4, 2013 at 8:50 AM, Edward Z. Yang
Akio, your derefWeak WHITEHOLE fix looks really weird. I don't know what the right pattern is, but it seems like asking for trouble when there are multiple concurrent derefs:
if (info == stg_WHITEHOLE_info) { ("ptr" info) = ccall lockClosure(w "ptr"); unlockClosure(w, info); }
I don't see what the problem is, could you elaborate? The purpose of the fix was to prevent a sequence like this: - w is a dead weak pointer. - Thread A: finalizeWeak# w. - Thread A: finalizeWeak# calls lockClosure(w), overwriting w->info with stg_WHITEHOLE_info. - Thread B: deRefWeak# w - Thread B: deRefWeak# sees stg_WHITEHOLE_info, and since it's not the same as stg_DEAD_WEAK_info, it thinks w is alive. The problem was that if deRefWeak# saw stg_WHITEHOLE_info, it was not clear whether the weak pointer was alive or not. So my fix adds a call to lockClosure, which never returns stg_WHITEHOLE_info.
addForeignPtrFinalizer retries in this case.
This can't be right; a dead weak pointer always stays dead, so won't this infinite loop?
No. When addForeignPtrFinalizer retries, it will use a new Weak# object, because foreignPtrFinalizer must have been replaced the content of the IORef.
I haven't got around to looking at this, but I see Edward is on the case with some code review. Do you think I should look at it before it goes in?
Now you're asking for it :) I would always be interested in seeing if I missed anything.
Cheers, Edward
Akio, I looked at your first patch, which mostly seems good. The sequence at the beginning of deRefWeak certainly looks strange - I think it's ok, but it needs a comment. One thing I'm confused about is the handling of DEAD_WEAK pointers. You removed the StgDeadWeak struct - how is that possible? I don't understand why DEAD_WEAK has 4 payload words, but StgDeadWeak has only one, and furthermore a DEAD_WEAK appears to have the link field in a different place from the WEAK closure. Something is suspicious here. Cheers, Simon
Thank you for your comments.
On Mon, May 6, 2013 at 4:32 AM, Simon Marlow
Akio, I looked at your first patch, which mostly seems good. The sequence at the beginning of deRefWeak certainly looks strange - I think it's ok, but it needs a comment.
I update my patches and added a comment to that part.
One thing I'm confused about is the handling of DEAD_WEAK pointers. You removed the StgDeadWeak struct - how is that possible? I don't understand why DEAD_WEAK has 4 payload words, but StgDeadWeak has only one, and furthermore a DEAD_WEAK appears to have the link field in a different place from the WEAK closure. Something is suspicious here.
Before my patch, dead weak pointers had the same size as live ones, but they had the link field in different places. With my patch, dead weak pointers have the same layout as live ones, so they can be accessed with the single StgWeak struct. The only differences between live and dead weak pointers now are the info pointer, and whether the "cfinalizers" field is followed by the GC. The code is simplified a bit because it doesn't need to deal with two different layouts. I think I needed this change in order to keep finalizeWeak# atomic, but now that I use lockClosure() to ensure atomicity, probably I can remove this change from the patch. Should I do so? - Akio
Cheers, Simon
participants (2)
-
Akio Takano -
Simon Marlow