My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 264406: Limit total number of system TLS slots used across Chrome
11 people starred this issue and may be notified of changes. Back to list
 
Project Member Reported by joth@chromium.org, Jul 25, 2013
The ThreadLocal APIs in base/threading/thread_local.h make it delightfully simple to use TLS in an app.
Unfortunately, posix has a limit on the number of TLS slots available to the app. This should be at least 128, but on some platforms (e.g. Android ICS) it can be as low as 60. And these could also be used be third party libraries, platform libraries, etc that are in use in the process.

Suggest placing some debug code in ThreadLocalPlatform to count number of allocations and assert it doesn't go over some limit (say, 50? 40?) and add a comment on the class promoting frugality: e.g. have one ThreadLocalPointer to a struct of bools, rather than numerous ThreadLocalBoolean
(also, ThreadLocalBoolean might be generally unsafe class to have in the toolbox, given such low limits? /shrug)

[more background in internal bug b/9997352]

Jul 25, 2013
#1 joth@chromium.org
FTR from a quick eyeball I see about 23 ThreadLocalPointer and 8 ThreadLocalBoolean in the code, so maybe limiting usage to ~40 is OK.

Jul 25, 2013
#2 joth@chromium.org
+ some base owners for thoughts on the proposal 

Cc: j...@chromium.org thakis@chromium.org willchan@chromium.org
Jul 25, 2013
#3 j...@chromium.org
As I recall, Windows doesn't support TLS much, and so I had to do a pile of work to make it work, with only one slot of "System TLS" to play with (there were more complications... but the basic expansion from one OS TLS lot to N Chrome TLS slots was done there).

Instead of restricting usage (re: don't use more than 40 TLS slots), what about shimming to our own TLS, so that we consistently have as much as we "need?"

Conceptually, all we need is one System TLS slot... which points to an allocated vector of Chrome TLS slots (for a given thread).  As long as the System supports callbacks on thread termination... we can then process our cleanup of the Chrome TLS slots, and deallocation of the Chrome TLS vector (for said thread).  

There is some care to be taken, but I think most of the hints/gotchas are commented about in the window's implementation (there are funky rules about thread destruction... which may cause other TLS slots to be registered at that time... but they too must be destroyed... re-registering destroyed slots... and you have to be careful about calling the deallocator, which uses TLS commonly (at least TCMalloc does)).

The perf cost is one additional indirection when fetching the slots, but that may not be that big a deal.
Cc: darin@chromium.org
Jul 25, 2013
#4 joth@chromium.org
Yes, implementing a private TLS allocator was also suggested by digit in
the internal bug - if chromium already has such an implementation that
could be very interesting. Especially so for webview, where the chromium
stack has to co-exist with arbitrary other application code that may also
be competing for slots.

Have to admit I'd completely missed the thread_local_storage_* classes, I
was only looking at thread_local_posix. (no _storage_ in the name)


So I'm slightly confused - thread_local_storage_win.cc goes to lengths to
only call  TlsAlloc() once, which I think is the one-slot-available code
you are referring to, but there's also thread_local_win.cc which seems to
call TlsAlloc() freely (and not concern itself with TLS destructors). So
why is it only an issue for one and not the other?
Jul 26, 2013
#5 j...@chromium.org
Ugh (re: two different TLS implementations in Windows :-(  ).  I too was surprised to see this.

Summary <opinion>: There should only be one.  We should modify thread_local_win.cc to merely forward its requests to the "more complete" thread_local_storage_win.cc.

If you are looking to build an extensible cross-platform TLS, you should look at thread_local_storage_win.cc.

IMO, the "good one" is thread_local_storage_win.cc.  It is heavily commented, and most importantly is properly tied into use by TCMalloc (as well as the profiler... but that is NBD).

You may also notice that the "other implementation" (thread_local_win.cc) is seemingly deficient in several regards, not the least of which is that there is no way to declare a "thread destructor function" for a given TLS slot (i.e., a per-slot function to call on each thread to clean up any allocations that the TLS slot may point to.). I set breakpoints (but didn't do a thorough search/review), and I believe that current users of that implementation mostly store booleans, and pointers for which cleanup is not (currently?) required.

The major uses of thread_local_win.cc seem to be related to booleans in the implementation of singleton (perhaps not per-singleton), although I saw some other small uses.

As a result, thread_local_storage_win.cc (with the more extensive functionality) can nicely be used in place of all uses of thread_local_win.cc.

IMO, all users of the current "other implementation" should be transitioned to using the more complete one.  (but that might be a separate bug... or it might be part of a cleanup and refactoring in this bug!).


p.s., Windows appears to have more than "one" slot, but the list is constrained (fixed limit), and Windows also fails to provide support for "thread cleanup." I'm pretty sure that is what drove the creation of thread_local_storage_win.cc, which in turn provides enough functionality that it can match what is provide in Linux etc.  I know I had to work very hard to make this work on new and old (XP) systems, and to work in DLL and .exe binary environments.
Jul 29, 2013
#6 joth@chromium.org
(Moving the OS label to OS-Android to reflect that's where it's going to hit first)

To update, looks like the limit will be fixed to be posix compliant in some future Android version: https://android-review.googlesource.com/#/c/62730 (which also reduces my own specific concerns re. webview)

But for targeting devices at least up to JB MR2, Chrome on Android will still need to live within the 60 limit. The fact all android tests run on ICS bots hopefully means we'll detect this case when it does happen without any additional check in the code.

So either when it does happen we can scramble and try and port over the majority of code in thread_local_storage_win.cc to be platform agnostic / work on Android, or can try and start work on this before that time hits...


Migrating application code to use thread_local_storage.h instead of thread_local.h seems somewhat orthogonal - the latter could be implemented in terms of the former I believe?

FWIW, the patch that introduced thread_local.h stated the aim was for this API to replace ThreadLocalStorage API: http://src.chromium.org/viewvc/chrome?view=revision&revision=1678
- hmmm if nothing else, this all tells me we lived happily enough with two competing APIs for the same thing for half a decade now :-/




Labels: -OS-All OS-Android
Jul 29, 2013
#7 vino...@chromium.org
(No comment was entered for this change.)
Status: Available
Jul 30, 2013
#8 rdsmith@chromium.org
(No comment was entered for this change.)
Labels: -Cr-Internals Cr-Internals-Network-SSL
Jul 30, 2013
#9 rsleevi@chromium.org
Back to internals.

Thread Local Storage != Transport Layer Security :)
Labels: -Cr-Internals-Network-SSL Cr-Internals
Nov 12, 2013
#10 michaelbai@chromium.org
I ran into this issue on a WebView unit tests running on Nexus Galaxy JB MR2 which has 64 TLS slots, see
https://code.google.com/p/chromium/issues/detail?id=313463#c3

Nexus Galaxy might be a special case and is not supported by KK, In order to find how many slots available for clank and web view, I did tests with new Nexus 7 and Nexus Galaxy running with JB MR2,  and found out the available TLS in didStopLoading(). I got below data.
I cleared the cache, loaded terms page in Clank, Browser process has 9, Render process has 12 in Nexus 7, For Nexus Galaxy,  Browser process has 7, Render process has 12

I also Ran a unit test for WebView, Nexus 7 has 4, Nexus Galaxy has 2 in didStopLoading().

Note the test just loads a very simple page, it seemed that we are near to run out of the TLS in Android, we maybe start to implement chrome’s TLS. 

Cc: yfriedman@chromium.org
Nov 13, 2013
#11 joth@chromium.org
yes I agree this is becoming a high priority. Those 7 free slots could
easily be used up in a release or two's time, and on top of that there can
be some other platform that uses more slots in the GL drivers etc.

(to put off the inevitable a little longer, we can probably audit existing
users and e.g. combine multiple ThreadLocals in one fine into a single tls
holding a struct of state, but that's very short-termism.)
Nov 14, 2013
#12 michaelbai@chromium.org
I wrote the design doc based on the comments above, Would you like take a look?

https://docs.google.com/a/google.com/document/d/1cMMZwqknuePg1J1pLWsBXc-N6QLezEIqCiQKVy61cPI/edit?usp=sharing
Status: Started
Owner: michaelbai@chromium.org
Dec 18, 2013
#17 darin@chromium.org
Note, deanm@ who no longer works on the project is the originator of thread_local*. He argued that it is a mistake to rely on destructors for TLS. Carlos was also just telling me today that you can't rely on destructors being run if you use TLS on Win32 worker threads (e.g., via RegisterWaitForSingleObject). Dean introduced thread_local* and meant to eliminate thread_local_storage*, but never got around to finishing that work.

Back then, we of course weren't in the business of providing a library for other applications to embed, so we did not worry about running out of TLS slots.
Cc: cpu@chromium.org
Dec 18, 2013
#18 darin@chromium.org
See this CL that introduced thread_local.h:
http://src.chromium.org/viewvc/chrome?view=revision&revision=1678
Dec 18, 2013
#19 michaelbai@chromium.org
I saw some code use destructor like tracked_objects.cc and dns_reloader.cc, 
Does that mean those code didn't work at all?

We could remove the destructor param like thread_local.h.

The new implementation didn't reuse the TLS slots, we could just simple increase the number of slot if we run out of it.
Dec 18, 2013
#20 darin@chromium.org
I wish I could recall the problems that led deanm@ to conclude that relying on TLS destructors was a bad idea :-(  My guess is that it works just fine in most cases.
Dec 18, 2013
#21 joth@chromium.org
other applications to embed, so we did not worry about running out of TLS
slots.

Note that regardless of WebView, we'll hit the limit in Chrome production
builds sometime: bionic only provides ~60 slots prior to kitkat, and
certain tests are already hitting that limit even without any other
application's code running in the process. (Chrome on Android needs to
retain support back to ICS)
Dec 18, 2013
#22 michaelbai@chromium.org
My next step is changing thread_local to use thread_local_storage,  this and the landed CL will not change Windows' situation, since there no API is changed, if the user replies on destructor, it still does, if it doesn't, it still doesn't. 
Dec 18, 2013
#23 darin@chromium.org
So that basically means undoing what Dean was trying to accomplish. Perhaps that is just how it should be given the constraint you are facing. Hmm.
Dec 18, 2013
#24 michaelbai@chromium.org
What Dean was trying to accomplish is making destructor reliable in Windows or completely removing the usage of destructor?  
Dec 18, 2013
#25 darin@chromium.org
He was trying to completely remove the usage of TLS destructors.
Dec 18, 2013
#26 j...@chromium.org
I put quite a bit of effort into making destructors reliable on a variety of Windows versions, and I *think* I succeeded (several years ago).

The destructors are critical to the integration of TCMalloc (which needs destructor notification to empty the thread-cache into the global cache), and are also critical to the profiler I landed.

I would be very interested in seeing any examples where this is not successful. It would certainly be devastating to Windows Chrome (currently) if it was not working, as we would leak like a sieve.  It is also pretty easy to watch for this problems via about:profiler... at least if the thread in question either posted, or ran tasks.
Dec 19, 2013
#27 darin@chromium.org
@jar, so Dean never explained to you why he thought that code was wrong? Maybe @cpu can help illuminate things. My understanding from talking to him is that Win32 worker threads (used in Chrome via base/threading/worker_pool_win.cc) get terminated in a manner that prevents TLS destructors from running. It is possible therefore that we are leaking memory in some cases if tcmalloc depends on TLS destructors running. I believe there have also been discussions about moving away from using Win32 worker threads.
Dec 19, 2013
#28 j...@chromium.org
tl;dr;  Background of making TLS destructors, including TCMalloc, work on Windows.

[Caveat: There is always a chance that this was broken over time in some way... but this is how it used to work... and there probably would have been no great reason to change the code.]

I'm pretty sure my final solution got the blessing of at least Ricardo at the time, and probably there was some discussion with Carlos.  

You can "see" that it is working by looking at about:profiler, and watching to see if the number of unique threads (which includes worker threads) grows without bounds over time (leaks contexts on worker thread termination).  You can also look at about:tcmalloc, and see if the "Thread heaps in use" counter is growing without bounds (its count, for a given process, is often a tad higher, as there are some threads that don't run or post tasks, and hence are not seen by the profiler).

Most of the underlying magic came from discussion in http://www.codeproject.com/threads/tls.asp which was cited (thankfully!!) in the TCMalloc distribution.

The hard(er) case (as I recall) was compatibility with Windows XP as well as later Windows systems.  That took a bunch of fancy footwork (hacks?) to tie together the two distinct notification systems (a dllMain callback, vs some linker hackery) so that which ever one was supported/active "would be used."  The details are heavily documented in src/base/win/dllmain.cc (which specifically talks about keeping in sync with base/threading/thread_local_storage_win.cc).

Finally, the support for TCMalloc has to be done well.  It is critical to handle <sigh> cases such as where a TLS destructor calls malloc or free... and hence TCMalloc must (again) clean up its TLS heap.  This is all naturally(?) handled by the pthread-style destructor callback interface/semantics, which keeps trying to run through the destructor list until all destructors have effectively un-registered (NULLed out their TLS slots) and no others have re-registered (set their slot again).  

In the end, I let TCMalloc get its destruction notification on a lower (Windows callback) chain, so that I was more simply assured that TCMalloc would be TLS destroyed *after* all the standard Chrome slots were destroyed. I think the critical code is in third_party/tcmalloc/vendor/src/windows/port.cc.

Still, the pthreads-style interface made sure dependent TLS destructors in Chrome "worked out their differences" cleanly.

If you wonder about who else is playing with the linker magic cited above, you can use cs.chromium.org to search for this string:

  CRT\$XL

It was interesting to notice that skia is playing around here... and it might be nice to get that system tied into our standard(?) Chromium TLS allocator.  The good news is that skia used an alphabetically lower name, so TCMalloc will (still) be destroyed after skia is.  :-)

It was my fear that other such systems would "roll their own" TLS that motivated me to put TCMalloc on a lower level callback chain, and maximally late in the Win-OS TLS destructor sequence.

<Whew>.  It was that simple ;-)  ... and I'm hoping it is still working.

Dec 19, 2013
#29 cpu@chromium.org
The guidance from microsoft is that you shall not use tls slots long term
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686760(v=vs.85).aspx

We are in 'don't do this' territory, we have been as such, for a long time. I can go into detail. I don't think we will explode, but I won't be surprised if other unexpected things (leaks or magically freed slots) happen.

I did superficial testing back when TCmalloc landed, but at that time TCmalloc had some very big (unrelated) leaks so it was not possible to tell.

Fast forward to win8, their threadpool implementation is vastly different and mostly lives in kernel, so even more magical things can happen.
Dec 19, 2013
#30 michaelbai@chromium.org
It seemed it is hard to say the implementation of Windows TLS destructors is problematic or not, we could encourage user to use thread_local by add comments in thread_local_storage.h as

We don't know whether the TLS destructors works reliably in Windows, please use ThreadLocalPointer/ThreadLocalBoolean if possible, see 'base/threading/thread_local.h' and http://crbug.com/264406

Make sense?

Dec 20, 2013
#31 darin@chromium.org
I'm not sure that comment will be that useful. It warns you about something we do a lot of.

It might be helpful to study the behavior on Windows. It wouldn't be that hard to test if Win32 worker threads run TLS destructors. Note, I believe there is work underway to eliminate base/threading/worker_pool.h, which would eliminate a good chunk of our usage of Win32 worker threads.

Perhaps it would be wise to make the ThreadLocalStorage implementation optionally (under an #ifdef) look more like raw calls to the OS functions if that works for the OS? It doesn't seem like there is any point to implementing TLS on top of one native TLS slot on some platforms.
Dec 20, 2013
#32 cpu@chromium.org
>> It wouldn't be that hard to test if Win32 worker threads run TLS destructors. 

Along with the 'when' are they run. The afaik the XP era api [1] does not have a shutdown function. Given that the threadpool is implemented in kernel32/ntll one can imagine that the threads exit after chrome.dll and chrome.exe have unwound.

If I was Microsoft I would kill them dead. Less compat problems.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms686766(v=vs.85).aspx

Dec 20, 2013
#33 michaelbai@chromium.org
The implementation of TLS on top of one native TLS slot doesn't conflict with removing TLS destructor, the TLS without destructor is same as passing destructor as NULL to ThreadLocalStorage.

They are two separated issues.
Dec 26, 2013
#34 bugdro...@chromium.org
------------------------------------------------------------------------
r242549 | kbr@chromium.org | 2013-12-26T20:05:11.977181Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage_win.cc?r1=242549&r2=242548&pathrev=242549
   D http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage.cc?r1=242549&r2=242548&pathrev=242549
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage.h?r1=242549&r2=242548&pathrev=242549
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage_posix.cc?r1=242549&r2=242548&pathrev=242549
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?r1=242549&r2=242548&pathrev=242549

Revert 241657 "Implement chromium's TLS."

Caused intermittent browser process crashes on Linux Debug configurations.
See  Issue 329747  for details.

BUG=329747,264406

> Implement chromium's TLS.
> 
> Using one system TLS to implement multiple chrome's TLS slots.
> 
> BUG=264406
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241144
> 
> Review URL: https://codereview.chromium.org/60743004

TBR=michaelbai@chromium.org

Review URL: https://codereview.chromium.org/121393002
------------------------------------------------------------------------
Dec 26, 2013
#35 kbr@chromium.org
r241657 caused intermittent browser process crashes on the Linux Debug (NVIDIA) bot on the chromium.gpu waterfall and I've reverted it in https://codereview.chromium.org/121393002/ / https://src.chromium.org/viewvc/chrome?view=rev&revision=242549 . See  Issue 329747 , and below for the stack trace in gdb that clearly points to this CL being at fault.

Unfortunately I wasn't able to reproduce these crashes on my local workstation, only on the R210s in the lab, but I might not have used the same GYP_DEFINES as on the bot: in particular, "component=shared_library disable_glibcxx_debug=1".

Please contact me directly if you need help reproducing these crashes.

#0  0x00007fd944c12833 in tcmalloc::PageHeap::Carve (this=0x0, span=0x0, n=0) at ../../third_party/tcmalloc/chromium/src/page_heap.cc:176
#1  0x00007fd944c11c5f in tcmalloc::PageHeap::SearchFreeAndLargeLists (this=0x7fd92b610000, n=2) at ../../third_party/tcmalloc/chromium/src/page_heap.cc:86
#2  0x00007fd944c11e52 in tcmalloc::PageHeap::New (this=0x7fd92b610000, n=2) at ../../third_party/tcmalloc/chromium/src/page_heap.cc:97
#3  0x00007fd944c060ca in tcmalloc::CentralFreeList::Populate (this=0x7fd9482a4120) at ../../third_party/tcmalloc/chromium/src/central_freelist.cc:315
#4  0x00007fd944c05efb in tcmalloc::CentralFreeList::FetchFromSpansSafe (this=0x7fd9482a4120) at ../../third_party/tcmalloc/chromium/src/central_freelist.cc:283
#5  0x00007fd944c05e29 in tcmalloc::CentralFreeList::RemoveRange (this=0x7fd9482a4120, start=0x7fd9362967b8, end=0x7fd9362967c0, N=1)
    at ../../third_party/tcmalloc/chromium/src/central_freelist.cc:262
#6  0x00007fd944bf6590 in tcmalloc::ThreadCache::FetchFromCentralCache (this=0x7fd92b5f4480, cl=23, byte_size=576)
    at ../../third_party/tcmalloc/chromium/src/thread_cache.cc:165
#7  0x00007fd944bfcdfa in tcmalloc::ThreadCache::Allocate (this=0x7fd92b5f4480, size=576, cl=23) at ../../third_party/tcmalloc/chromium/src/thread_cache.h:368
#8  0x00007fd944bfa023 in (anonymous namespace)::do_malloc (size=576) at ../../third_party/tcmalloc/chromium/src/tcmalloc.cc:1111
#9  0x00007fd944bfe8f3 in MallocBlock::Allocate (size=512, type=-1125458062) at ../../third_party/tcmalloc/chromium/src/debugallocation.cc:519
#10 0x00007fd944bfbe7d in DebugAllocate (size=512, type=-1125458062) at ../../third_party/tcmalloc/chromium/src/debugallocation.cc:994
#11 0x00007fd944bff8c7 in debug_cpp_alloc (size=512, new_type=-1125458062, nothrow=false) at ../../third_party/tcmalloc/chromium/src/debugallocation.cc:1113
#12 0x00007fd946e960f2 in tc_newarray (size=512) at ../../third_party/tcmalloc/chromium/src/debugallocation.cc:1254
#13 0x00007fd941e00c92 in (anonymous namespace)::ConstructTlsVector () at ../../base/threading/thread_local_storage.cc:104
#14 0x00007fd941e0123d in base::ThreadLocalStorage::StaticSlot::Get (this=0x7fd941ef6b88) at ../../base/threading/thread_local_storage.cc:233
#15 0x00007fd941e079d0 in tracked_objects::ThreadData::InitializeThreadContext (suggested_name="CrShutdownDetector") at ../../base/tracked_objects.cc:318
#16 0x00007fd941dedf7e in base::PlatformThread::SetName (name=0x7fd946ee8a5b "CrShutdownDetector") at ../../base/threading/platform_thread_linux.cc:49
#17 0x00007fd944ec06d3 in (anonymous namespace)::ShutdownDetector::ThreadMain (this=0x57e2d0cd070) at ../../chrome/browser/chrome_browser_main_posix.cc:203
#18 0x00007fd941dee425 in base::(anonymous namespace)::ThreadFunc (params=0x7fffb74e9dc0) at ../../base/threading/platform_thread_posix.cc:80
#19 0x00007fd9333e8e9a in start_thread (arg=0x7fd936298700) at pthread_create.c:308
#20 0x00007fd93229b3fd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#21 0x0000000000000000 in ?? ()

Cc: kbr@chromium.org
Dec 26, 2013
#36 kbr@chromium.org
(No comment was entered for this change.)
Blockedon: chromium:329747
Jan 7, 2014
#37 michaelbai@chromium.org
I were able to reproduce the crash locally, it seemed not my patch's issue, my patch just made it easy to trigger this crash, that's why kbr also got the crash without my patch, I guess it is the edge case of tcmalloc, I filled another bug to track it. 

https://code.google.com/p/chromium/issues/detail?id=332297
Blockedon: chromium:332297
Jan 8, 2014
#38 kbr@chromium.org
To be clear, the assertion failure I saw without your patch was a preexisting bug in GPU process shutdown. Your patch was definitely the cause of the intermittent browser process crashes.

Jan 9, 2014
#39 michaelbai@chromium.org
The crash was fixed by https://src.chromium.org/viewvc/chrome?view=rev&revision=243702

I am landing the patch again. Let me know if you still see any issue after it landed.
Mar 4, 2014
#41 michaelbai@chromium.org
Issue 341800 has been merged into this issue.
Mar 5, 2014
#42 bugdro...@chromium.org
------------------------------------------------------------------------
r254986 | michaelbai@chromium.org | 2014-03-05T09:52:49.993256Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage.cc?r1=254986&r2=254985&pathrev=254986
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?r1=254986&r2=254985&pathrev=254986
   D http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_win.cc?r1=254986&r2=254985&pathrev=254986
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local.h?r1=254986&r2=254985&pathrev=254986
   D http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_posix.cc?r1=254986&r2=254985&pathrev=254986

Switch thread_local to use chromium's TLS implementation

This patch switch thread_local to use ThreadLocalStorage::Slot, so it get the
TLS slot from the chromium's TLS instead of OS's.

There is debate whether the Windows' TLS's destructor implementation is reliable
or not, if we are able to remove all the use of TLS destructor, only ThreadLocalStorage::Slot
needs to change, as thread_local doesn't use TLS destructor at all.

BUG=264406

Review URL: https://codereview.chromium.org/139633003
------------------------------------------------------------------------
Mar 5, 2014
#43 bugdro...@chromium.org
------------------------------------------------------------------------
r255102 | oshima@chromium.org | 2014-03-05T18:47:46.378428Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local.h?r1=255102&r2=255101&pathrev=255102
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_posix.cc?r1=255102&r2=255101&pathrev=255102
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage.cc?r1=255102&r2=255101&pathrev=255102
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?r1=255102&r2=255101&pathrev=255102
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_win.cc?r1=255102&r2=255101&pathrev=255102

Revert 254986 "Switch thread_local to use chromium's TLS impleme..."

> Switch thread_local to use chromium's TLS implementation
> 
> This patch switch thread_local to use ThreadLocalStorage::Slot, so it get the
> TLS slot from the chromium's TLS instead of OS's.
> 
> There is debate whether the Windows' TLS's destructor implementation is reliable
> or not, if we are able to remove all the use of TLS destructor, only ThreadLocalStorage::Slot
> needs to change, as thread_local doesn't use TLS destructor at all.
> 
> BUG=264406
> 
> Review URL: https://codereview.chromium.org/139633003

TBR=michaelbai@chromium.org

Review URL: https://codereview.chromium.org/184273006
------------------------------------------------------------------------
Mar 5, 2014
#44 kbr@chromium.org
When reverting CLs, could you please add a brief description of why it was reverted, so others know what symptoms to look for?

Cc: oshima@chromium.org
Mar 6, 2014
#45 michaelbai@chromium.org
The reverted patch broke Memory FYI on Linux Tests (valgrind), with below error
slot_ < kThreadLocalStorageSize (256 vs. 256)

It means the number of TLS slot we allocated is too small, I tried to increase it to 512, the test still failed; the Linux system has 1024 slots, it means I have to define it as 1024 to pass the test.

Defining the slot number as 1024 means 4K memory will be allocated in stake, then moved to heap for each thread that use TLS.

This seems too much for the platforms other than Android, as currently, only Android is running out of TLS.

So, I decided to only switch ThreadLocal to use chromium's TLS on Android.


May 8, 2014
#46 brettw@chromium.org
(No comment was entered for this change.)
Summary: Limit total number of system TLS slots used across Chrome (was: Limit total number of TLS slots used across Chrome)
May 12, 2014
#47 bugdro...@chromium.org
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5846e5f9563a1d344e1225aa0f690fc6eb1a05ec

commit 5846e5f9563a1d344e1225aa0f690fc6eb1a05ec
Author: michaelbai@chromium.org <michaelbai@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Mon May 12 20:00:11 2014

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

Review URL: https://codereview.chromium.org/189263003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269854 0039d316-1c4b-4281-b951-d872f2087c98


May 12, 2014
#48 bugdro...@chromium.org
------------------------------------------------------------------
r269854 | michaelbai@chromium.org | 2014-05-12T20:00:11.794124Z

Changed paths:
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_android.cc?r1=269854&r2=269853&pathrev=269854
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local.h?r1=269854&r2=269853&pathrev=269854
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_posix.cc?r1=269854&r2=269853&pathrev=269854
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_local_storage.cc?r1=269854&r2=269853&pathrev=269854
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?r1=269854&r2=269853&pathrev=269854

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

Review URL: https://codereview.chromium.org/189263003
-----------------------------------------------------------------
Jun 4, 2014
#49 Gordana....@imgtec.com
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/13770/steps/gpu_unittests/logs/stdio

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?
Jun 5, 2014
#50 michaelbai@chromium.org
If this issue became worse, we could add TLS slots for Android.
Sign in to add a comment

Powered by Google Project Hosting