1304891 - Modify MSCOM code to use thread-local MainThreadInvoker objects

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Another area where we see latency in the MainThreadHandoff stuff is with the construction and destruction of MainThreadInvoker objects.

Every time we do that, we create and destroy an event object. Thinking more about the use case for the MainThreadInvoker, it makes sense for each thread to have access to its own persistent MainThreadInvoker object.

I have some patches that do the following:

1) Create a GetThreadLocalObject<T>() that lazily creates and returns a thread-local instance of type T that is automatically destroyed when the thread terminates.

2) Modify the mscom stuff to use that for MainThreadInvoker instead of constructing it directly.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - ThreadLocalObject<T>

This part implements the ThreadLocalObject<T> function.

I am leaving it in ipc/mscom for now with pthreads "roughed-in." I'd like to hold off on exposing it as fully cross-platform until I know for certain that it will work correctly on all of our tier-1 platforms.

One use case that I intend this function to be useful for is the case where it is being called by a thread whose lifetime we do not control (for example, the RPC thread pool that is implicitly started by Microsoft COM). It is imperative that the cleanup functionality that is exposed by this function works correctly in such contexts.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Integrate ThreadLocalObject into other mscom code

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8796280 [details] [diff] [review]
Part 1 - ThreadLocalObject<T>

Review of attachment 8796280 [details] [diff] [review]:
-----------------------------------------------------------------

The high-order bit here is that ThreadLocalObject.* is pretty hairy: we've got type erasure, specialization of the guts of nsTArray, magic statics (which, FWIW, we have turned off right now because of crashes in Windows XP:

http://dxr.mozilla.org/mozilla-central/source/old-configure.in#303

), etc. etc.  The user-land TLS slot implementation strikes me as a bit of premature optimization as well: we have one use of this stuff, so all the array bits are needless complexity at this point.  It's not immediately clear that a given class is necessarily only instantiable once on a given thread, just from looking at the class declaration.

My counter-proposal: I think it should be possible to write something like:

// PerThreadInstance, SingletonPerThread, not sure what the name should be.
class MainThreadInvoker : public PerThreadInstance<MainThreadInvoker>
{
private:
  friend class PerThreadInstance<MainThreadInvoker>;

  // It would be nice if we could statically enforce that these methods were private
  // but I don't think we have any place to automagically generate a static_assert...
  // unless we can do it in one of PerThreadInstance's methods, somehow?
  MainThreadInvoker();
  ~MainThreadInvoker();

public:
  // Various methods and such.

  using PerThreadInstance<MainThreadInvoker>::GetForThread;

private:
  // Internal implementation details.
};

// I think some (all?) of PerThreadInstance's methods would have to be defined outside of the class
// definition to make things work.
template<typename T>
class PerThreadInstance /* : public PlatformSpecificMachinery possibly needed? */
{
protected:
  PerThreadInstance() = default;
  ~PerThreadInstance() = default;

  T* GetForThread();

  static void DeleteThisInstance(void*);
};

template<typename T>
MOZ_NEVER_INLINE T*
PerThreadInstance<T>::GetForThread()
{
  // static_assert T's non-destructibility, like we do here:
  // http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#32

  // Ensure the TLS slot is set up.

  // If we already have an instance in the TLS slot, return that.

  // Create instance of type T.
  T* instance = new T();

  // Stuff instance into the TLS slot.  The TLS machinery now owns the pointer.
  // Register the pointer to be destroyed at thread exit.

  // Return the raw pointer.  We don't need to play NonDeletable tricks, because we're
  // assured that T's destructor isn't publicly accessible, above.
  return instance.
}

I *think* this works, but I haven't really sat down with the compiler + test cases to make sure that it does.  It seems like a fairly straightforward translation of what you already have.  Having a class like this gives us a nice place to hang some documentation off of: why we have this class in addition to MOZ_THREAD_LOCAL, examples of use, etc. etc.

WDYT?  Did you try this already and it fell apart in flames because of something I haven't thought of?

Canceling review for now, but I am open to discussion. Please push back!

The testing seems pretty reasonable; I didn't go over the core of the test with a fine-toothed comb, but it's about what I would expect from a test for this stuff.

::: ipc/mscom/ThreadLocalObject.h
@@ +55,5 @@
> + * to an instance of a deleter that is specialized on the original type of the
> + * object.
> + *
> + * We use std::unique_ptr instead of mozilla::UniquePtr because the latter does
> + * not support void* (due to its operator* method).

I tried removing UniquePtr::operator* and wound up in a world of hurt. :(

::: ipc/mscom/test/TestThreadLocalObject.cpp
@@ +14,5 @@
> +#include "TestHarness.h"
> +
> +#include "../ThreadLocalObject.cpp"
> +
> +class WrappedMonitor

We can't use mozilla::{Reentrant,}Monitor here because...oh, because this is external to libxul, and we don't have those types available?

::: ipc/mscom/test/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +GeckoCppUnitTests([
> +    'TestThreadLocalObject',

If you're going to do this rather than a gtest--gtests are preferred--then you need to add this test to the testing/cppunittest.ini file.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

I have made some changes to MainThreadInvoker such that this bug is no longer necessary.