Add Reclaim Support to ThreadLocalStorage

Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Committed: https://crrev.com/45609b5403d73864e5119cd5084bd6f9c7f62076
Cr-Original-Commit-Position: refs/heads/master@{#421320}
Cr-Commit-Position: refs/heads/master@{#423911}

[robliao, 2016-09-22 17:28:59]

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

[commit-bot: I haz the power, 2016-09-22 17:29:49]

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

[robliao, 2016-09-22 17:47:14]

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

[commit-bot: I haz the power, 2016-09-22 18:31:27]

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

[commit-bot: I haz the power, 2016-09-22 18:31:28]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-09-23 17:35:51]

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

[commit-bot: I haz the power, 2016-09-23 17:36:34]

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

[brettw, 2016-09-23 20:29:00]

This implementation seems very reasonable.

My only concern is that it might add a bunch of extra locking during startup
when many such things are created? Do you know how many slots we allocate during
startup?

Also, it would be good if it's clear what the motivation for this change is.
This new behavior seems like a good idea in general, but do we have problems
with this in practice?

[commit-bot: I haz the power, 2016-09-23 20:34:53]

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

[commit-bot: I haz the power, 2016-09-23 20:34:54]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-09-23 20:40:46]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

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

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
==========

[robliao, 2016-09-23 20:44:25]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
==========

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization. 
Tests that involve TLS usage caused the process to run out of TLS slots 
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
==========

[robliao, 2016-09-23 20:46:12]

On 2016/09/23 20:29:00, brettw (ping on IM after 24h) wrote:
> This implementation seems very reasonable.
> 
> My only concern is that it might add a bunch of extra locking during startup
> when many such things are created? Do you know how many slots we allocate
during
> startup?
> 
> Also, it would be good if it's clear what the motivation for this change is.
> This new behavior seems like a good idea in general, but do we have problems
> with this in practice?

When I discovered Windows basically does this in its TLS libraries, I figure it
would be good enough for us. 
The lock hold time is short enough that I would be surprised if things
substantially regressed.
If contention does become an issue, I can start looking into more lock-free
techniques for this (but they appear to be fraught with peril at first
investigation).

The high water mark right now is 3 on Windows, but that will go up once I
combine the two TLS implementations together. I imagine it's higher on Android
where this is already unified.

The problem that usually manifests for TLS are in tests. I've updated the
description and BUG to account for this. Because tests don't restart the
process, the TLS usage monotonically increases with each test that utilizes the
TLS. 
http://crbug.com/590907 covers an actual case of this.

[robliao, 2016-09-27 18:56:22]

On 2016/09/23 20:46:12, robliao wrote:
> On 2016/09/23 20:29:00, brettw (ping on IM after 24h) wrote:
> > This implementation seems very reasonable.
> > 
> > My only concern is that it might add a bunch of extra locking during startup
> > when many such things are created? Do you know how many slots we allocate
> during
> > startup?
> > 
> > Also, it would be good if it's clear what the motivation for this change is.
> > This new behavior seems like a good idea in general, but do we have problems
> > with this in practice?
> 
> When I discovered Windows basically does this in its TLS libraries, I figure
it
> would be good enough for us. 
> The lock hold time is short enough that I would be surprised if things
> substantially regressed.
> If contention does become an issue, I can start looking into more lock-free
> techniques for this (but they appear to be fraught with peril at first
> investigation).
> 
> The high water mark right now is 3 on Windows, but that will go up once I
> combine the two TLS implementations together. I imagine it's higher on Android
> where this is already unified.
> 
> The problem that usually manifests for TLS are in tests. I've updated the
> description and BUG to account for this. Because tests don't restart the
> process, the TLS usage monotonically increases with each test that utilizes
the
> TLS. 
> http://crbug.com/590907 covers an actual case of this.

Any other comments?

[brettw, 2016-09-27 19:14:12]

Sorry I dropped this.

I just did a test and counted 34 TLS uses which is reasonable for locking I
think.

LGTM

[robliao, 2016-09-27 19:17:22]

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

[commit-bot: I haz the power, 2016-09-27 19:17:47]

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-27 20:20:06]

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

[commit-bot: I haz the power, 2016-09-27 20:20:08]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-09-27 20:21:10]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2016-09-27 20:21:37]

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-27 20:28:01]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization. 
Tests that involve TLS usage caused the process to run out of TLS slots 
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
==========

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization. 
Tests that involve TLS usage caused the process to run out of TLS slots 
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
==========

[commit-bot: I haz the power, 2016-09-27 20:28:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-27 20:29:36]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization. 
Tests that involve TLS usage caused the process to run out of TLS slots 
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907
==========

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}
==========

[commit-bot: I haz the power, 2016-09-27 20:29:37]

Patchset 1 (id:??) landed as
https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}

[robliao, 2016-10-06 02:08:43]

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

The reason for reverting is: Precautionary revert while waiting for
https://codereview.chromium.org/2383833004/

Will reland after branch point..

[robliao, 2016-10-06 02:09:08]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}
==========

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}
==========

[robliao, 2016-10-07 17:06:48]

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

[commit-bot: I haz the power, 2016-10-07 17:07:03]

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 18:02:39]

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

[commit-bot: I haz the power, 2016-10-07 18:02:40]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-10-07 18:02:58]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2016-10-07 18:03:20]

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 18:09:44]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}
==========

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}
==========

[commit-bot: I haz the power, 2016-10-07 18:09:45]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-07 18:12:45]

Description was changed from

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Cr-Commit-Position: refs/heads/master@{#421320}
==========

to

==========
Add Reclaim Support to ThreadLocalStorage

Previously, ThreadLocalStorage::StaticSlot::Free() did not actually
release the corresponding ThreadLocalStorage slot. It simply cleared
out the slot and did not reuse it. As a result, each process had a
finite number of calls to ThreadLocalStorage::StaticSlot::Initialize()
before running out of slots.

This problem would manifest itself in tests where a single process runs
many tests that each do their own initialization and uninitialization.
Tests that involve TLS usage caused the process to run out of TLS slots
because there was no free support in ThreadLocalStorage.

This change adds in free support by doing what most operating systems
do, lock and track metadata in an array.

BUG=590907

Committed: https://crrev.com/783d1f9630c86d816669ab23981d0f3722c96808
Committed: https://crrev.com/45609b5403d73864e5119cd5084bd6f9c7f62076
Cr-Original-Commit-Position: refs/heads/master@{#421320}
Cr-Commit-Position: refs/heads/master@{#423911}
==========

[commit-bot: I haz the power, 2016-10-07 18:12:47]

Patchset 1 (id:??) landed as
https://crrev.com/45609b5403d73864e5119cd5084bd6f9c7f62076
Cr-Commit-Position: refs/heads/master@{#423911}