Avoid any possibility of an Alloc during TLS thread teardown

Avoid any possibility of an Alloc during TLS thread teardown

GIven that TCMalloc might get its thread teardown
notifgication ahead of the profiler, we need to be
careful to not do an alloc in the teardown handler.
If we did, we might bring TCMalloc back to life on
this thread... and it might not get a second teardown
notification.

r=rtenneti
BUG=104696
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110706

[ramant (doing other things), 2011-11-17 22:59:36]

lgtm

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:205: unregistered_thread_data_pool_->pop();
nit: Should we consider adding a DCHECK for worker_thread_data? Can it be NULL?

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:214: worker_thread_data = new ThreadData(thread_number);
nit: can thread_number be zero? If not should we consider adding a DCHECK?
thanks

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:237: unregistered_thread_data_pool_->push(this);
nit: Can we add a DCHECK for unregistered_thread_data_pool_?

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:587: if (empty_slot_ < stack_.size()) {
nit: is there a value to check if thread_data->worker_thread_number_ is not
greater than stack_.size()? thanks.

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.h#newco...
base/tracked_objects.h:528: // Implment a stack that avoids allocations during a
push() by having enough
nit: Implment  -> Implement

[jar (doing other things), 2011-11-18 01:06:42]

Responses to comments by rtenneti.

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:205: unregistered_thread_data_pool_->pop();
Line 205 would access lower memory if the instance were null... so we
effectively have a CHECK() call (at no extra charge).

On 2011/11/17 22:59:36, ramant wrote:
> nit: Should we consider adding a DCHECK for worker_thread_data? Can it be
NULL?

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:214: worker_thread_data = new ThreadData(thread_number);
On 2011/11/17 22:59:36, ramant wrote:
> nit: can thread_number be zero? If not should we consider adding a DCHECK?
> thanks

Line 215 checks that via the DCHECK().  We're loading thread number in... and it
verifies that it was indeed non-zero.

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:237: unregistered_thread_data_pool_->push(this);
If unregistered_thread_data_pool_ is null, then we'll access memory near zero,
and segv anyway... so the DCHECK seems redundant (it is almost like we have a
CHECK on this line ;-) )

On 2011/11/17 22:59:36, ramant wrote:
> nit: Can we add a DCHECK for unregistered_thread_data_pool_?

http://codereview.chromium.org/8587031/diff/4003/base/tracked_objects.cc#newc...
base/tracked_objects.cc:587: if (empty_slot_ < stack_.size()) {
On 2011/11/17 22:59:36, ramant wrote:
> nit: is there a value to check if thread_data->worker_thread_number_ is not
> greater than stack_.size()? thanks.

If that violation takes place, then I'll fall through to line 592 (and DCHECK). 
IF this happens in release mode, then I'd "gracefully" discard the argument,
rather than pushing it, and I'd leak.  I was trying to be extra careful to even
check for that case...  but most stack implementations wouldn't have bothered to
do so.  In addition, I don't think that *all* of the worker threads usually get
shut down, so there is almost always some extra space in the stack... and we
should be far away from it.

Is that convincing??

[ramant (doing other things), 2011-11-18 01:26:41]

sounds good to me. thanks.