Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1881933005:
Oilpan: Add ThreadState cleanup callbacks for static thread-specific persistent. (Closed)
|
Created:
4 years, 8 months ago by jbroman Modified:
4 years, 8 months ago CC:
Mads Ager (chromium), blink-reviews, chromium-reviews, kinuko+watch, kouhei+heap_chromium.org, oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOilpan: Add ThreadState cleanup callbacks for static thread-specific persistent.
This allows clients to register a cleanup hook in ThreadState before the final
collection occurs, so that, for example, static thread-specific persistent handles
can be cleared.
Also provided is a helper function, with an example, which uses this facility
to provide for a static local thread-specific persistent handle.
Committed: https://crrev.com/8a89723f74422ac8bd015f5e36aca06b296bed4e
Cr-Commit-Position: refs/heads/master@{#387519}
Patch Set 1 #
Total comments: 5
Patch Set 2 : change to a method on Persistent<T>, per haraken #Patch Set 3 : unit test #
Total comments: 2
Patch Set 4 : rename per haraken #
Messages
Total messages: 25 (8 generated)
Description was changed from ========== WIP: ThreadState cleanup callbacks for static thread-specific persistent BUG= ========== to ========== Oilpan: Add ThreadState cleanup callbacks for static thread-specific persistent. This allows clients to register a cleanup hook in ThreadState before the final collection occurs, so that, for example, static thread-specific persistent handles can be cleared. Also provided is a helper function, with an example, which uses this facility to provide for a static local thread-specific persistent handle. ==========
jbroman@chromium.org changed reviewers: + haraken@chromium.org, xlai@chromium.org
PTAL. Ideally I'd like to have unit tests for this, but it seems fairly straightforward and is blocking Olivia's forward progress. The pattern for clients is not too long, so I didn't try to make the DEFINE_THREAD_SAFE_STATIC_LOCAL macro extra magic; we already have assertions that would catch f failing to do this correctly.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881933005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881933005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h (right): https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h:16: // It's important that the Persistent<T> exist until then, because this takes a exist => exists I'm okay with putting the assumption (i.e., the Persistent<T> exists until shutdown) at this point, but this is a TODO. A better fix would be: 1) Introduce ThreadSpecificnessPersistentConfiguration to Handle.h. 2) Modify PersistentBase::trace like this: void PersistentBase::trace(Visitor* visitor) { if (threadSpecificConfiguration == ThreadSpecificPersistentConfiguration && visitor->isTerminationGC()) { // Don't trace the persistent handle. } ...; } https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h:29: // } I might prefer introducing Persistent<>::clearOnThreadShutdown so that we can write the code as: fooHandle->clearOnThreadShutdown();
Thanks for working on this btw!
https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h (right): https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h:16: // It's important that the Persistent<T> exist until then, because this takes a On 2016/04/14 02:36:23, haraken wrote: > > exist => exists > > I'm okay with putting the assumption (i.e., the Persistent<T> exists until > shutdown) at this point, but this is a TODO. > > A better fix would be: > > 1) Introduce ThreadSpecificnessPersistentConfiguration to Handle.h. > > 2) Modify PersistentBase::trace like this: > > void PersistentBase::trace(Visitor* visitor) { > if (threadSpecificConfiguration == ThreadSpecificPersistentConfiguration && > visitor->isTerminationGC()) { > // Don't trace the persistent handle. > } > ...; > } Pardon me for popping a question here: The reason why https://codereview.chromium.org/1870503002/ doesn't work is that the destroy of ThreadSpecific instances happens after ThreadState::cleanup(), in which it checks and finds that there're some uncleaned Persistent handles (e.g. a static instance ThreadSpecific<Persistent<CSSValuePool>>). Jeremy's patch tries to do clean-up on this special-case Persistent handles. But what you suggested here, in my understanding, is not to trace these special-case Persistent handles at all. Would that be too inconsistent with other Persistent handles?
https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h (right): https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h:16: // It's important that the Persistent<T> exist until then, because this takes a On 2016/04/14 at 02:36:23, haraken wrote: > exist => exists I believe "exist" is correct here, due to the subjunctive mood (e.g. https://en.wikipedia.org/wiki/English_subjunctive has examples of the form "it's important that..."). > I'm okay with putting the assumption (i.e., the Persistent<T> exists until shutdown) at this point, but this is a TODO. > > A better fix would be: > > 1) Introduce ThreadSpecificnessPersistentConfiguration to Handle.h. > > 2) Modify PersistentBase::trace like this: > > void PersistentBase::trace(Visitor* visitor) { > if (threadSpecificConfiguration == ThreadSpecificPersistentConfiguration && visitor->isTerminationGC()) { > // Don't trace the persistent handle. > } > ...; > } I did consider this (and actually tried it, now that you've mentioned it). Some reasons for the approach in this patchset: 1. it's less intrusive in the PersistentNode code: a) to actually fix the assertion in ThreadState, thread-linked persistent like this have to be excluded from the count of remaining persistent handles, which means that the number of these must be known to the PersistentRegion, and b) there is another assertion in ~PersistentNode (which happens while the thread is shutting down, but before the system destroys thread-locals) that fails if any slot is still used at that time 2) it doesn't allow the dangling Persistent to exist between the GC and thread termination https://codereview.chromium.org/1881933005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapThreadSpecific.h:29: // } On 2016/04/14 at 02:36:23, haraken wrote: > I might prefer introducing Persistent<>::clearOnThreadShutdown so that we can write the code as: > > fooHandle->clearOnThreadShutdown(); OK, done.
...and I have now also added a unit test.
LGTM > I did consider this (and actually tried it, now that you've mentioned it). > > Some reasons for the approach in this patchset: > 1. it's less intrusive in the PersistentNode code: > a) to actually fix the assertion in ThreadState, thread-linked persistent like > this have to be excluded from the count of remaining persistent handles, which > means that the number of these must be known to the PersistentRegion, and > b) there is another assertion in ~PersistentNode (which happens while the > thread is shutting down, but before the system destroys thread-locals) that > fails if any slot is still used at that time > 2) it doesn't allow the dangling Persistent to exist between the GC and thread > termination Yeah, my concern is that if someone uses the clearOnThreadShutdown for a persistent handle that can die before shutdown, it will produce a dangling pointer. That said, I agree with you that it's not trivial to implement the suggested approach. We need to 1) tweak the trace method so that it doesn't trace thread-specific persistent handles in a termination GC and 2) introduce PersistentNodeRegion::nonThreadSpecificPersistentCounts() by making each PersistentNode somehow understand if the PersistentNode is a thread-specific one or not. https://codereview.chromium.org/1881933005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1881933005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.h:512: void registerCleanupHook(PassOwnPtr<SameThreadClosure>); registerCleanupHook => registerThreadShutdownHooks https://codereview.chromium.org/1881933005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.h:669: Vector<OwnPtr<SameThreadClosure>> m_cleanupHooks; m_cleanupHooks => m_threadShutdownHooks
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1881933005/#ps60001 (title: "rename per haraken")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881933005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881933005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Oilpan: Add ThreadState cleanup callbacks for static thread-specific persistent. This allows clients to register a cleanup hook in ThreadState before the final collection occurs, so that, for example, static thread-specific persistent handles can be cleared. Also provided is a helper function, with an example, which uses this facility to provide for a static local thread-specific persistent handle. ========== to ========== Oilpan: Add ThreadState cleanup callbacks for static thread-specific persistent. This allows clients to register a cleanup hook in ThreadState before the final collection occurs, so that, for example, static thread-specific persistent handles can be cleared. Also provided is a helper function, with an example, which uses this facility to provide for a static local thread-specific persistent handle. Committed: https://crrev.com/8a89723f74422ac8bd015f5e36aca06b296bed4e Cr-Commit-Position: refs/heads/master@{#387519} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8a89723f74422ac8bd015f5e36aca06b296bed4e Cr-Commit-Position: refs/heads/master@{#387519}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
It looks like registerStaticPersistentNode() could handle this use case?
Message was sent while issue was closed.
On 2016/04/16 at 05:34:39, sigbjornf wrote: > It looks like registerStaticPersistentNode() could handle this use case? Olivia and I looked at that code when trying to solve this, but it's not quite the same thing. registerStaticPersistentNode (which, incidentally, is only enabled only for leak sanitizer builds) deals with static references that truly never go away (and so it's an error to have a static persistent pointing into a heap whose thread is terminating). This handles something importantly different: these persistents are head in thread-local storage (and only the key is held in a static). While static persistents shouldn't exist when a thread is terminated (because, being static, the handle will outlive the thread), thread-specific persistents do, because the handle will be torn down by pthreads (or equivalent on other platforms) when the OS thread exits.
Message was sent while issue was closed.
On 2016/04/16 13:58:28, jbroman wrote: > On 2016/04/16 at 05:34:39, sigbjornf wrote: > > It looks like registerStaticPersistentNode() could handle this use case? > > Olivia and I looked at that code when trying to solve this, but it's not quite > the same thing. > > registerStaticPersistentNode (which, incidentally, is only enabled only for leak > sanitizer builds) deals with static references that truly never go away (and so > it's an error to have a static persistent pointing into a heap whose thread is > terminating). This handles something importantly different: these persistents > are head in thread-local storage (and only the key is held in a static). > It's the same facility, just that we don't currently have thread-specific singletons (we used to, for cssTextCache) created via DEFINE_STATIC_LOCAL(), so there's no need for ThreadState to support this outside of the main thread. No reason why that mechanism couldn't be exposed outside of LSan, but only have DEFINE_STATIC_LOCAL() use it with LSan enabled. > While static persistents shouldn't exist when a thread is terminated (because, > being static, the handle will outlive the thread), thread-specific persistents > do, because the handle will be torn down by pthreads (or equivalent on other > platforms) when the OS thread exits. I don't see the difference in behaviour, some persistents needing to remain alive until the ThreadState is detached. But have to be cleared out then.
Message was sent while issue was closed.
On 2016/04/16 at 14:12:16, sigbjornf wrote: > On 2016/04/16 13:58:28, jbroman wrote: > > On 2016/04/16 at 05:34:39, sigbjornf wrote: > > > It looks like registerStaticPersistentNode() could handle this use case? > > > > Olivia and I looked at that code when trying to solve this, but it's not quite > > the same thing. > > > > registerStaticPersistentNode (which, incidentally, is only enabled only for leak > > sanitizer builds) deals with static references that truly never go away (and so > > it's an error to have a static persistent pointing into a heap whose thread is > > terminating). This handles something importantly different: these persistents > > are head in thread-local storage (and only the key is held in a static). > > > > It's the same facility, just that we don't currently have thread-specific singletons (we used to, for cssTextCache) created via DEFINE_STATIC_LOCAL(), so there's no need for ThreadState to support this outside of the main thread. No reason why that mechanism couldn't be exposed outside of LSan, but only have DEFINE_STATIC_LOCAL() use it with LSan enabled. I wasn't aware of cssTextCache having been similar in the past. It's not clear to me why in that case, you only needed to deal with this issue in the ENABLE(OILPAN) && defined(LEAK_SANITIZER) case, as the assertions that no persistent handles remain fires even if !defined(LEAK_SANITIZER). I suspect evolution of surrounding code. > > While static persistents shouldn't exist when a thread is terminated (because, > > being static, the handle will outlive the thread), thread-specific persistents > > do, because the handle will be torn down by pthreads (or equivalent on other > > platforms) when the OS thread exits. > > I don't see the difference in behaviour, some persistents needing to remain alive until the ThreadState is detached. But have to be cleared out then. When I checked, it seemed the existing code did not deal correctly with the handles being destroyed after that, in the TLS destruction callbacks. (I think it double-frees the PersistentNode; am I missing something that deals with that?) Dealing with that seemed like it would take extra plumbing, much as haraken's other suggestion would. It's possible it's not as dissimilar as it seemed to me. I don't object to some other factoring of this if you or the rest of the Oilpan team prefer; this CL seemed the most straightforward and unobtrusive way to solve the immediate problem.
Message was sent while issue was closed.
On 2016/04/16 22:40:20, jbroman wrote: > On 2016/04/16 at 14:12:16, sigbjornf wrote: > > On 2016/04/16 13:58:28, jbroman wrote: > > > On 2016/04/16 at 05:34:39, sigbjornf wrote: > > > > It looks like registerStaticPersistentNode() could handle this use case? > > > > > > Olivia and I looked at that code when trying to solve this, but it's not > quite > > > the same thing. > > > > > > registerStaticPersistentNode (which, incidentally, is only enabled only for > leak > > > sanitizer builds) deals with static references that truly never go away (and > so > > > it's an error to have a static persistent pointing into a heap whose thread > is > > > terminating). This handles something importantly different: these > persistents > > > are head in thread-local storage (and only the key is held in a static). > > > > > > > It's the same facility, just that we don't currently have thread-specific > singletons (we used to, for cssTextCache) created via DEFINE_STATIC_LOCAL(), so > there's no need for ThreadState to support this outside of the main thread. No > reason why that mechanism couldn't be exposed outside of LSan, but only have > DEFINE_STATIC_LOCAL() use it with LSan enabled. > > I wasn't aware of cssTextCache having been similar in the past. It's not clear > to me why in that case, you only needed to deal with this issue in the > ENABLE(OILPAN) && defined(LEAK_SANITIZER) case, as the assertions that no > persistent handles remain fires even if !defined(LEAK_SANITIZER). I suspect > evolution of surrounding code. > > > > While static persistents shouldn't exist when a thread is terminated > (because, > > > being static, the handle will outlive the thread), thread-specific > persistents > > > do, because the handle will be torn down by pthreads (or equivalent on other > > > platforms) when the OS thread exits. > > > > I don't see the difference in behaviour, some persistents needing to remain > alive until the ThreadState is detached. But have to be cleared out then. > > When I checked, it seemed the existing code did not deal correctly with the > handles being destroyed after that, in the TLS destruction callbacks. (I think > it double-frees the PersistentNode; am I missing something that deals with > that?) Dealing with that seemed like it would take extra plumbing, much as > haraken's other suggestion would. > > It's possible it's not as dissimilar as it seemed to me. I don't object to some > other factoring of this if you or the rest of the Oilpan team prefer; this CL > seemed the most straightforward and unobtrusive way to solve the immediate > problem. I think Sigbjorn's point makes sense, if I'm not missing something. I guess we can just replace registerStaticPersistentNode with Persistent::clearOnThreadShutdown() (probably regardless of whether LSAN is enabled or disabled, for simplicity). What matters for the leak detector is that the underlying PersistentNodes (and objects that are reachable from the Persistent Nodes) are released, rather than that the Persistent handles are released (if this becomes problematic, we can just mark all Persistent handles as leaky objects so that LSAN can understand it). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
