Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8785013:
Track webcore worker message loops in chromium. (Closed)
|
Created:
9 years ago by dgrogan Modified:
9 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dpranke-watch+content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTrack webcore worker message loops in chromium.
BUG=106265
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114157
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114192
Patch Set 1 #
Total comments: 12
Patch Set 2 : address comments #Patch Set 3 : rebase onto ToT #Patch Set 4 : address comments after rebase #
Total comments: 1
Patch Set 5 : address 2/3 remaining issues #Patch Set 6 : reupload patchset 4 #Patch Set 7 : address 2/3 remaining issues #Patch Set 8 : lazy-init and leak worker task runner #
Total comments: 8
Patch Set 9 : rebase onto ToT #Patch Set 10 : add static accessor to WorkerTaskRunner, use custom ObserverList #
Total comments: 15
Patch Set 11 : address comments; move to webkit/glue #Patch Set 12 : remove debug code #
Total comments: 1
Patch Set 13 : only notify on stop #Patch Set 14 : remove extra include #
Total comments: 22
Patch Set 15 : reorder a couple blocks of code #Patch Set 16 : address comments #
Total comments: 8
Patch Set 17 : address style comments #Patch Set 18 : rebase onto ToT #Patch Set 19 : add WEBKIT_GLUE_EXPORT #
Messages
Total messages: 35 (0 generated)
Michael, Darin, Could you review this? I split this off from the IDB-on-workers mega patch: http://codereview.chromium.org/8747002/ so Michael has seen it once before.
http://codereview.chromium.org/8785013/diff/1/content/common/child_thread.h File content/common/child_thread.h (right): http://codereview.chromium.org/8785013/diff/1/content/common/child_thread.h#n... content/common/child_thread.h:146: scoped_refptr<WorkerTaskRunner> worker_task_runner_; Would a leaky lazy instance be a safer/better choice for the worker task runner? As coded in this CL, the refcounted'ness isn't used. Also do you know if worker threads are guaranteed to be shutdown at ChildThread dtor time? http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:17: using WebKit::WebWorkerRunLoop; please avoid 'using' in .h files http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:22: ~WorkerTaskRunner(); refcounted classes don't need public dtors http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:23: // This could be changed to take a chromium closure that is then wrapped by i think it'd be good to use a base::Bind closure here so the WebKit api datatype doesn't leak up out of this interface http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:31: virtual void onLoopRegistered() { } nits: upper case method names and no space needed in {} http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:46: void RegisterCurrentWorkerLoop(const WebWorkerRunLoop& loop); consider making the method names for start/stop notifications more similar to the webkit api layer webkit. didStartWorkerRunLoop() / didStopWorkerRunLoop() taskRunner. OnWorkerRunLoopStarted(loop) / OnWorkerRunLoopStopped(loop) observer. OnWorkerRunLoopStarted(id) / OnWorkerRunLoopStopped(id)
The diff between patchset 3 and 4 shows what I changed. patchset 2 had both new trunk code and my revisions, so it's harder to tell what I actually changed. http://codereview.chromium.org/8785013/diff/1/content/common/child_thread.h File content/common/child_thread.h (right): http://codereview.chromium.org/8785013/diff/1/content/common/child_thread.h#n... content/common/child_thread.h:146: scoped_refptr<WorkerTaskRunner> worker_task_runner_; On 2011/12/05 22:02:09, michaeln wrote: > Would a leaky lazy instance be a safer/better choice for the worker task runner? Not having to worry about the lifetime would be nice. Less code, etc. > As coded in this CL, the refcounted'ness isn't used. Also do you know if > worker threads are guaranteed to be shutdown at ChildThread dtor time? I don't think they are guaranteed to be shutdown at ChildThread dtor time. ---- After looking into this, LazyInstance seems to not add anything because IndexedDBMessageFilter will add itself as an observer right after construction, which is in the ChildThread ctor. How about leaking it by removing the ref counting and replacing scoped_refptr<WorkerTaskRunner> with WorkerTaskRunner*? http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:17: using WebKit::WebWorkerRunLoop; On 2011/12/05 22:02:09, michaeln wrote: > please avoid 'using' in .h files fixed http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:22: ~WorkerTaskRunner(); On 2011/12/05 22:02:09, michaeln wrote: > refcounted classes don't need public dtors A clang plugin told me to add one: [chromium-style] Complex class/struct needs an explicit out-of-line destructor. http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors talks about it. http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:23: // This could be changed to take a chromium closure that is then wrapped by On 2011/12/05 22:02:09, michaeln wrote: > i think it'd be good to use a base::Bind closure here so the WebKit api datatype > doesn't leak up out of this interface Agreed. I skipped that because I wanted cut anything to get this in for 17. But it looks like that ship has sailed. Changed to base::Closure. http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:31: virtual void onLoopRegistered() { } On 2011/12/05 22:02:09, michaeln wrote: > nits: upper case method names and no space needed in {} Done. http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... content/common/worker_task_runner.h:46: void RegisterCurrentWorkerLoop(const WebWorkerRunLoop& loop); On 2011/12/05 22:02:09, michaeln wrote: > consider making the method names for start/stop notifications more similar to > the webkit api layer > webkit. didStartWorkerRunLoop() / didStopWorkerRunLoop() > taskRunner. OnWorkerRunLoopStarted(loop) / OnWorkerRunLoopStopped(loop) > observer. OnWorkerRunLoopStarted(id) / OnWorkerRunLoopStopped(id) Done.
> Not having to worry about the lifetime would be nice. Less code, etc. > > After looking into this, LazyInstance seems to not add anything because > IndexedDBMessageFilter will add itself as an observer right after construction, > which is in the ChildThread ctor. How about leaking it by removing the ref > counting and replacing scoped_refptr<WorkerTaskRunner> with WorkerTaskRunner*? It annotates the code to make it clear where leaky singletons are. I'm not sure what the best option is here either, but a refcounted class where the refcounting is not actually used seems off, a raw ptr begs the question about when its deleted, as coded it's hard to know if we won't run into use-after-delete type of bugs. Makes me think Leaky/LazyInstance is a reasonable fit. > http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... > content/common/worker_task_runner.h:22: ~WorkerTaskRunner(); > On 2011/12/05 22:02:09, michaeln wrote: > > refcounted classes don't need public dtors > > A clang plugin told me to add one: > [chromium-style] Complex class/struct needs an explicit out-of-line destructor. It may need a dtor, but it doesn't need to be public. If it's refcounted, delete should not be called on it by consumers. > http://codereview.chromium.org/8785013/diff/1/content/common/worker_task_runn... > content/common/worker_task_runner.h:23: // This could be changed to take a > chromium closure that is then wrapped by > On 2011/12/05 22:02:09, michaeln wrote: > > i think it'd be good to use a base::Bind closure here so the WebKit api > datatype > > doesn't leak up out of this interface > > Agreed. I skipped that because I wanted cut anything to get this in for 17. > But it looks like that ship has sailed. Changed to base::Closure. Thnx, looks like about the same amount of code, maybe even a little less when you consider the callsites.
http://codereview.chromium.org/8785013/diff/12001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/12001/content/common/worker_task_... content/common/worker_task_runner.h:23: // This could be changed to take a chromium closure that is then wrapped by this comment is stale
Lazy-initing worker task runner seems to make the utility v8 process not start correctly... or something. Still working on it.
Michael, ptal? http://codereview.chromium.org/8785013/diff/26001/content/common/child_thread.h File content/common/child_thread.h (right): http://codereview.chromium.org/8785013/diff/26001/content/common/child_thread... content/common/child_thread.h:147: static base::LazyInstance<WorkerTaskRunner, I don't think this needs to be static, but LazyInstance doesn't construct WorkerTaskRunner in the utility process otherwise.
I just have a couple of nits. I'm not sure if ObserverList will work in this way, embedded ThreadChecker's in the weakptr infrastucture may trip this up. It might be better to roll your own here. Not sure if we need unittests for this class, but there could be some. http://codereview.chromium.org/8785013/diff/26001/content/common/child_thread.h File content/common/child_thread.h (right): http://codereview.chromium.org/8785013/diff/26001/content/common/child_thread... content/common/child_thread.h:147: static base::LazyInstance<WorkerTaskRunner, On 2011/12/07 22:47:39, dgrogan wrote: > I don't think this needs to be static... LazyInstances are pretty much always static. They're used to create lazily initialized globals that are never deleted (the leaky part). http://codereview.chromium.org/8785013/diff/26001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/26001/content/common/worker_task_... content/common/worker_task_runner.h:16: extra blank line http://codereview.chromium.org/8785013/diff/26001/content/common/worker_task_... content/common/worker_task_runner.h:37: Might be nicer to have a static instance getter in this class instead of going thru ChildThread, easier to discover where to get the instance pointer if your looking at WorkerTaskRunner.h. Runner* Runner::GetInstance() { static base::LazyInstance<Runner,...> instance = LAZY_INSTANCE_INITIALIZER; return instance.Pointer(); } http://codereview.chromium.org/8785013/diff/26001/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/8785013/diff/26001/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:652: worker_tracker->OnWorkerRunLoopStarted(runLoop); we could avoid altering and the dependency on ChildThread for this altogether... WorkerTaskRunner::GetInstance()->OnWorkerRunLoopStarted(loop)
I'm going to play around with a unit test. http://codereview.chromium.org/8785013/diff/26001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/26001/content/common/worker_task_... content/common/worker_task_runner.h:16: On 2011/12/07 23:57:38, michaeln wrote: > extra blank line Done. http://codereview.chromium.org/8785013/diff/26001/content/common/worker_task_... content/common/worker_task_runner.h:37: On 2011/12/07 23:57:38, michaeln wrote: > Might be nicer to have a static instance getter in this class instead of going > thru ChildThread, easier to discover where to get the instance pointer if your > looking at WorkerTaskRunner.h. > > Runner* Runner::GetInstance() { > static base::LazyInstance<Runner,...> instance = LAZY_INSTANCE_INITIALIZER; > return instance.Pointer(); > } Changed. http://codereview.chromium.org/8785013/diff/26001/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/8785013/diff/26001/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:652: worker_tracker->OnWorkerRunLoopStarted(runLoop); On 2011/12/07 23:57:38, michaeln wrote: > we could avoid altering and the dependency on ChildThread for this altogether... > > WorkerTaskRunner::GetInstance()->OnWorkerRunLoopStarted(loop) Changed.
Code looks good to me, but i think we should put the new class in webkit/glue instead of in the content library, and put the did[Start|Stop]WorkerRunLoop() method impls directly into webkit_glue::WebKitPlatformSupportImpl. Doing so means this stuff will get incorporated into chrome renderers and workers, as well as test_shell and DRT environments. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.cc:48: worker_task_runner_; other usages of LazyInstance assign the LAZY_INSTANCE_INITIALIZER value to it, can you do that here too unless there's reason not to. static base::LazyInstance<WorkerTaskRunner, base::LeakyLazyInstanceTraits<WorkerTaskRunner> > instance = LAZY_INSTANCE_INITIALIZER; http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.h:36: // Returns number of observers removed. why the return value? http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:649: DCHECK(ChildThread::current()); does the dcheck still apply?
http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.cc:76: (*iter)->OnWorkerRunLoopStarted(); It is usually bad design to hold locks while calling out to foreign code. It can lead to dead-locks. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.h:14: #include <map> nit: these are supposed to go above the other includes: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:650: WorkerTaskRunner* worker_tracker = WorkerTaskRunner::Instance(); why is the variable called worker_tracker when the class name is WorkerTaskRunner?
http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.cc:76: (*iter)->OnWorkerRunLoopStarted(); On 2011/12/08 22:41:21, darin wrote: > It is usually bad design to hold locks while calling out to foreign code. It > can lead to dead-locks. This is generally true, but also not much an issue for the first consumer of this class and i don't expect it to be for any other consumers either. Observers can reasonably be expected to be added/removed on either the 'main' or 'io' threads and never added/removed on the 'worker' threads. Maybe we could make an assertion in Add/Remove observer to codify this... DCHECK(!CurrentWorkerId()); ... which eliminates the possibility of mutex recursion. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.h:29: virtual void OnWorkerRunLoopStopped(); pure virtual?
There's a small unittest included now. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.cc:48: worker_task_runner_; On 2011/12/08 03:10:47, michaeln wrote: > other usages of LazyInstance assign the LAZY_INSTANCE_INITIALIZER value to it, > can you do that here too unless there's reason not to. > > static base::LazyInstance<WorkerTaskRunner, > base::LeakyLazyInstanceTraits<WorkerTaskRunner> > > instance = LAZY_INSTANCE_INITIALIZER; > You're right. Changed. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.cc:76: (*iter)->OnWorkerRunLoopStarted(); On 2011/12/09 00:43:32, michaeln wrote: > On 2011/12/08 22:41:21, darin wrote: > > It is usually bad design to hold locks while calling out to foreign code. It > > can lead to dead-locks. > > This is generally true, but also not much an issue for the first consumer of > this class and i don't expect it to be for any other consumers either. > > Observers can reasonably be expected to be added/removed on either the 'main' or > 'io' threads and never added/removed on the 'worker' threads. Maybe we could > make an assertion in Add/Remove observer to codify this... > DCHECK(!CurrentWorkerId()); > ... which eliminates the possibility of mutex recursion. Darin and I talked about this today, mostly in the context of the larger problem of chromium not waiting around for the webcore worker threads to die. I added a snapshot here to avoid the narrow problem of mutex recursion. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... File content/common/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.h:14: #include <map> On 2011/12/08 22:41:21, darin wrote: > nit: these are supposed to go above the other includes: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... Done. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.h:29: virtual void OnWorkerRunLoopStopped(); On 2011/12/09 00:43:32, michaeln wrote: > pure virtual? I gave them a default empty implementation. http://codereview.chromium.org/8785013/diff/34001/content/common/worker_task_... content/common/worker_task_runner.h:36: // Returns number of observers removed. On 2011/12/08 03:10:47, michaeln wrote: > why the return value? Mostly because it's available and could feasibly be useful to some callers. I clarified the comment. http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:649: DCHECK(ChildThread::current()); On 2011/12/08 03:10:47, michaeln wrote: > does the dcheck still apply? Nope, deleted. http://codereview.chromium.org/8785013/diff/34001/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:650: WorkerTaskRunner* worker_tracker = WorkerTaskRunner::Instance(); On 2011/12/08 22:41:21, darin wrote: > why is the variable called worker_tracker when the class name is > WorkerTaskRunner? In an old revision the class was called WorkerTracker. Fixed.
I forgot to mention another possible issue. Normally, we use base/observer_list.h whenever we want to implement a list of observers. This helper solves the problem of mutations to the list happening during the loop that notifies each observer. The approach you have taken of copying the observer list avoids one type of bug, but what if an observer is removed before you notify it? You could end up notifying an observer that has already been removed, which could cause a crash, right? One way to avoid this problem might be to require that the observer list not be modified during observer callbacks.
http://codereview.chromium.org/8785013/diff/39002/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/39002/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:90: ObserverList observers_snapshot(observer_list_); This still doesn't work. As coded it's more amenable to 'observers' with thread affinity removing themselves in their stopped notification (but will break if other observers that haven't been notified yet get removed). Also this creates a larger problem for our intended use where long lived observers w/o thread affinity (like IDBMessageFilter) are setup early on at init time and only removed near the end of time. Let's rethink this? I don't think we actually need the starting notification for our use case, we do need the stopping notification and that does need to be called on the 'worker' thread itself (so idb objects with thread affinity can get cleaned up on the right thread). What if instead we had this API... WorkerTaskRunner { class StopObserver { OnStopping(); }; // Add/Remove an observer that will get notified when // the current worker run loop is stopping. This observer // will not get notified when other threads are stopping. // It's only valid to call these on a worker thread. // DCHECK(CurrentWorkerId()); void AddStopObserverForCurrentWorker(obs); void RemoveStopObserverForCurrentWorker(obs); }; When the IDBDispatcher is first created for worker thread, it can be added as a stopping observer and can delete itself upon receiving that notification. An instance of base::ObserverList would work for that per-thread collection of StoppingObservers. No locking required. Instead of putting a ptr to std::pair<int, WebWorkerRunLoop>, we could put a ptr to an object with three items... namespace { class PerThreadState { int id; WebWorkerRunLoop loop; base::ObserverList stop_observers; }; } wdyt?
Hmmm... go figure... there's are two separate TLS wrappers in the base library, thread_local.h and thread_local_slot.h... ThreadLocalStorage::Slot provides destructibility of values on thread exit, whereas as ThreadLocalPointer does not. That *might* be an option for us in place of an Observer'ish interface, but I seem to recall that pthread dtors are not guaranteed to be invoked in the associated thread (they may be invoked on the joining thread). I think we want rock solid guarantees that we're executing on the worker thread so that and v8 objects that are reachable in the graph of IDB objects to be cleaned up are poked at on the correct isolate thread. Maybe that's why we have two different wrappers? So i think i like the per-thread StopObserver approach outlined earlier because it evades any kind of platform specific problems like that.
On Fri, Dec 9, 2011 at 2:40 PM, <michaeln@chromium.org> wrote: > Hmmm... go figure... there's are two separate TLS wrappers in the base > library, > thread_local.h and thread_local_slot.h... ThreadLocalStorage::Slot provides > destructibility of values on thread exit, whereas as ThreadLocalPointer > does > not. > > That *might* be an option for us in place of an Observer'ish interface, > but I > seem to recall that pthread dtors are not guaranteed to be invoked in the > associated thread (they may be invoked on the joining thread). I think we > want > rock solid guarantees that we're executing on the worker thread so that > and v8 > objects that are reachable in the graph of IDB objects to be cleaned up are > poked at on the correct isolate thread. Maybe that's why we have two > different > wrappers? > > It seems like this should be verified. I see ThreadLocalStorage::Slot destructors being used elsewhere in Chrome. If there is a problem with that mechanism, then we should eliminate it from the codebase instead of just avoiding it here. -Darin > So i think i like the per-thread StopObserver approach outlined earlier > because > it evades any kind of platform specific problems like that. > > > http://codereview.chromium.**org/8785013/<http://codereview.chromium.org/8785... >
On Fri, Dec 9, 2011 at 2:56 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Fri, Dec 9, 2011 at 2:40 PM, <michaeln@chromium.org> wrote: > >> Hmmm... go figure... there's are two separate TLS wrappers in the base >> library, >> thread_local.h and thread_local_slot.h... ThreadLocalStorage::Slot >> provides >> destructibility of values on thread exit, whereas as ThreadLocalPointer >> does >> not. >> >> That *might* be an option for us in place of an Observer'ish interface, >> but I >> seem to recall that pthread dtors are not guaranteed to be invoked in the >> associated thread (they may be invoked on the joining thread). I think we >> want >> rock solid guarantees that we're executing on the worker thread so that >> and v8 >> objects that are reachable in the graph of IDB objects to be cleaned up >> are >> poked at on the correct isolate thread. Maybe that's why we have two >> different >> wrappers? >> >> > It seems like this should be verified. I see ThreadLocalStorage::Slot > destructors > being used elsewhere in Chrome. If there is a problem with that mechanism, > then we should eliminate it from the codebase instead of just avoiding it > here. > > While unlikely, is it possible that those other uses don't care which thread the destructor is called on?
> While unlikely, is it possible that those other uses don't care which > thread the destructor is called on? That could be true. I'll ask jar what he knows about this since he touched the ::Slot stuff very recently. I can think of another reason to keep it at a higher level too. TLS::Slot dtor ordering issues. I can imagine that v8::Isolates make use of TLS in some way. If ~IDBDispatcher ends up calling into v8 after it's TLS stuff has been shot, thats a potential for badness. WorkerTaskRunner absolutely *needs* to stopped notification from webcore, there's no way around that at all (prior to backing WebCore::WorkerRunLoop being deleted, we have to clear out our pointers to it). Given that, and some not so easy to clarify uncertainties about relying on TLS dtors... i'm sticking with my StopObserver strategy :)
jim tells me that the dtor is guaranteed to be called on the associated thread.
And regarding dtor ordering...
do while there are Slots with non-zero values {
foreach Slot {
prior to calling the dtor for a Slot, the slot value is reset to 0
the dtor(value) is invoked
}
}
... it makes several passes over the values to handle the case where a dtor
reinstatiates another Slot's value (or its own slot).
> That could be true. I'll ask jar what he knows about this since he touched the
> ::Slot stuff very recently.
> do while there are Slots with non-zero values {
well... it doesn't really setup for an infinite loop like this, it will give up
after a few iterations
> And regarding dtor ordering...
>
> do while there are Slots with non-zero values {
> foreach Slot {
> prior to calling the dtor for a Slot, the slot value is reset to 0
> the dtor(value) is invoked
> }
> }
We still have the ordering problem with ~IDBDispatcher and v8 though, right?
I'm still in favor of the observer approach.
> We still have the ordering problem with ~IDBDispatcher and v8 though, right?
> I'm still in favor of the observer approach.
Me too. The explicit observer is more explicit and in your face and comes in
advance of the ScriptExecutionContext being deleted (TLS dtor will never do
that). Much more orderly of a shutdown. Please update the CL accordingly.
#if PLATFORM(CHROMIUM)
PlatformSupport::didStopWorkerRunLoop(&m_runLoop);
#endif
.... couple lines of code ...
ASSERT(m_workerContext->hasOneRef());
// The below assignment will destroy the context, which will in turn notify
messaging proxy.
// We cannot let any objects survive past thread exit, because no other
thread will run GC or otherwise destroy them.
m_workerContext = 0;
Michael, could you take a look at the latest revision? It notifies only when a worker runloop stops.
http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:96: delete current_tls_.Get(); Oops, this has to go below the CurrentWorkerId call below.
http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:33: WebKit::WebWorkerRunLoop run_loop_; since your using this on line 12, no need for WebKit:: here http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:34: ObserverList<webkit_glue::WorkerTaskRunner::Observer> stop_observers_; no need for webkit_glue:: since we're in that namespace, and also since this is an inner struct, no need for WorkerTaskRunner:: either (i think) http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:39: DCHECK(!id_sequence_.GetNext()); DCHECKs dont exist in release builds http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:60: worker_task_runner_ = LAZY_INSTANCE_INITIALIZER; this wrapping looks odd to me, can't distinguish between the type and the variable name? http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:61: return worker_task_runner_.Pointer(); not a data member, so no need for the underbar, frequently these sort of globals have a g_ prefix http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:79: DCHECK(!current_tls_.Get()); just the 2nd DCHECK is probably sufficient http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:81: ThreadLocalState* tls = new ThreadLocalState; a ctor might be nice... current_tls_.Set(new State(id, loop)); http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:92: DCHECK(current_tls_.Get()); ditto http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:100: DCHECK(loop_map_[CurrentWorkerId()] == loop); is the value of CurrentWorkerId() always 0 at this point? http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.h:5: #ifndef CONTENT_COMMON_WORKER_TASK_RUNNER_H_ WEBKIT_GLUE_...
http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:33: WebKit::WebWorkerRunLoop run_loop_; On 2011/12/10 01:19:32, michaeln wrote: > since your using this on line 12, no need for WebKit:: here Done. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:34: ObserverList<webkit_glue::WorkerTaskRunner::Observer> stop_observers_; On 2011/12/10 01:19:32, michaeln wrote: > no need for webkit_glue:: since we're in that namespace, and also since this is > an inner struct, no need for WorkerTaskRunner:: either (i think) Done. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:39: DCHECK(!id_sequence_.GetNext()); On 2011/12/10 01:19:32, michaeln wrote: > DCHECKs dont exist in release builds Right. Fixed. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:60: worker_task_runner_ = LAZY_INSTANCE_INITIALIZER; On 2011/12/10 01:19:32, michaeln wrote: > this wrapping looks odd to me, can't distinguish between the type and the > variable name? Any better? http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:61: return worker_task_runner_.Pointer(); On 2011/12/10 01:19:32, michaeln wrote: > not a data member, so no need for the underbar, frequently these sort of globals > have a g_ prefix Underscore removed. I didn't add a g_ because it's not really global. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:79: DCHECK(!current_tls_.Get()); On 2011/12/10 01:19:32, michaeln wrote: > just the 2nd DCHECK is probably sufficient Done. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:81: ThreadLocalState* tls = new ThreadLocalState; On 2011/12/10 01:19:32, michaeln wrote: > a ctor might be nice... > current_tls_.Set(new State(id, loop)); Done. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:92: DCHECK(current_tls_.Get()); On 2011/12/10 01:19:32, michaeln wrote: > ditto Done. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:100: DCHECK(loop_map_[CurrentWorkerId()] == loop); On 2011/12/10 01:19:32, michaeln wrote: > is the value of CurrentWorkerId() always 0 at this point? Yes, it was. But now that the current_tls_ lines are below, it's not. http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.h (right): http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.h:5: #ifndef CONTENT_COMMON_WORKER_TASK_RUNNER_H_ On 2011/12/10 01:19:32, michaeln wrote: > WEBKIT_GLUE_... Done.
i just see a few style nits http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/45003/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:60: worker_task_runner_ = LAZY_INSTANCE_INITIALIZER; > Any better? yes, thnx http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:18: RunClosureTask(const base::Closure& task) : task_(task) { } nit: {} instead of { } http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:32: ThreadLocalState(int id, const WebWorkerRunLoop& loop) : id_(id), nit: i think its more common to put the start of the initializer list on a new line if it doesn't fit on the first line http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:49: base::AutoLock locker_(loop_map_lock_); nit: no _ on locker http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:95: base::AutoLock locker_(loop_map_lock_); nit: no _ on locker
http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... File webkit/glue/worker_task_runner.cc (right): http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:18: RunClosureTask(const base::Closure& task) : task_(task) { } On 2011/12/10 02:08:35, michaeln wrote: > nit: {} instead of { } Done. http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:32: ThreadLocalState(int id, const WebWorkerRunLoop& loop) : id_(id), On 2011/12/10 02:08:35, michaeln wrote: > nit: i think its more common to put the start of the initializer list on a new > line if it doesn't fit on the first line Done. http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:49: base::AutoLock locker_(loop_map_lock_); On 2011/12/10 02:08:35, michaeln wrote: > nit: no _ on locker Done. http://codereview.chromium.org/8785013/diff/45004/webkit/glue/worker_task_run... webkit/glue/worker_task_runner.cc:95: base::AutoLock locker_(loop_map_lock_); On 2011/12/10 02:08:35, michaeln wrote: > nit: no _ on locker Done.
LGTM
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/8785013/54001
Change committed as 114157
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/8785013/58001
Change committed as 114192 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
