Switch thread_local to use ThreadPlatformStorage in Android

Switch thread_local to use ThreadPlatformStorage in Android

It might not be good to switch thread_local to use ThreadPlatformStorage
on all platforms, in order to pass all tests in Linux, the number of TLS
slot has be 1024, it means 4k memory will be temporarily increased in stack,
and then move to heap for every thread which use the TLS.

Android only needs 256 slot which is much smaller than Linux's usage, and
TLS slot is currently only running out on Android's tests.

So this patch is only for Android platform.

BUG=264406
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269854

[brettw, 2014-03-07 18:04:06]

Did you mean to check in the goma lock file?

Sorry if I lost context on this. CAn you remind me of the background on this
change? Why do we need another TLS implementation for Android? How does the
pthread implementation avoid the problem of having so many slots you're
describing? It seems undesirable to have two forever.

If we're going to have two, I think we should have an _android.cc implementation
file rather than ifdeffing every single function. And we should also make all
the thread_local headers clear about what they're used for (android? not
android?). I think the current state is very confusing if you just want a TLS
variable.

[michaelbai, 2014-03-07 19:50:44]

Sorry, actually, I had another patch which remove the lock file, but failed to
upload. here is the background.

The first patch for Chromium's TLS is here
(https://codereview.chromium.org/60743004/), which just make ThreadLocalStorage
to use our own TLS implementation, but most of usage of TLS are calling to
thread_local,
Then I made another patch (https://codereview.chromium.org/139633003/), which
made the thread_local's classes use Chromium's TLS in all platform as well,
after it was landed, it broke Memory FYI on Linux Tests (valgrind) because the
TLS slot is running out. I tried to increase from 256 to 512, the test still
failed. I think it might need to increase to 1024 to pass the tests because
Linux platform has 1024 TLS slots.

Then I were thinking allocate 1024 slots for all platform might not good,
because
- in this case, our implementation need to allocate 4k bytes memory in stack
first, then move the memory to heap for each thread that uses TLS.
- the system TLS is only used up in Android which only has 128 TLS.

The pthread implementation doesn't avoid having large number of slots, it has
large number of slot already, they may can reuse the released TLS slot(I don't
know), but we can't as it is hard to implement as I understand.

The point to only use chromium's TLS for Android is that other platforms already
defines a large number of TLS, why don't we use them?

I knew it is not good to have two here, I wonder additional 4K memory in each
thread is really matter? If not, I can continue landed my previous patch. or  we
can wait Linux Memory FYI to run out of TLS in the future, at that time, we will
have one here. :)

or, Is there way to define different number of TLS slot for tests in compile
stage? it seemed that only unit tests requires large number of TLS, as it is
continue running unit test without restarting.

Let me know if you need any other information.

[michaelbai, 2014-03-11 16:00:42]

ping

[michaelbai, 2014-04-28 15:45:16]

I have moved the logical to android specific file, PTAL, thanks

[michaelbai, 2014-04-29 18:19:16]

Hi Brett, Could you help to review this again? I really want to get it fixed
this time.

[michaelbai, 2014-05-02 16:09:36]

ping

[michaelbai, 2014-05-08 18:38:11]

ping again

[brettw, 2014-05-08 19:21:00]

Looking now, sorry.

[brettw, 2014-05-08 19:41:24]

lgtm

[michaelbai, 2014-05-08 20:24:08]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-08 20:29:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/189263003/60001

[michaelbai, 2014-05-09 01:48:42]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-09 01:54:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/189263003/80001

[commit-bot: I haz the power, 2014-05-09 21:29:04]

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

[commit-bot: I haz the power, 2014-05-09 21:29:04]

Failed to apply patch for base/base.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file base/base.gypi
  Hunk #1 FAILED at 137.
  Hunk #2 succeeded at 610 (offset 4 lines).
  1 out of 2 hunks FAILED -- saving rejects to file base/base.gypi.rej

Patch:       base/base.gypi
Index: base/base.gypi
diff --git a/base/base.gypi b/base/base.gypi
index
811028739515b31cebe0f1e78315fef4c570277b..2155afef07ae0f8d777a7be9d1872c903f90f61e
100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -137,7 +137,7 @@
           'debug/dump_without_crashing.cc',
           'debug/dump_without_crashing.h',
           'debug/gdi_debug_util_win.cc',
-          'debug/gdi_debug_util_win.h',        
+          'debug/gdi_debug_util_win.h',
           # This file depends on files from the 'allocator' target,
           # but this target does not depend on 'allocator' (see
           # allocator.gyp for details).
@@ -606,6 +606,7 @@
           'threading/thread_id_name_manager.cc',
           'threading/thread_id_name_manager.h',
           'threading/thread_local.h',
+          'threading/thread_local_android.cc',
           'threading/thread_local_posix.cc',
           'threading/thread_local_storage.cc',
           'threading/thread_local_storage.h',

[michaelbai, 2014-05-12 16:09:06]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-12 16:09:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/189263003/100001

[michaelbai, 2014-05-12 16:13:22]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-12 16:14:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/189263003/100001

[commit-bot: I haz the power, 2014-05-12 18:28:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-12 20:00:14]

Change committed as 269854

[gordanac, 2014-05-27 15:07:36]

Hi michaelbai,

Limitations of current chromium's TLS implementation are met while executing
gpu_unittests on Android devices.

If you take a look into chromium buildbot, maximum number of TLS slots is
reached:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/13...

Actually, the only reason this test is still passing in buildbot is that there
are three test devices that tests cases are shared among (with default
--num_retries=2).
For example, if only one test devices is used num_retries must be 6 in order for
all test cases to be executed.

So, I am wondering if there is a plan to further improve chromium's TLS
implementation? Do you plan to increase number of TLS slots, or to reuse
previously used/freed slots in the future?