Closed Bug 1399031 Opened 2 years ago Closed 2 years ago

Use mozilla/ThreadLocal.h in mozjemalloc

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files)

mozjemalloc is using adhoc code on each platform, and that makes adding new uses of the thread local variables it uses painful. But we have mozilla/ThreadLocal.h that provides such convenience, so we can use it. However, mozjemalloc being a low-level thing, it has requirements that are different from the typical users of ThreadLocal, so we first need to make ThreadLocal a little more friendly to it.
Comment on attachment 8906970 [details]
Bug 1399031 - Expose pthread_{get,set}specific-based TLS whether "native" TLS is supported or not.

https://reviewboard.mozilla.org/r/178726/#review183812

::: mfbt/ThreadLocal.h:76
(Diff revision 1)
>  template<typename T>
> -class ThreadLocal
> +class ThreadLocalKeyTrait

I have a slight preference for calling this something like `KeyStorage`...WDYT?

::: mfbt/ThreadLocal.h:125
(Diff revision 1)
> +  bool mInited;
> +};
>  #endif
> +
> +template<typename T>
> +class ThreadLocalNativeTrait

...and then this would be `NativeStorage` or `RuntimeStorage` or somesuch.

::: mfbt/ThreadLocal.h:150
(Diff revision 1)
> +template<typename T, typename Trait>
> +class ThreadLocal: public Trait

...and we'd call this `Storage` instead of `Trait`.
Attachment #8906970 - Flags: review?(nfroyd) → review+
Comment on attachment 8906971 [details]
Bug 1399031 - Add a Tls{Get,Set}Value-based ThreadLocal implementation for Windows.

https://reviewboard.mozilla.org/r/178728/#review183814

Seems reasonable, but I think we should change a few things.  Especially interested to hear if you have thoughts on the refactoring proposed below.

::: mfbt/ThreadLocal.h:93
(Diff revision 1)
> +template<typename T>
> +class ThreadLocalKeyTrait

A comment on why we have this when our Windows platforms support `__thread` or the equivalent perfectly well would be good, so people can understand why we're not using this below.

::: mfbt/ThreadLocal.h:181
(Diff revision 1)
>  
>  private:
>    T mValue;
>  };
>  
> -template<typename T, typename Trait>
> +template<typename T, typename Trait = ThreadLocalKeyTrait<T>>

See comment below about not defaulting to something here.

::: mfbt/ThreadLocal.h:239
(Diff revision 1)
>  #define MOZ_THREAD_LOCAL(TYPE) thread_local mozilla::detail::ThreadLocal<TYPE, mozilla::detail::ThreadLocalNativeTrait<TYPE>>
>  #else
>  #define MOZ_THREAD_LOCAL(TYPE) __thread mozilla::detail::ThreadLocal<TYPE, mozilla::detail::ThreadLocalNativeTrait<TYPE>>
>  #endif
>  #else
> -#define MOZ_THREAD_LOCAL(TYPE) mozilla::detail::ThreadLocal<TYPE, mozilla::detail::ThreadLocalKeyTrait<TYPE>>
> +#define MOZ_THREAD_LOCAL(TYPE) mozilla::detail::ThreadLocal<TYPE>

I almost think we should have a Windows-only check that `MOZ_THREAD_LOCAL` uses `thread_local` storage, rather than the `Tls*`-style storage, but that seems a little awkward.

Maybe we should refactor this to:

```
#ifdef MOZ_HAS_THREAD_LOCAL
#define MOZ_TLS_STYLE(TYPE) ::mozilla::detail::NativeStorage<TYPE>
#else
#define MOZ_TLS_STYLE(TYPE) ::mozilla::detail::KeyStorage<TYPE>
#endif

// "storage specifier" is standardese.
#ifdef MOZ_HAS_THREAD_LOCAL
#if Windows or thread_local
#define MOZ_TLS_STORAGE_SPECIFIER thread_local
#else
#define MOZ_TLS_STORAGE_SPECIFIER __thread
#endif
#else
#define MOZ_TLS_STORAGE_SPECIFIER /* nothing */
#endif

#define MOZ_THREAD_LOCAL(TYPE) \
  MOZ_TLS_STORAGE_SPECIFIER \
  ::mozilla::detail::ThreadLocal<TYPE, MOZ_TLS_STYLE(TYPE)>
```

?  Separating things out makes the code easier to follow, IMHO, and makes it harder to inadvertently switch Windows to the key-based storage version.  Then we can also switch `ThreadLocal` away from defaulting on the storage kind, which seems like a good thing.
Attachment #8906971 - Flags: review?(nfroyd)
Comment on attachment 8906973 [details]
Bug 1399031 - Use mozilla/ThreadLocal.h in mozjemalloc.

https://reviewboard.mozilla.org/r/178732/#review183828

This is much nicer.

::: memory/build/mozjemalloc.cpp:958
(Diff revision 1)
>  /*
> - * Map of pthread_self() --> arenas[???], used for selecting an arena to use
> + * The arena associated with the current thread (per jemalloc_thread_local_arena)
> - * for allocations.
>   */
>  #if !defined(XP_WIN) && !defined(XP_DARWIN)
> -static __thread arena_t	*arenas_map;
> +static MOZ_THREAD_LOCAL(arena_t*) thread_arena;
> +#else
> +static mozilla::detail::ThreadLocal<arena_t*> thread_arena;
>  #endif

A comment here explaining why Windows and Darwin get to use a different thread local implementation would be a good thing.  It's not obvious to me (their runtime-based TLS calls malloc, maybe?).
(In reply to Nathan Froyd [:froydnj] from comment #6)
> ?  Separating things out makes the code easier to follow, IMHO, and makes it
> harder to inadvertently switch Windows to the key-based storage version. 
> Then we can also switch `ThreadLocal` away from defaulting on the storage
> kind, which seems like a good thing.

Since the refactor removed all the in-code #ifdefs, something simpler would be to remove MOZ_HAS_THREAD_LOCAL and inline it. Something like:

#if defined(XP_WIN) || defined(MACOSX_HAS_THREAD_LOCAL)
#define MOZ_THREAD_LOCAL(TYPE) thread_local mozilla::detail::ThreadLocal<TYPE, mozilla::detail::ThreadLocalNativeTrait<TYPE>>
#elif defined(HAVE_THREAD_TLS_KEYWORD)
#define MOZ_THREAD_LOCAL(TYPE) __thread mozilla::detail::ThreadLocal<TYPE, mozilla::detail::ThreadLocalNativeTrait<TYPE>>
#else
#define MOZ_THREAD_LOCAL(TYPE) mozilla::detail::ThreadLocal<TYPE>
#endif

Thoughts?
Comment on attachment 8906972 [details]
Bug 1399031 - Remove the setting of NO_TLS when PIC is not defined in mozjemalloc: it's always defined.

https://reviewboard.mozilla.org/r/178730/#review184178
Attachment #8906972 - Flags: review?(n.nethercote) → review+
Comment on attachment 8906973 [details]
Bug 1399031 - Use mozilla/ThreadLocal.h in mozjemalloc.

https://reviewboard.mozilla.org/r/178732/#review184180

r=me. Bonus points if you invert the sense of NO_TLS and get rid of all the `#ifndef NO_TLS` double negatives.
Attachment #8906973 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #10)
> Comment on attachment 8906973 [details]
> Bug 1399031 - Use mozilla/ThreadLocal.h in mozjemalloc.
> 
> https://reviewboard.mozilla.org/r/178732/#review184180
> 
> r=me. Bonus points if you invert the sense of NO_TLS and get rid of all the
> `#ifndef NO_TLS` double negatives.

I'll skip this, considering we'll eventually have to disable NO_TLS for stylo on android.
Attachment #8906970 - Flags: review+ → review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #7)
> A comment here explaining why Windows and Darwin get to use a different
> thread local implementation would be a good thing.  It's not obvious to me
> (their runtime-based TLS calls malloc, maybe?).

Added a comment. Note I /think/ we could live with native thread local on Windows (and that the only reason for not using it was Windows XP). But at this point I don't want to change any behavior. We can try things in followups.
Attachment #8906970 - Flags: review?(nfroyd)
(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> > ?  Separating things out makes the code easier to follow, IMHO, and makes it
> > harder to inadvertently switch Windows to the key-based storage version. 
> > Then we can also switch `ThreadLocal` away from defaulting on the storage
> > kind, which seems like a good thing.
> 
> Since the refactor removed all the in-code #ifdefs, something simpler would
> be to remove MOZ_HAS_THREAD_LOCAL and inline it. Something like:
> 
> #if defined(XP_WIN) || defined(MACOSX_HAS_THREAD_LOCAL)
> #define MOZ_THREAD_LOCAL(TYPE) thread_local
> mozilla::detail::ThreadLocal<TYPE,
> mozilla::detail::ThreadLocalNativeTrait<TYPE>>
> #elif defined(HAVE_THREAD_TLS_KEYWORD)
> #define MOZ_THREAD_LOCAL(TYPE) __thread mozilla::detail::ThreadLocal<TYPE,
> mozilla::detail::ThreadLocalNativeTrait<TYPE>>
> #else
> #define MOZ_THREAD_LOCAL(TYPE) mozilla::detail::ThreadLocal<TYPE>
> #endif

This is simpler, yes.  I still think default template arguments are not great here, but I suppose having them makes the mozjemalloc use not have to dive quite so deeply into mozilla::detail.
Comment on attachment 8906970 [details]
Bug 1399031 - Expose pthread_{get,set}specific-based TLS whether "native" TLS is supported or not.

https://reviewboard.mozilla.org/r/178726/#review184388
Attachment #8906970 - Flags: review?(nfroyd) → review+
Comment on attachment 8906971 [details]
Bug 1399031 - Add a Tls{Get,Set}Value-based ThreadLocal implementation for Windows.

https://reviewboard.mozilla.org/r/178728/#review184390
Attachment #8906971 - Flags: review?(nfroyd) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/a6e26c4c5704
Expose pthread_{get,set}specific-based TLS whether "native" TLS is supported or not. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/47efe55def9e
Add a Tls{Get,Set}Value-based ThreadLocal implementation for Windows. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/3025f5550677
Remove the setting of NO_TLS when PIC is not defined in mozjemalloc: it's always defined. r=njn
https://hg.mozilla.org/integration/autoland/rev/0eb129729d90
Use mozilla/ThreadLocal.h in mozjemalloc. r=njn
You need to log in before you can comment on or make changes to this bug.