Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
60743004:
Implement chromium's TLS. (Closed)
|
Created:
7 years, 1 month ago by michaelbai Modified:
6 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, bajones Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement 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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244124
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 22
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : sync #Patch Set 8 : #
Total comments: 5
Patch Set 9 : #
Total comments: 2
Patch Set 10 : address comments #Patch Set 11 : fix release build #Patch Set 12 : sync and reland #
Messages
Total messages: 37 (0 generated)
https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.cc:55: CHECK(PlatformThreadLocalStorage::AllocTLS(&key)); Note: I used CHECK here, we almost can do nothing if it failed. This is same as thread_local_posix.cc
ping
I have some high level expectations, but I don't see them expressed in a single header file, that most Chromium users would call. The header would need to carefully define semantics, and usage of the methods. That has to be the starting point (for a consumer of this interface). You can then hide a lot of stuff in internal implementations. My general expectation is to support something like pthread semantics on all platforms. Fro most platforms, this is simple, and for Windows, we have a reasonable implementation. You just have to hide the details, and support an abstracted API. I just skimmed through, and didn't review details carefully, but here are some comments. https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.cc:59: // whether the key is set in below NoBarrier_CompareAndSwap. Please make this run-on sentence into a set of smaller sentences. https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.h:37: static bool AllocTLS(TLSKey* key); The cross platform API must include destructors (called on each thread as the thread is terminated). Why are you not including it? https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.h:42: // This is the TLS destructor and not a cross-platform API. The implementation Please describe what the method is for, or what it does. https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.h:43: // of cross-platform APIs should call this method when thread exit. The I couldn't grok: ...call this method when thread exit. Do you mean: ...call this method on the terminating thread when a thread exits. https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.h:44: // |value| is always NULL in Windows. I couldn't understand this comment, from start to finish. This last line tells me that an argument is null one one platform, but gave no hint as to what the argument is, does, or is used for on any other platform. https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage_win.cc (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage_win.cc:25: bool PlatformThreadLocalStorage::AllocTLS(TLSKey* key) { I'm very surprised that you don't accept a destructor callback. I know of consumers that will need this, and would expect to see it on all platforms. What am I missing?
I were thinking the ThreadLocalStorage::StaticSlot/Slot is sufficient to use as STL for the reset of world, if we want pthread semantics TLS interface, should I add the TLS methods into ThreadLocalStorage or have a brand new class? Current PlatformThreadLocalStorage class could move to its own header file. The reason I put them together because thread_local.h does this. I want to clear these issues before provide a new patch. Please also check my reply for your comments Thanks https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.h:37: static bool AllocTLS(TLSKey* key); That because Windows doesn't support destructor key mapping, I explain why I did this in cc file's comment, please check that. On 2013/11/20 01:46:15, jar wrote: > The cross platform API must include destructors (called on each thread as the > thread is terminated). Why are you not including it? https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage_win.cc (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage_win.cc:25: bool PlatformThreadLocalStorage::AllocTLS(TLSKey* key) { Windows doesn't support key-destructor mapping, right? If I put it as parameter here, we need to implement that mapping because we might alloc 2 system key in some cases, it might difficult to implement that mapping in Windows since we want to limit the base service we used. I think we don't want to use new/malloc etc. Well, we actually only have one destructor in this particular implementation, but I am not comfortable that we have a static variable to hold that destructor in Windows, it kind of hack and not easy to be found since from API it seemed that there is a key and destructor mapping. So I choose to have the destructor in PlatformThreadLocalStorage, and it should be called when thread terminates. It might have other way to implement destructor, I might not aware of it. On 2013/11/20 01:46:15, jar wrote: > I'm very surprised that you don't accept a destructor callback. > > I know of consumers that will need this, and would expect to see it on all > platforms. What am I missing?
PTAL I didn't move PlatformThreadLocalStorage to its own header file, because the destructor of platform TLS(PlatformThreadLocalStorage::OnThreadExit) needs to access chrome's TLS vector which is defined in thread_local_storage.cc, I think it is OK for PlatformThreadLocalStorage to stay in thread_local_storage.h as long as it is in internal namespace, and we have comments to say it shouldn't be used by anyone else except ThreadLocalStorage; also, thread_local.h has same pattern. https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/1/base/threading/thread_local_s... base/threading/thread_local_storage.cc:59: // whether the key is set in below NoBarrier_CompareAndSwap. On 2013/11/20 01:46:15, jar wrote: > Please make this run-on sentence into a set of smaller sentences. Done.
https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:18: // keep track of and call when threads terminate. nit: Starting on line 13, better comment would be: In order to make TLS destructors work, we need to keep around a function pointer to the destructor for each slot. We keep this array of pointers in a global (static) array. We use the single OS-level TLS slot (giving us one pointer per thread) to hold a pointer to a per-thread array (table) of slots that we allocate to Chromium consumers. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:52: void** ConstructTlsVector() { Please add a comment: This function is called to initialize our entire Chromium TLS system. It may be called very early, and we need to complete most all of the setup (initialization) before calling *any* memory allocator functions, which may recursively depend on this initialization. As a result, we use Atomics, and avoid anything (like a singleton) that might require memory allocations. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:53: if (g_native_tls_key == PlatformThreadLocalStorage::TLS_KEY_OUT_OF_INDEXES) { I think you need an atomic access ofr g_native_tls_key here. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:66: PlatformThreadLocalStorage::FreeTLS(tmp); This and line 78 are perhaps your only use of FreeTLS... and it is probably better to leak this value. I don't expect this code to be used... and I wonder if we *do* free the value, will be be reallocated? I suspect not... Maybe it is "nicer" to be clean and call FreeTLS. Note that at process teardown, we do *not* FreeTLS... to we are fine with leaking some of this. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:68: // Atomically test-and-set the tls_key. If the key is TLS_OUT_OF_INDEXES, This comment about TLS_OUT_OF_INDEXES seems to have changed with your new code. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:81: DCHECK(!PlatformThreadLocalStorage::GetTLSValue(g_native_tls_key)); I think that once you declare a value to be Atomic, all accesses much use the Atomic functions. My guess is that you want to do an Atomic load... and then keep it in a local for uses such as on line 94. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:36: // just alloc another one. nit: I don't understand the comment. Perhaps you meant to say: The following is a "reserved key" which is used in our generic Chromium ThreadLocalStorage implementation. We expect that an OS will not return such a key... but if it is returned (i.e., the OS tries to allocate it) we will just discard/leak it, and request another key. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:39: // Cross-platform APIs nit: Better comment is: The following methods need to be supported on each OS platform, so that the Chromium ThreadLocalStore functionality can be constructed. Chromium will use these methods to acquire a single OS slot, and then use that to support a much larger number of Chromium slots (independent of the OS restrictions). https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:40: static bool AllocTLS(TLSKey* key); You need a comment above line 40 this describing the API: The following returns true IFF it successfully is able to return an OS key in |key|. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:41: static void FreeTLS(TLSKey key); Why do we need to Free our OS slot? Why do we care about the leak? https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:45: // This is the TLS destructor, the implementation of cross-platform APIs Nit: Delete the phrase "This is the TLS destructor," I think the comment should be: Each platform (OS implementation) is required to call this method on each terminating thread when the thread is about to terminate. This method will then call all registered destructors for slots in Chromium ThreadLocalStorage, until there are no slot values remaining as having been set on this thread. [Destructors may end up being called multiple times on a terminating thread, as other destructors may re-set slots that were previously destroyed.]
PTAL I should address all your comments, I also found an issue of my previous patch, I need to cast the native_tls_key before pass it to atomicops. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:18: // keep track of and call when threads terminate. On 2013/11/26 20:05:24, jar wrote: > nit: Starting on line 13, better comment would be: > > In order to make TLS destructors work, we need to keep around a function pointer > to the destructor for each slot. We keep this array of pointers in a global > (static) array. > We use the single OS-level TLS slot (giving us one pointer per thread) to hold a > pointer to a per-thread array (table) of slots that we allocate to Chromium > consumers. Done. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:52: void** ConstructTlsVector() { On 2013/11/26 20:05:24, jar wrote: > Please add a comment: > > This function is called to initialize our entire Chromium TLS system. > It may be called very early, and we need to complete most all of the setup > (initialization) before calling *any* memory allocator functions, which may > recursively depend on this initialization. > As a result, we use Atomics, and avoid anything (like a singleton) that might > require memory allocations. Done. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:53: if (g_native_tls_key == PlatformThreadLocalStorage::TLS_KEY_OUT_OF_INDEXES) { On 2013/11/26 20:05:24, jar wrote: > I think you need an atomic access ofr g_native_tls_key here. Done. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:66: PlatformThreadLocalStorage::FreeTLS(tmp); Right, I want it be clean, it looks weird if there is no FreeTLS there, On 2013/11/26 20:05:24, jar wrote: > This and line 78 are perhaps your only use of FreeTLS... and it is probably > better to leak this value. I don't expect this code to be used... and I wonder > if we *do* free the value, will be be reallocated? I suspect not... > > Maybe it is "nicer" to be clean and call FreeTLS. > > Note that at process teardown, we do *not* FreeTLS... to we are fine with > leaking some of this. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:68: // Atomically test-and-set the tls_key. If the key is TLS_OUT_OF_INDEXES, I changed TLS_OUT_OF_INDEXES to TLS_KEY_OUT_OF_INDEXES, otherwise I didn't do anything different On 2013/11/26 20:05:24, jar wrote: > This comment about TLS_OUT_OF_INDEXES seems to have changed with your new code. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.cc:81: DCHECK(!PlatformThreadLocalStorage::GetTLSValue(g_native_tls_key)); On 2013/11/26 20:05:24, jar wrote: > I think that once you declare a value to be Atomic, all accesses much use the > Atomic functions. My guess is that you want to do an Atomic load... and then > keep it in a local for uses such as on line 94. Thanks, Done. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:36: // just alloc another one. Correct, update the comment. On 2013/11/26 20:05:24, jar wrote: > nit: I don't understand the comment. Perhaps you meant to say: > > The following is a "reserved key" which is used in our generic Chromium > ThreadLocalStorage implementation. We expect that an OS will not return such a > key... but if it is returned (i.e., the OS tries to allocate it) we will just > discard/leak it, and request another key. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:39: // Cross-platform APIs On 2013/11/26 20:05:24, jar wrote: > nit: Better comment is: > > The following methods need to be supported on each OS platform, so that the > Chromium ThreadLocalStore functionality can be constructed. Chromium will use > these methods to acquire a single OS slot, and then use that to support a much > larger number of Chromium slots (independent of the OS restrictions). Done. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:40: static bool AllocTLS(TLSKey* key); On 2013/11/26 20:05:24, jar wrote: > You need a comment above line 40 this describing the API: > > The following returns true IFF it successfully is able to return an OS key in > |key|. > Done. https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:41: static void FreeTLS(TLSKey key); I think it just here for code looks clean. On 2013/11/26 20:05:24, jar wrote: > Why do we need to Free our OS slot? Why do we care about the leak? https://codereview.chromium.org/60743004/diff/90001/base/threading/thread_loc... base/threading/thread_local_storage.h:45: // This is the TLS destructor, the implementation of cross-platform APIs On 2013/11/26 20:05:24, jar wrote: > Nit: Delete the phrase "This is the TLS destructor," > > I think the comment should be: > > Each platform (OS implementation) is required to call this method on each > terminating thread when the thread is about to terminate. This method will then > call all registered destructors for slots in Chromium ThreadLocalStorage, until > there are no slot values remaining as having been set on this thread. > [Destructors may end up being called multiple times on a terminating thread, as > other destructors may re-set slots that were previously destroyed.] Done.
https://codereview.chromium.org/60743004/diff/110001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/110001/base/threading/thread_lo... base/threading/thread_local_storage.cc:55: base::subtle::NoBarrier_Load(reinterpret_cast<long*>(&g_native_tls_key))); This cast is IMO going the wrong way. The variable should be Atomic, and only accessed via atomic primitives. When you extract the value, you may pass it around on the thread without atomic primitives. In addition, when you call these Atomic operators, you should give them an address of an Atomic value... but here you have oddly casted away the Atomic type... and passed the pointer to the non-atomic type into the Atomic operator. https://codereview.chromium.org/60743004/diff/110001/base/threading/thread_lo... base/threading/thread_local_storage.cc:101: base::subtle::Atomic32 key = GetNativeTLSKey(); Why don't you do this before line 69, and then maybe update it if we had to change, inside the section from lines 70-99? It is also probably better avoid having two |key| values in this method (see line 70 vs line 101).
Jim, PTAL There is 2 main changes of patch# 4 1. I used AtomicWord for g_native_tls_key, it should large enough for both Windows and Posix. 2. I added value parameter back to PlatformThreadLocalStorage::OnThreadExit(), because the TLS value was already reset in Posix before OS call the TLS destructor, we can't get the value back in destructor. I think it is the reason the destructor has value parameter in Posix. https://codereview.chromium.org/60743004/diff/110001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/110001/base/threading/thread_lo... base/threading/thread_local_storage.cc:55: base::subtle::NoBarrier_Load(reinterpret_cast<long*>(&g_native_tls_key))); On 2013/11/27 03:36:24, jar wrote: > This cast is IMO going the wrong way. The variable should be Atomic, and only > accessed via atomic primitives. When you extract the value, you may pass it > around on the thread without atomic primitives. > > In addition, when you call these Atomic operators, you should give them an > address of an Atomic value... but here you have oddly casted away the Atomic > type... and passed the pointer to the non-atomic type into the Atomic operator. Done. https://codereview.chromium.org/60743004/diff/110001/base/threading/thread_lo... base/threading/thread_local_storage.cc:101: base::subtle::Atomic32 key = GetNativeTLSKey(); On 2013/11/27 03:36:24, jar wrote: > Why don't you do this before line 69, and then maybe update it if we had to > change, inside the section from lines 70-99? > > It is also probably better avoid having two |key| values in this method (see > line 70 vs line 101). Done.
https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.cc:86: key = base::subtle::NoBarrier_Load(&g_native_tls_key); nit: I think we only need to do this when some other thread set a better key. In most cases, we just set the key. Bottom line: Can't you do this between lines 84 and 85 above? https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.cc:118: // |value| is NULL in Windows platform which doesn't support TLS destructor, Best would be to have the platform specific code figure this out... but if you have to do it here.... Handling of this should be done in an #if defined(...) region for Windows. You can then CHECK() that you're not called (for example) on Posix with a null value. https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.cc:204: if (slot_ >= kThreadLocalStorageSize) { Can you change this to a: CHECK_LT(slot_, kThreadLocalStorageSize); We really want to crash when we've exceeded this limit. https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.h:67: static void OnThreadExit(void* value); It sure would have been nice if the Window's implementation converted the call into one that has the actual OS slot. Then you wouldn't have to talk about the distinction here. Why wouldn't that work?
PTAL, thanks https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.cc:86: key = base::subtle::NoBarrier_Load(&g_native_tls_key); Right, it should be between line 84 and 85. On 2013/12/05 19:53:32, jar wrote: > nit: I think we only need to do this when some other thread set a better key. > In most cases, we just set the key. > > Bottom line: Can't you do this between lines 84 and 85 above? https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.cc:118: // |value| is NULL in Windows platform which doesn't support TLS destructor, On 2013/12/05 19:53:32, jar wrote: > Best would be to have the platform specific code figure this out... but if you > have to do it here.... > > Handling of this should be done in an #if defined(...) region for Windows. > > You can then CHECK() that you're not called (for example) on Posix with a null > value. I wanted to avoid #if defined(OS_WIN) code here, In Posix, the value can't be NULL, these code is de facto Windows specific code; but I don't have strong feeling about this. https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.cc:204: if (slot_ >= kThreadLocalStorageSize) { On 2013/12/05 19:53:32, jar wrote: > Can you change this to a: > CHECK_LT(slot_, kThreadLocalStorageSize); > > We really want to crash when we've exceeded this limit. Done. https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/130001/base/threading/thread_lo... base/threading/thread_local_storage.h:67: static void OnThreadExit(void* value); In order to do this, we need to know the TLS key is actually used in Windows implementation, but there is no good way to do this. On 2013/12/05 19:53:32, jar wrote: > It sure would have been nice if the Window's implementation converted the call > into one that has the actual OS slot. Then you wouldn't have to talk about the > distinction here. > > Why wouldn't that work?
One last thing I noticed... see comments below. Also, with regard to the ifdef for the platform: You could define two entry points for a thread termination, one that windows calls (without pulling out the OS key), and one that Linux calls (having already pulled out the key). Either one could then set the state to be equivalent to the other... and then call into common code. You're doing this via ifdefs... but it might be cleaner to have them separate. I can live with what you have.... but it would be smoother the other way. https://codereview.chromium.org/60743004/diff/150001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/150001/base/threading/thread_lo... base/threading/thread_local_storage.cc:185: PlatformThreadLocalStorage::SetTLSValue(key, NULL); Now that you're handling both Linux and Windows, you need to be careful to reset your state to the initial state. PlatformThreadLocalStorage::SetTLSValue(key, PlatformThreadLocalStorage::TLS_KEY_OUT_OF_INDEXES). This is just in case some other (OS Level) TLS destructor causes us to get called again (for example: as an allocator?), then we'll re-initialize... and then we'll also then get called to clean out our slots.
LGTM (modulo the alternate suggestion made earlier about where to put platform specific distinctions) https://codereview.chromium.org/60743004/diff/150001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/150001/base/threading/thread_lo... base/threading/thread_local_storage.cc:185: PlatformThreadLocalStorage::SetTLSValue(key, NULL); <doh> This comment is mistaken. NULL is correct to return this *thread* to the uninitialized state. On 2013/12/06 22:20:09, jar wrote: > Now that you're handling both Linux and Windows, you need to be careful to reset > your state to the initial state. > > > PlatformThreadLocalStorage::SetTLSValue(key, > PlatformThreadLocalStorage::TLS_KEY_OUT_OF_INDEXES). > > This is just in case some other (OS Level) TLS destructor causes us to get > called again (for example: as an allocator?), then we'll re-initialize... and > then we'll also then get called to clean out our slots.
Used different OnThreadExit callback for Windows and Posix
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/60743004/210001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
PTAL
This is indeed much closer to what I was after. You'll end up needing a base/* reviewer as well (I've opted out of the job)... but I have a history with these TLS files, so I don't mind helping out on them. https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... base/threading/thread_local_storage.cc:176: PlatformThreadLocalStorage::TLSKey key = You *could* put all this code in the windows only file... and then just call to to the common code directly (what you call OnTheradExitInternal(...)). That is what you posix stub does (and it could be more direct as well). You'd then end up not needing different callbacks. https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... base/threading/thread_local_storage.cc:179: return; nit: undent 2
I noticed you removed yourself from base owner, it is very nice to have you help on reviewing this. jam is the owner of threading directory, I'd like him to review this CL, the gyp change which is in base directory is very simple. https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... base/threading/thread_local_storage.cc:176: PlatformThreadLocalStorage::TLSKey key = I can't do that, the Windows version of OnThreadExit() needs to access g_native_tls_key which is not visible for Windows implementation. On 2013/12/10 02:27:11, jar wrote: > You *could* put all this code in the windows only file... and then just call to > to the common code directly (what you call OnTheradExitInternal(...)). That is > what you posix stub does (and it could be more direct as well). > > You'd then end up not needing different callbacks. > https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... base/threading/thread_local_storage.cc:179: return; On 2013/12/10 02:27:11, jar wrote: > nit: undent 2 Done.
LGTM https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... File base/threading/thread_local_storage.cc (right): https://codereview.chromium.org/60743004/diff/210001/base/threading/thread_lo... base/threading/thread_local_storage.cc:176: PlatformThreadLocalStorage::TLSKey key = On 2013/12/10 03:00:57, michaelbai wrote: > I can't do that, the Windows version of OnThreadExit() needs to access > g_native_tls_key which is not visible for Windows implementation. > > > On 2013/12/10 02:27:11, jar wrote: > > You *could* put all this code in the windows only file... and then just call > to > > to the common code directly (what you call OnTheradExitInternal(...)). That > is > > what you posix stub does (and it could be more direct as well). > > > > You'd then end up not needing different callbacks. > > > Your right. thanks. This way seems fine.
On 2013/12/10 03:00:57, michaelbai wrote: > I noticed you removed yourself from base owner, it is very nice to have you help > on reviewing this. > > jam is the owner of threading directory, I'd like him to review this CL, the gyp > change which is in base directory is very simple. per the owners file there: # For thread_resrictions.* 2 jam@chromium.org
PTAL
ping, brett, PTAL
I'm assuming jar did a thorough review of the details, this is base owners LGTM stamp. https://codereview.chromium.org/60743004/diff/230001/base/threading/thread_lo... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/230001/base/threading/thread_lo... base/threading/thread_local_storage.h:38: // The following methods need to be supported on each OS platform, so that Can you put a blank line above this to help separate it from the platform stuff above?
https://codereview.chromium.org/60743004/diff/230001/base/threading/thread_lo... File base/threading/thread_local_storage.h (right): https://codereview.chromium.org/60743004/diff/230001/base/threading/thread_lo... base/threading/thread_local_storage.h:38: // The following methods need to be supported on each OS platform, so that On 2013/12/16 17:56:37, brettw wrote: > Can you put a blank line above this to help separate it from the platform stuff > above? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/60743004/250001
Message was sent while issue was closed.
Change committed as 241144
Fixed the crash on buildbot, there is no major change, I were making a stupid mistake to put some function call into DCHECK. Land it again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/60743004/270001
Message was sent while issue was closed.
Change committed as 241657
Message was sent while issue was closed.
Note that this CL caused intermittent browser process crashes on Linux Debug configurations and was reverted in https://codereview.chromium.org/121393002 / https://src.chromium.org/viewvc/chrome?view=rev&revision=242549 . See http://crbug.com/329747 .
Reland as the crash has been fixed by https://src.chromium.org/viewvc/chrome?revision=243702&view=revision
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/60743004/340001
+bajones (current GPU sheriff) Brandon, please watch out for intermittent failures on the Linux Debug (NVIDIA) bot after this lands.
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/60743004/340001
Message was sent while issue was closed.
Change committed as 244124 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
