Using C++11 thread_local storage class

[Ken Rockot]

I see there is currently no usage of this in the tree, and I would like to introduce some. To my surprise I can find no evidence of prior discussion.

What's the deal? Can we use it? If not, is there a suitable alternative?

Specifically of interest is a need to ensure that the lifetime of my thread-local thing is actually bounded by each thread's lifetime, and our //base thread-local support does not appear to be sufficient for such things.

[Nico Weber]

(moving to cxx@ per http://chromium-cpp.appspot.com/)

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/415f44d6-88ed-476c-a6e1-cacccfe2cc85%40chromium.org.

[Gabriel Charette]

//base's TLS has support for custom destructor FWIW (DesrructorFunc IIRC)

[Darin Fisher]

Also, compilers have long had built-ins like this, but I vaguely remember these having issues when used within a dynamically loaded at runtime (not specified at link time) DSO / DLL.

-Darin

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LJTH9H54OikYw5L9beNoVfXxpLO9BcRw17_5MJLiz5yrA%40mail.gmail.com.

[Gabriel Charette]

base does support custom destructors for TLS state (see DestructorFunc IIRC)

[Brett Wilson]

I don't think we would want to use anything but the base implementation of TLS.

On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again.

We should probably put this on the explicitly banned list.

Brett

--

[Ken Rockot]

Thanks for the reference to ThreadLocalStorage::DestructorFunc, which I clearly missed. That sounds sufficient to me.

On Tue, Aug 15, 2017 at 3:04 PM, Brett Wilson <bre...@chromium.org> wrote:

I don't think we would want to use anything but the base implementation of TLS.

On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again.

We should probably put this on the explicitly banned list.

Noted. That makes sense and sounds like the right thing to do.

[Joe Mason]

This seems like a good thread to mention that we've seen crashes on exit using base TLS on some Windows 8 machines: see https://bugs.chromium.org/p/chromium/issues/detail?id=704591

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BapAgH4NoB5wjE_P5P6GJ2ZrB3E3fRQXUw%2BA-u%3D1wsyyafuPw%40mail.gmail.com.

[Leonard Mosescu]

On Tue, Aug 15, 2017 at 3:04 PM, Brett Wilson <bre...@chromium.org> wrote:

I don't think we would want to use anything but the base implementation of TLS.

On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again.

There may be some valid concerns about thread_local but this should not be one of them (it is not technically feasible to map thread_local variables to individual TLS slots provided by the host OS).

 

We should probably put this on the explicitly banned list.

What's the rule of thumb here? I'm new to Chrome but I'd expect that all thing being equal we'd favor standard language features over hand crafted alternatives. Thread storage class is a first class, fundamental part of C++ since C++11, with well defined semantics (and one would expect first class, solid implementations in all the major toolchains)

 

Brett

On Mon, Aug 14, 2017 at 10:11 AM, Ken Rockot <roc...@chromium.org> wrote:

I see there is currently no usage of this in the tree, and I would like to introduce some. To my surprise I can find no evidence of prior discussion.

What's the deal? Can we use it? If not, is there a suitable alternative?

Specifically of interest is a need to ensure that the lifetime of my thread-local thing is actually bounded by each thread's lifetime, and our //base thread-local support does not appear to be sufficient for such things.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/415f44d6-88ed-476c-a6e1-cacccfe2cc85%40chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiGVV8t8SG3kRp8d1jh9YT5stzcmfEbv%2Bku-0Nb2aJpJSTZBg%40mail.gmail.com.

[Brett Wilson]

On Tue, Aug 15, 2017 at 3:47 PM, Leonard Mosescu <mos...@chromium.org> wrote:

On Tue, Aug 15, 2017 at 3:04 PM, Brett Wilson <bre...@chromium.org> wrote:

I don't think we would want to use anything but the base implementation of TLS.

On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again.

There may be some valid concerns about thread_local but this should not be one of them (it is not technically feasible to map thread_local variables to individual TLS slots provided by the host OS).

 

We should probably put this on the explicitly banned list.

What's the rule of thumb here? I'm new to Chrome but I'd expect that all thing being equal we'd favor standard language features over hand crafted alternatives. Thread storage class is a first class, fundamental part of C++ since C++11, with well defined semantics (and one would expect first class, solid implementations in all the major toolchains)

If the implementation is good and does what we want on all platforms for all compilers we support and there are no specific concerns, then we should use it. I am not so optimistic as you about uniformly good implementations though...

Brett

[Leonard Mosescu]

MSVC places thread local variables(*) in a special section (.tls) . A single TLS slot points to the base of the thread-specific copy of this section so accessing a thread_local variable is .tls_base[offset]. I'll let Linux/macOS experts chime in about how things are done on these platforms although it seems very close to the PE/COFF solution.

(*) MSVC implements thread_local pretty much by repackaging of the old declspec(thread) with a few rough corners (around initialization) fixed.

[Jeremy Roman]

On Tue, Aug 15, 2017 at 7:11 PM, Leonard Mosescu <mos...@chromium.org> wrote:

MSVC places thread local variables(*) in a special section (.tls) . A single TLS slot points to the base of the thread-specific copy of this section so accessing a thread_local variable is .tls_base[offset]. I'll let Linux/macOS experts chime in about how things are done on these platforms although it seems very close to the PE/COFF solution.

(*) MSVC implements thread_local pretty much by repackaging of the old declspec(thread) with a few rough corners (around initialization) fixed.

As I mentioned in the cxx@ fork of this thread, on Mac implementation has sharp edges.

 

On Tue, Aug 15, 2017 at 3:56 PM, Brett Wilson <bre...@chromium.org> wrote:

On Tue, Aug 15, 2017 at 3:47 PM, Leonard Mosescu <mos...@chromium.org> wrote:

On Tue, Aug 15, 2017 at 3:04 PM, Brett Wilson <bre...@chromium.org> wrote:

I don't think we would want to use anything but the base implementation of TLS.

On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again.

There may be some valid concerns about thread_local but this should not be one of them (it is not technically feasible to map thread_local variables to individual TLS slots provided by the host OS).

 

We should probably put this on the explicitly banned list.

What's the rule of thumb here? I'm new to Chrome but I'd expect that all thing being equal we'd favor standard language features over hand crafted alternatives. Thread storage class is a first class, fundamental part of C++ since C++11, with well defined semantics (and one would expect first class, solid implementations in all the major toolchains)

If the implementation is good and does what we want on all platforms for all compilers we support and there are no specific concerns, then we should use it. I am not so optimistic as you about uniformly good implementations though...

Brett

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANFFm8g_hFQg_M1afPyJ2uWAkBi1cjAdqzHwvykEPF-hKFq27A%40mail.gmail.com.

[Leonard Mosescu]

@Thomas: good question. Short answer : you should be fine even if you don't use std::thread. Long answer: The C++ standard defines a threading model (1.10) and also provides std::thread as a way to control threads, but as far as I remember it's underspecified how things behave when other platform facilities are used to create threads. In practice, if you use the CRT you need to use a CRT facility to create thread (ex. _beginthreadex() instead of calling CreateThread() directly on Windows). This may sound a bit scary, although if you use the CRT and you don't use the CRT thread functions you have worse things to worry about than thread_local.

@Jeremy: do you happen to remember the details, or perhaps have a repro? The thread_local variables can be tricky since they have all the problems associated with global variables plus additional nastiness related to thread lifetimes.

I don't want to suggest that thread_local is cheap or safe (so I definitely don't advocate sprinkling it around). I was just curious if there are other technical reasons to outright ban the language feature and/or prefer the base TLS support.

[Gabriel Charette]

Also worth mentioning in this thread: base now provides SequenceLocalStorageSlot as a sequence-friendly version of ThreadLocalStorage::Slot. As we move more and more towards sequences, I suspect most TLS usage outside of base internals should migrate to SLS.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANFFm8h8BR04WUzsKX8MY_tu1xV%3Do0QrqPMhJemzXS-NsyMYQA%40mail.gmail.com.

[Brett Wilson]

Here is a proposed addition to the style guide that disallows this:

https://chromium-review.googlesource.com/c/617064

I'm not 100% confident about this advice as I don't have an intuitive feeling for how bad the issues on Mac are. It would be nice to use if we can.

Brett

[Lucas Gadani]

I'd like to revisit this and propose that we allow the thread_local usage.

Here's a CL that shows that it works on all platforms: https://chromium-review.googlesource.com/c/chromium/src/+/743696

The caveat on Mac is that we cannot access TLS objects during the destructors of other TLS objects, which seems fairly reasonable, and I believe is the issue that Jeremy was hitting when experimenting, and why I had to make a change in ThreadSpecific::Destroy in the CL above.

I've done some performance comparisons between using thread_local and using WTF::ThreadSpecific, and there's a 2.5x performance improvement[1] on both Windows and Mac. The generated code is also significantly smaller, which is a nice benefit as well.

[1] The testcase I used for performance comparison was:

  DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<bool>, test_bool, ());

  for (int i = 0; i < 1000000000; i++)

    *test_bool = !*test_bool;

  static thread_local volatile bool test_bool;

  for (int i = 0; i < 1000000000; i++)

    test_bool = !test_bool;

[Leonard Mosescu]

The caveat on Mac is that we cannot access TLS objects during the destructors of other TLS objects, which seems fairly reasonable

It's worth noting that this is by design and not an implementation issue. It's pretty much the same constraint that applies to global objects.

[Jeremy Roman]

On Thu, Nov 2, 2017 at 6:44 PM, Leonard Mosescu <mos...@chromium.org> wrote:

The caveat on Mac is that we cannot access TLS objects during the destructors of other TLS objects, which seems fairly reasonable

It's worth noting that this is by design and not an implementation issue. It's pretty much the same constraint that applies to global objects.

Note: at the time I tested, this included primitive types (can't access a bool, even). IIRC, the issue I encountered was roughly:

- thread_local stuff torn down

- pthread destructors run

-- WTFThreadData (inc. AtomicStringTable torn down)

-- other thing uses AtomicString; accesses ThreadSpecific<WTFThreadData>

-- ThreadSpecific::operator T* (new object case) accesses checks if main thread

-- main thread check accesses thread_local bool (mac crashed here)

- pthread destructors run again, re-destroy WTFThreadData

Are we sure this cannot still happen? It looks to me like it could (though the mayNotBeMainThread stuff might be hiding it in non-edge cases).

The order of destruction being undefined is reasonably analogous to globals; what I encountered was a primitive (which doesn't need destruction at all) going away, which AFAIK doesn't happen with globals.

[Lucas Gadani]

What I found is that on Mac, having a TLS object will automatically generate and accessor function, which dynamically allocates memory for the object on first access on that thread. They get destroyed/deallocated on thread destruction, which means we shouldn't access even POD TLS data on the destructors.

I still think this is a reasonable limitation given the benefits of improved performance and better generated code.

As for the specific issue you mentioned with AtomicStrings, I haven't seen that happening, it's possible that it got fixed. In particular, this would also be bad even without thread_local, since accessing an AtomicString after the destruction on the WTFThreadData would lead to leaked memory, since the WTFThreadData would be re-allocated and nothing will free it afterwards. I'm fine if that gets changed to a crash, since it should be fixed anyway.

[Jeremy Roman]

On Tue, Nov 7, 2017 at 11:00 AM, Lucas Gadani <l...@chromium.org> wrote:

What I found is that on Mac, having a TLS object will automatically generate and accessor function, which dynamically allocates memory for the object on first access on that thread. They get destroyed/deallocated on thread destruction, which means we shouldn't access even POD TLS data on the destructors.

I still think this is a reasonable limitation given the benefits of improved performance and better generated code.

As for the specific issue you mentioned with AtomicStrings, I haven't seen that happening, it's possible that it got fixed. In particular, this would also be bad even without thread_local, since accessing an AtomicString after the destruction on the WTFThreadData would lead to leaked memory, since the WTFThreadData would be re-allocated and nothing will free it afterwards. I'm fine if that gets changed to a crash, since it should be fixed anyway.

Good to hear it hasn't been an issue in practice anymore.

However, note that this is not quite what happened (at least on POSIX). pthreads is clever enough to try to run destructors more than once if TLS keys are created during destruction, at least PTHREAD_DESTRUCTOR_ITERATIONS (in practice, four) times. This does allow for WTFThreadData being re-freed in a subsequent pass (and definitely did happen in practice, when I last looked).

It's kinda hard to make this an absolute restriction (though I concede "eh, four ought to do it" is hardly elegant) unless we can control the order of thread destructors, because on the surface it seems reasonable for an object's destructor to access the AtomicString table (e.g. a thread-specific HashMap<AtomicString, T> would need to to remove strings from the table and unref them if they would otherwise leak).

[Lucas Gadani]

I thoroughly investigated this today, here's what I found about the behavior on Mac:

- thread_local variables are destroyed before WTF::ThreadSpecific (which uses pthreads TLS).

- If a thread_local is used after it was destroyed, it'll be re-created. This means that, for example, if you have POD it'll be reinitialized according to its default initialization.

- The exact same thing happens to WTF::ThreadSpecific, it'll be re-created if it's used after it has been destroyed. It is also re-constructed according to its default initializers.

- If you have a circular dependence between creating TLS inside the destructors, it'll eventually give up.

So, basically, thread_local on Mac behaves identically to WTF::ThreadSpecific, with the exception that it is always destroyed first.

Here's the test code I used to test this behavior:

static void TLSThreadTest();

static void TLSThreadTest2();

class TLSHelperB {

 public:

  TLSHelperB() {

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

  }

  ~TLSHelperB() {

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

    TLSThreadTest();

  }

};

class WTFTLSHelperB {

 public:

  WTFTLSHelperB() {

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

  }

  ~WTFTLSHelperB() {

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

    TLSThreadTest2();

  }

};

class TLSHelperA {

 public:

  TLSHelperA() {

    static thread_local TLSHelperB tls_helper;

    DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperB>, tls_helper_wtf, ());

    *tls_helper_wtf;

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

  }

  ~TLSHelperA() {

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

  }

};

class WTFTLSHelperA {

 public:

  WTFTLSHelperA() {

    static thread_local TLSHelperB tls_helper;

    DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperB>, tls_helper_wtf, ());

    *tls_helper_wtf;

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

  }

  ~WTFTLSHelperA() {

    LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;

  }

};

static void TLSThreadTest() {

  LOG(ERROR) << "Start " << __PRETTY_FUNCTION__;

  static thread_local TLSHelperA tls_helper_a;

  DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperA>, tls_helper_wtf, ());

  *tls_helper_wtf;

  LOG(ERROR) << "End " << __PRETTY_FUNCTION__;

}

static void TLSThreadTest2() {

  LOG(ERROR) << "Start " << __PRETTY_FUNCTION__;

  static thread_local TLSHelperA tls_helper_a;

  DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperA>, tls_helper_wtf, ());

  *tls_helper_wtf;

  LOG(ERROR) << "End " << __PRETTY_FUNCTION__;

}

TEST(..., TLS) {

  Thread a("Test");

  EXPECT_TRUE(a.Start());

  a.task_runner()->PostTask(

      FROM_HERE,

      base::BindOnce(&TLSThreadTest));

  a.task_runner()->PostTask(

      FROM_HERE,

      base::BindOnce(&TLSThreadTest2));

  a.Stop();

}

And here's the output:

Start void WTF::TLSThreadTest()

0x7ffef6e08582 WTF::TLSHelperB::TLSHelperB()

0x27fae18038 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x7ffef6e08580 WTF::TLSHelperA::TLSHelperA()

0x7ffef6e08590 WTF::TLSHelperB::TLSHelperB()

0x27fae18088 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x27fae18060 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest()

Start void WTF::TLSThreadTest2()

0x7ffef6e085a0 WTF::TLSHelperA::TLSHelperA()

0x27fae180b0 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest2()

0x7ffef6e085a0 WTF::TLSHelperA::~TLSHelperA()

0x7ffef6e08590 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

End void WTF::TLSThreadTest()

0x7ffef6e08580 WTF::TLSHelperA::~TLSHelperA()

0x7ffef6e08582 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

End void WTF::TLSThreadTest()

0x27fae18038 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

0x7ffef6e07d62 WTF::TLSHelperB::TLSHelperB()

0x7ffef6e07d80 WTF::TLSHelperA::TLSHelperA()

End void WTF::TLSThreadTest2()

0x27fae18060 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x27fae18088 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

End void WTF::TLSThreadTest2()

0x27fae180b0 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x7ffef6e07d80 WTF::TLSHelperA::~TLSHelperA()

0x7ffef6e07d62 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

0x27fae180b0 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x7ffef6e07d60 WTF::TLSHelperA::TLSHelperA()

0x7ffef6e07d70 WTF::TLSHelperB::TLSHelperB()

0x27fae18060 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x27fae18088 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest()

0x27fae180b0 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

0x7ffef6e07d62 WTF::TLSHelperB::TLSHelperB()

0x7ffef6e07d80 WTF::TLSHelperA::TLSHelperA()

0x7ffef6e07d70 WTF::TLSHelperB::TLSHelperB()

0x27fae18038 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest2()

0x27fae18088 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x27fae18060 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

End void WTF::TLSThreadTest2()

0x27fae18038 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x7ffef6e07d70 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

0x27fae18038 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x7ffef6e07d60 WTF::TLSHelperA::TLSHelperA()

0x27fae18088 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x27fae18060 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest()

0x7ffef6e07d80 WTF::TLSHelperA::~TLSHelperA()

0x7ffef6e07d62 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

End void WTF::TLSThreadTest()

0x7ffef6e07d70 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

End void WTF::TLSThreadTest()

0x7ffef6e07d60 WTF::TLSHelperA::~TLSHelperA()

0x27fae18038 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

0x7ffef6e07d62 WTF::TLSHelperB::TLSHelperB()

0x7ffef6e07d80 WTF::TLSHelperA::TLSHelperA()

0x7ffef6e07d70 WTF::TLSHelperB::TLSHelperB()

0x27fae180b0 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest2()

0x27fae18060 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x27fae18088 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

End void WTF::TLSThreadTest2()

0x27fae180b0 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x7ffef6e07d70 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

0x27fae180b0 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x7ffef6e07d60 WTF::TLSHelperA::TLSHelperA()

0x27fae18060 WTF::WTFTLSHelperB::WTFTLSHelperB()

0x27fae18088 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest()

0x7ffef6e07d80 WTF::TLSHelperA::~TLSHelperA()

0x7ffef6e07d62 WTF::TLSHelperB::~TLSHelperB()

Start void WTF::TLSThreadTest()

End void WTF::TLSThreadTest()

0x7ffef6e07d60 WTF::TLSHelperA::~TLSHelperA()

0x27fae180b0 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

0x7ffef6e07e82 WTF::TLSHelperB::TLSHelperB()

0x7ffef6e07ea0 WTF::TLSHelperA::TLSHelperA()

0x7ffef6e07e90 WTF::TLSHelperB::TLSHelperB()

0x27fae18038 WTF::WTFTLSHelperA::WTFTLSHelperA()

End void WTF::TLSThreadTest2()

0x27fae18088 WTF::WTFTLSHelperA::~WTFTLSHelperA()

0x27fae18060 WTF::WTFTLSHelperB::~WTFTLSHelperB()

Start void WTF::TLSThreadTest2()

End void WTF::TLSThreadTest2()

0x27fae18038 WTF::WTFTLSHelperA::~WTFTLSHelperA()

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13chS9Q_xUCVJ%3D0jkq4sLbVsBYUqni50sY4E-GWNj4%3D-JA%40mail.gmail.com.

[Jeremy Roman]

OK. I'm a little frustrated that the C++ spec doesn't seem to explicitly spell out what happens in cases like this, but it sounds like this is safe at least for a simple POD like is_main_thread, and we can see what issues we run into in practice from there. If thakis or danakj agrees, I'm happy to approve a CL which moves it to allowed with use by special dispensation and adds a use for a simple such case. I'm reluctant to give carte-blanche for broader use until we have more experience with how this interacts with the various other TLS in Chromium (because it tickles so many weird platform/compiler-specific things), but I would like to see us ultimately use the language-level thread_local if we can, because it's pretty nice. :)

Thanks for the detailed investigation.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALQVofpGyD4LdqkaQE9Fb-8v3Rm2BBN4n2NYfr_4j7o7SJ8sLw%40mail.gmail.com.

[David Turner]

On Tue, Nov 28, 2017 at 11:08 PM, Jeremy Roman <jbr...@chromium.org> wrote:

OK. I'm a little frustrated that the C++ spec doesn't seem to explicitly spell out what happens in cases like this, but it sounds like this is safe at least for a simple POD like is_main_thread, and we can see what issues we run into in practice from there. If thakis or danakj agrees, I'm happy to approve a CL which moves it to allowed with use by special dispensation and adds a use for a simple such case. I'm reluctant to give carte-blanche for broader use until we have more experience with how this interacts with the various other TLS in Chromium (because it tickles so many weird platform/compiler-specific things), but I would like to see us ultimately use the language-level thread_local if we can, because it's pretty nice. :)

For the record, I beliieve that thread_local was broken Android before Android M :-( [1]. It is possible to work around this by using a more recent version of the Android NDK and /or libc++abi sources though.

- Digit

[1] https://github.com/android-ndk/ndk/issues/156 and more specifically this comment:

"""We can support trivial cases of thread_local, but thread_local with non-trivial destructors requires run time support that wasn't available until M (and I think the NDK's current libc++abi doesn't have the support either). There was actually a patch recently made to libc++abi adding support for this on platforms that don't have native support, so once that lands and we pick it up we could actually support that on older platforms."""

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACuR13d0FnS0Nga7EEEu%2BwdWciB1%2BRrUjzYQXTGvYdK88GZkSg%40mail.gmail.com.

[David Turner]

On Wed, Nov 29, 2017 at 11:05 AM, David Turner <di...@chromium.org> wrote:

On Tue, Nov 28, 2017 at 11:08 PM, Jeremy Roman <jbr...@chromium.org> wrote:

OK. I'm a little frustrated that the C++ spec doesn't seem to explicitly spell out what happens in cases like this, but it sounds like this is safe at least for a simple POD like is_main_thread, and we can see what issues we run into in practice from there. If thakis or danakj agrees, I'm happy to approve a CL which moves it to allowed with use by special dispensation and adds a use for a simple such case. I'm reluctant to give carte-blanche for broader use until we have more experience with how this interacts with the various other TLS in Chromium (because it tickles so many weird platform/compiler-specific things), but I would like to see us ultimately use the language-level thread_local if we can, because it's pretty nice. :)

For the record, I beliieve that thread_local was broken Android before Android M :-( [1]. It is possible to work around this by using a more recent version of the Android NDK and /or libc++abi sources though.

Good news!

I have briefly looked into this, and it looks like that we're already using a recent version of the Android NDK (r16 according to the git log), even though third_party/android_tools/ndk/README.chromium claimes it is only r12b (and lists a number of patches that may not apply anymore).

This means it should be ok to use thread_local on any Android version now. I'll look at the best way to update the README.chromium, but that's a separate issue.

Sorry for the mis-warning.

[Dmitry Skiba]

Hmm, I think r16 hasn't landed yet, see crbug.com/771171. +Brian who works on it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACnJMqqnp6nT-1iiV7oBHNXGO%2BkL6L2B3drGHjVHjXgqK43upQ%40mail.gmail.com.

[Brian Sheedy]

We're still on r12b. A Skia change went in recently that had a fix we needed for Chromium to compile with r16 (that's probably what you saw). The Chromium-side changes seem to all work, so barring any unexpected blockers, r16 will probably be in shortly after branch point (I would guess early next week?).

[David Turner]

On Wed, Nov 29, 2017 at 6:46 PM, Brian Sheedy <bsh...@google.com> wrote:

We're still on r12b. A Skia change went in recently that had a fix we needed for Chromium to compile with r16 (that's probably what you saw). The Chromium-side changes seem to all work, so barring any unexpected blockers, r16 will probably be in shortly after branch point (I would guess early next week?).

Thanks for the correction, my bad, I misread the git history for //third_party/, it is r12b at the moment indeed, though agrieve@ has an r16 branch on the chromium repository in preparation.

Brian, your patch looks promising, thanks a lot, can't wait for this to land.

[Peter Kasting]

On Mon, Jan 8, 2018 at 3:57 PM, <zer...@gmail.com> wrote:

thakis/danakj, want to comment on the above?  It seems like this thread is a bit in limbo right now, I'd like to see it move to a resolved place.

PK

In case it wasn't clear, that was me...

PK

[Jeremy Roman]

I believe Lucas has a patch to add a single use for IsMainThread, but AFAIK it hasn't landed yet. If that succeeds, we'll look at allowing more uses.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDK9Y5OuzoG%3DcnGeN%2BC5kYAj3CEms7SWGvPY%2BYuCx7TMA%40mail.gmail.com.

[Lucas Gadani]

Yes, that has been in the backburner for a while, I'll try to get it rebased and reviewed this week.

[vtsyrk...@google.com]

The underlying issue [1] blocking the CL noted below is fixed. I'm not sure if lfg@ is still interested in landing this patch, but if anyone else has a cross-platform use for thread_local it could be a good opportunity to give it a test before landing the style guide change.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=829078

[Dave Tapuska]

Regarding Lucas's patch... I think the whole CurrentThread should actually be rewritten in terms of base::PlatformThread::CurrentThreadId(). Blink may want to have a thread_local around that call but I think it can be more lightweight.

dave.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/54654263-36c5-46a9-9d73-788185208794%40chromium.org.

[Vlad Tsyrklevich]

I have no opinion how Blink should implement it, but I'll just note that the Linux implementation of PlatformThread::CurrentThreadID() already uses thread_local: https://cs.chromium.org/chromium/src/base/threading/platform_thread_posix.cc?type=cs&q=thread_local&g=0&l=149

[Dave Tapuska]

Yes I wrote that code... We needed to fix a perf problem with the swi becoming more expensive in a newer version of glibc. The swi code is duplicated in blink and the base primitive should just be used. If I get time I'll put together a pull request.

The guide should be updated separately if this is now fixed and is available for broad adoption.

dave.

[Lucas Gadani]

I'll try to get my patch rebased and reviewed again now that the Mac blocker is fixed.

Regarding rewriting WTF::CurrentThread, it can be done, but it should be orthogonal to my patch, which only moved the caching of the current thread to a thread_local instead of going through WTF::ThreadSpecific (that was just a perf improvement, since it avoids a jump when checking if we are on the main thread).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALz_-TYPChvLOzPkHfG947dO7vkZ6Km-nnbSwnvcRThTKwhYnA%40mail.gmail.com.

[Jeremy Roman]

I fiddled with this a little last week: https://chromium-review.googlesource.com/c/chromium/src/+/1434079

I got a little sidetracked trying to actually make the @plt stubs disappear (which I couldn't manage), because this seems like something that should be inlined (and with LTO, that should be rather easy to do), but it may be a win to just land it anyway.

[Jeremy Roman]

It appears that uses have snuck in so it appears that at least for simple uses to work. I'd be curious to see what the codegen actually looks like on our various build configs (and whether we need to tweak -ftls-model).

On an internal thread, +Egor Pasko and others looked into this in the context of some memory safety efforts in August; I'm not sure what the status of that is.

On Thu, Oct 1, 2020 at 6:53 AM Corentin Wallez <cwa...@chromium.org> wrote:

Has there been any updates on the experiments to use thread_local in Chromium? We're interested in using it in a third-party dependency of Chromium (Dawn), maybe that can be the experiment.

Cheers,

Corentin

[Corentin Wallez]

Has there been any updates on the experiments to use thread_local in Chromium? We're interested in using it in a third-party dependency of Chromium (Dawn), maybe that can be the experiment.

Cheers,

Corentin

On Monday, January 28, 2019 at 8:50:13 PM UTC+1 Jeremy Roman wrote:

[Benoit Lize]

Hi Corentin,

I've tried using it, faced issues, but did not dig much further afterwards to see where they were coming from:

- For simple objects, it seems to be working fine (as Jeremy pointed out, it is already used in the codebase in a couple places), and the codegen is what you expect, at least on desktop, that is very good (static offset from a register)

- For my use case, which had a destructor etc., I encountered weird failures on Windows 7 and Lollipop bots.

The issues may be down to me holding it wrong, but at least I think that for simple object (e.g. a POD one) it should work fine. I haven't checked the generated code on Android though, but Egor may have.

Cheers,

Benoit

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACuR13cbtMOmOGFe0pg%2B-vOLRHnjhr2FqiJ-PDTN6_B87J9JVQ%40mail.gmail.com.

[Corentin Wallez]

Hey Benoit, thanks for doing another test! In our case it is a POD so it's probably fine. It's quite good that it's just a static offset from a register and that we don't have to walk the task structure!

[Bin Wu]

Any update on this? I'd like to add a thread local raw pointer in net/third_party/quiche.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.