Refactor thread_local.h's TLS Implementation to use ThreadLocalStorage

Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824
Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Committed: https://crrev.com/a681bfe835c65798b50602354056af05a1fdbc51
Cr-Original-Commit-Position: refs/heads/master@{#377774}
Cr-Commit-Position: refs/heads/master@{#424162}

[robliao, 2016-02-23 23:24:03]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-23 23:31:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726203002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726203002/1

[robliao, 2016-02-23 23:56:22]

Description was changed from

==========
Refactor thread_local.h's TLS Usage to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824
==========

to

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824
==========

[commit-bot: I haz the power, 2016-02-24 01:38:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-24 01:38:53]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-02-24 01:41:55]

robliao@chromium.org changed reviewers:
+ brettw@chromium.org

[robliao, 2016-02-24 01:41:55]

brettw: Please review this change. Thanks!

[robliao, 2016-02-25 18:14:53]

On 2016/02/24 01:41:55, robliao wrote:
> brettw: Please review this change. Thanks!

Ping for brettw! Thanks

[brettw, 2016-02-25 22:57:45]

LGTM, sorry for the delay!

https://codereview.chromium.org/1726203002/diff/1/base/threading/thread_local.h
File base/threading/thread_local.h (right):

https://codereview.chromium.org/1726203002/diff/1/base/threading/thread_local...
base/threading/thread_local.h:9: // These classes implement a wrapper around
ThreadLocalStorage::Slot. On
At the top of thread_local_storage.h it says you should use a
ThreadLocalStorage::Slot, I think we might want to add this class to the list of
recommended stuff in that file.

[robliao, 2016-02-25 23:57:11]

Patchset #2 (id:20001) has been deleted

[robliao, 2016-02-26 00:00:41]

Patchset #2 (id:40001) has been deleted

[robliao, 2016-02-26 00:02:51]

Thanks for the review!

https://codereview.chromium.org/1726203002/diff/1/base/threading/thread_local.h
File base/threading/thread_local.h (right):

https://codereview.chromium.org/1726203002/diff/1/base/threading/thread_local...
base/threading/thread_local.h:9: // These classes implement a wrapper around
ThreadLocalStorage::Slot. On
On 2016/02/25 22:57:45, brettw wrote:
> At the top of thread_local_storage.h it says you should use a
> ThreadLocalStorage::Slot, I think we might want to add this class to the list
of
> recommended stuff in that file.

Done. Updated to...

// WARNING: You should *NOT* use this class directly.
// PlatformThreadLocalStorage is a low-level abstraction of the OS's TLS
// interface. Instead, you should use one of the following:
// * ThreadLocalBoolean (from thread_local.h) for booleans.
// * ThreadLocalPointer (from thread_local.h) for pointers.
// * ThreadLocalStorage::StaticSlot/Slot for more direct control of the slot.

[robliao, 2016-02-26 00:05:28]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-02-26 00:05:28]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1726203002/#ps60001
(title: "Update thread_local_storage.h comment")

[commit-bot: I haz the power, 2016-02-26 00:13:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726203002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726203002/60001

[commit-bot: I haz the power, 2016-02-26 02:17:13]

Description was changed from

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824
==========

to

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824
==========

[commit-bot: I haz the power, 2016-02-26 02:17:13]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-02-26 02:18:30]

Description was changed from

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824
==========

to

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}
==========

[commit-bot: I haz the power, 2016-02-26 02:18:31]

Patchset 2 (id:??) landed as
https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}

[robliao, 2016-02-26 18:23:16]

A revert of this CL (patchset #2 id:60001) has been created in
https://codereview.chromium.org/1743693002/ by robliao@chromium.org.

The reason for reverting is: Reverting to consider this leak in base_unittests
and unit_tests

Leak_DefinitelyLost
8,192 bytes in 4 blocks are definitely lost in loss record 40,719 of 40,770
  operator new[](unsigned long) (m_replacemalloc/vg_replace_malloc.c:1144)
  (anonymous namespace)::ConstructTlsVector()
(base/threading/thread_local_storage.cc:107)
  base::ThreadLocalStorage::StaticSlot::Set(void*)
(base/threading/thread_local_storage.cc:246)
  base::ThreadLocalPointer<void>::Set(void*) (base/threading/thread_local.h:69)
50 new files were left in /tmp: Fix the tests to clean up themselves.
  base::ThreadLocalBoolean::Set(bool) (base/threading/thread_local.h:88)
  base::(anonymous namespace)::WorkerThread::ThreadMain()
(base/threading/worker_pool_posix.cc:79)
  base::(anonymous namespace)::ThreadFunc(void*)
(base/threading/platform_thread_posix.cc:68)
Suppression (error hash=#F6EC67ECF9CF9B28#):
  For more info on using suppressions see
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:_Zna*
   fun:_ZN12_GLOBAL__N_118ConstructTlsVectorEv
   fun:_ZN4base18ThreadLocalStorage10StaticSlot3SetEPv
   fun:_ZN4base18ThreadLocalPointerIvE3SetEPv
   fun:_ZN4base18ThreadLocalBoolean3SetEb
   fun:_ZN4base12_GLOBAL__N_112WorkerThread10ThreadMainEv
   fun:_ZN4base12_GLOBAL__N_110ThreadFuncEPv
}.

[robliao, 2016-02-26 19:06:57]

Description was changed from

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}
==========

to

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}
==========

[robliao, 2016-02-29 22:46:42]

On 2016/02/26 18:23:16, robliao wrote:
> A revert of this CL (patchset #2 id:60001) has been created in
> https://codereview.chromium.org/1743693002/ by mailto:robliao@chromium.org.
> 
> The reason for reverting is: Reverting to consider this leak in base_unittests
> and unit_tests
> 
> Leak_DefinitelyLost
> 8,192 bytes in 4 blocks are definitely lost in loss record 40,719 of 40,770
>   operator new[](unsigned long) (m_replacemalloc/vg_replace_malloc.c:1144)
>   (anonymous namespace)::ConstructTlsVector()
> (base/threading/thread_local_storage.cc:107)
>   base::ThreadLocalStorage::StaticSlot::Set(void*)
> (base/threading/thread_local_storage.cc:246)
>   base::ThreadLocalPointer<void>::Set(void*)
(base/threading/thread_local.h:69)
> 50 new files were left in /tmp: Fix the tests to clean up themselves.
>   base::ThreadLocalBoolean::Set(bool) (base/threading/thread_local.h:88)
>   base::(anonymous namespace)::WorkerThread::ThreadMain()
> (base/threading/worker_pool_posix.cc:79)
>   base::(anonymous namespace)::ThreadFunc(void*)
> (base/threading/platform_thread_posix.cc:68)
> Suppression (error hash=#F6EC67ECF9CF9B28#):
>   For more info on using suppressions see
>
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
> {
>    <insert_a_suppression_name_here>
>    Memcheck:Leak
>    fun:_Zna*
>    fun:_ZN12_GLOBAL__N_118ConstructTlsVectorEv
>    fun:_ZN4base18ThreadLocalStorage10StaticSlot3SetEPv
>    fun:_ZN4base18ThreadLocalPointerIvE3SetEPv
>    fun:_ZN4base18ThreadLocalBoolean3SetEb
>    fun:_ZN4base12_GLOBAL__N_112WorkerThread10ThreadMainEv
>    fun:_ZN4base12_GLOBAL__N_110ThreadFuncEPv
> }.

An update on the investigation: It appears death tests might cause thread
termination problems (omitting sanity checks): 
tools/valgrind/chrome_tests.sh --tool memcheck --build-dir
/chromium_asan/src/out/Release -t base_unittests
--gtest_filter=AllocationRegister*Overflow*:Wo*
Reliably reproduces the leak:
[==========] Running 2 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from WorkerPoolTest
[ RUN      ] WorkerPoolTest.PostTask
[       OK ] WorkerPoolTest.PostTask (187 ms)
[----------] 1 test from WorkerPoolTest (214 ms total)

[----------] 1 test from AllocationRegisterTest
[ RUN      ] AllocationRegisterTest.OverflowDeathTest

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:834:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test detected 3 threads.
[       OK ] AllocationRegisterTest.OverflowDeathTest (230 ms)
[----------] 1 test from AllocationRegisterTest (230 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 2 test cases ran. (485 ms total)
[  PASSED  ] 2 tests.

Leak_DefinitelyLost
2,048 bytes in 1 blocks are definitely lost in loss record 9,742 of 9,752
  operator new[](unsigned long) (m_replacemalloc/vg_replace_malloc.c:1144)
  (anonymous namespace)::ConstructTlsVector()
(base/threading/thread_local_storage.cc:107)
  base::ThreadLocalStorage::StaticSlot::Set(void*)
(base/threading/thread_local_storage.cc:246)
  base::ThreadLocalPointer<void>::Set(void*) (base/threading/thread_local.h:69)
  base::ThreadLocalBoolean::Set(bool) (base/threading/thread_local.h:88)
  base::(anonymous namespace)::WorkerThread::ThreadMain()
(base/threading/worker_pool_posix.cc:79)
  base::(anonymous namespace)::ThreadFunc(void*)
(base/threading/platform_thread_posix.cc:68)
Suppression (error hash=#F6EC67ECF9CF9B28#):
  For more info on using suppressions see
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:_Zna*
   fun:_ZN12_GLOBAL__N_118ConstructTlsVectorEv
   fun:_ZN4base18ThreadLocalStorage10StaticSlot3SetEPv
   fun:_ZN4base18ThreadLocalPointerIvE3SetEPv
   fun:_ZN4base18ThreadLocalBoolean3SetEb
   fun:_ZN4base12_GLOBAL__N_112WorkerThread10ThreadMainEv
   fun:_ZN4base12_GLOBAL__N_110ThreadFuncEPv
}

[brettw, 2016-08-05 18:17:56]

Should we be trying to reland this?

[robliao, 2016-08-05 18:21:21]

On 2016/08/05 18:17:56, brettw (ping on IM after 24h) wrote:
> Should we be trying to reland this?

Yep. It's been on the back burner, but I've got some changes that implement the
free functionality. I need to validate that I've taken care of the race
conditions or just use locks (which I'm tempted to just do).

[robliao, 2016-09-28 23:47:42]

Patchset #3 (id:80001) has been deleted

[robliao, 2016-09-28 23:47:50]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 23:48:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 23:51:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 23:51:32]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[robliao, 2016-09-28 23:53:02]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 23:53:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 23:57:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 23:57:43]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[robliao, 2016-09-30 17:30:15]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 17:30:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 18:31:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 18:31:14]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[robliao, 2016-10-07 20:29:00]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 20:29:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 22:23:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 22:23:47]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-10-07 22:34:40]

On 2016/08/05 18:21:21, robliao wrote:
> On 2016/08/05 18:17:56, brettw (ping on IM after 24h) wrote:
> > Should we be trying to reland this?
> 
> Yep. It's been on the back burner, but I've got some changes that implement
the
> free functionality. I need to validate that I've taken care of the race
> conditions or just use locks (which I'm tempted to just do).

I think we're good to reland this next Monday (10/10/2016).

[robliao, 2016-10-09 22:04:48]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 22:04:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-09 23:17:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-09 23:17:56]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-10-10 16:01:47]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-10-10 16:01:48]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1726203002/#ps100001
(title: "Rebase to d98ce9a")

[commit-bot: I haz the power, 2016-10-10 16:01:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 16:07:42]

Description was changed from

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}
==========

to

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}
==========

[commit-bot: I haz the power, 2016-10-10 16:07:43]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-10-10 16:10:24]

Description was changed from

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Cr-Commit-Position: refs/heads/master@{#377774}
==========

to

==========
Refactor thread_local.h's TLS Implemetation to use ThreadLocalStorage

thread_local_*'s implementation of TLS was redundant with
thread_local_storage_*'s implementation.

This change reduces the number of implementations by one.

BUG=588824

Committed: https://crrev.com/8f0b97ae15609cee6fe67d2b0b958ef37cffb1e0
Committed: https://crrev.com/a681bfe835c65798b50602354056af05a1fdbc51
Cr-Original-Commit-Position: refs/heads/master@{#377774}
Cr-Commit-Position: refs/heads/master@{#424162}
==========

[commit-bot: I haz the power, 2016-10-10 16:10:25]

Patchset 3 (id:??) landed as
https://crrev.com/a681bfe835c65798b50602354056af05a1fdbc51
Cr-Commit-Position: refs/heads/master@{#424162}