As mentioned in bug 1321812 comment 8, it makes sense to have runnable named at creation using NewRunnableMethod related APIs.

Add type aliases for the runnable templates used by NS_NewRunnableFunction and NewRunnableMethod. This makes the function overloading of {NS_NewRunnableFunction, NewRunnableMethod}(const char* aName, ...) in patch part 2 easier.

Wait for treeherder result complete: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39ca6fdf79dbb4b9ae045c2a0697e94d8d3f4b70

Comment on attachment 8834328 [details] [diff] [review] (v1) Part 1: Add Type Aliases for NewRunnable Templates. I'd like to overload NS_NewRunnableFunction, NewRunnableMethod with a 'Name' as 1st parameter of these APIs. Patch part 1 is to have type aliases for the templates used by these APIs to make the new overloading functions of patch part2 defined in an easier way. Hi Nathan, May I have your review for these change? Thanks!

Comment on attachment 8834337 [details] [diff] [review] (v1) Part 2: Overload the NewRunnable APIs with a 'Name' as 1st Parameter. Review of attachment 8834337 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review while we get the implementation details worked out. ::: xpcom/threads/nsThreadUtils.h @@ +329,5 @@ > +inline already_AddRefed<T> > +AddRefAndSetName(T* aObj, const char* aName) > +{ > + RefPtr<T> ref(aObj); > + if (aName) { Why would people be passing a null name here? I think we should just disallow null names completely, and if people really do need a null name, then they can use the unnamed variant of the *NewRunnable* overloads. @@ +440,5 @@ > +{ > + // We store a non-reference in RunnableFunction, but still forward aFunction > + // to move if possible. > + return AddRefAndSetName(new mozilla::detail::RunnableFunctionImpl<Function> > + (mozilla::Forward<Function>(aFunction)), aName); How about just: return detail::SetRunnableName(NS_NewRunnableFunction(...), aName); and so on for all the rest of the functions you have added here?

Comment on attachment 8834338 [details] [diff] [review] (v1) Part 3: Add Test Coverage for Named NewRunnable APIs. Review of attachment 8834338 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review, since this will probably need changes after part 2 gets changed. ::: xpcom/tests/gtest/TestThreadUtils.cpp @@ +167,5 @@ > +static void ExpectRunnableName(Runnable* aRunnable, const char* aExpectedName) > +{ > + nsAutoCString name; > + EXPECT_TRUE(NS_SUCCEEDED(aRunnable->GetName(name))) << "Runnable::GetName()"; > +#ifdef RELEASE_OR_BETA This seems like a peculiar restriction. Why are we doing this? @@ +184,5 @@ > nsCOMPtr<nsIRunnable> trackedRunnable; > { > TestCopyWithNoMove tracker(&copyCounter); > + trackedRunnable = aNamed ? NS_NewRunnableFunction(nullptr, tracker) : > + NS_NewRunnableFunction(tracker); I guess the |aNamed| parameter here is attempting to verify that the named variants produce runnables that behave identical to their non-named counterparts?

Comment on attachment 8834338 [details] [diff] [review] (v1) Part 3: Add Test Coverage for Named NewRunnable APIs. Review of attachment 8834338 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/tests/gtest/TestThreadUtils.cpp @@ +167,5 @@ > +static void ExpectRunnableName(Runnable* aRunnable, const char* aExpectedName) > +{ > + nsAutoCString name; > + EXPECT_TRUE(NS_SUCCEEDED(aRunnable->GetName(name))) << "Runnable::GetName()"; > +#ifdef RELEASE_OR_BETA It's because that the 'name' will always be truncated by Runnable::GetName() in RELEASE_OR_BETA build according to http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/xpcom/threads/nsThreadUtils.cpp#50-61 @@ +184,5 @@ > nsCOMPtr<nsIRunnable> trackedRunnable; > { > TestCopyWithNoMove tracker(&copyCounter); > + trackedRunnable = aNamed ? NS_NewRunnableFunction(nullptr, tracker) : > + NS_NewRunnableFunction(tracker); Correct!

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8) > ::: xpcom/tests/gtest/TestThreadUtils.cpp > @@ +167,5 @@ > > +static void ExpectRunnableName(Runnable* aRunnable, const char* aExpectedName) > > +{ > > + nsAutoCString name; > > + EXPECT_TRUE(NS_SUCCEEDED(aRunnable->GetName(name))) << "Runnable::GetName()"; > > +#ifdef RELEASE_OR_BETA > > It's because that the 'name' will always be truncated by Runnable::GetName() > in RELEASE_OR_BETA build according to > http://searchfox.org/mozilla-central/rev/ > 672c83ed65da286b68be1d02799c35fdd14d0134/xpcom/threads/nsThreadUtils.cpp#50- > 61 OK, but why would we do *that*? It'd be much better for release/beta to be just as introspectable as our nightly builds, and that code seems like the wrong way to go for introspection purposes.

(In reply to Nathan Froyd [:froydnj] from comment #9) > (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8) > > ::: xpcom/tests/gtest/TestThreadUtils.cpp > > @@ +167,5 @@ > > > +static void ExpectRunnableName(Runnable* aRunnable, const char* aExpectedName) > > > +{ > > > + nsAutoCString name; > > > + EXPECT_TRUE(NS_SUCCEEDED(aRunnable->GetName(name))) << "Runnable::GetName()"; > > > +#ifdef RELEASE_OR_BETA > > > > It's because that the 'name' will always be truncated by Runnable::GetName() > > in RELEASE_OR_BETA build according to > > http://searchfox.org/mozilla-central/rev/ > > 672c83ed65da286b68be1d02799c35fdd14d0134/xpcom/threads/nsThreadUtils.cpp#50- > > 61 > > OK, but why would we do *that*? It'd be much better for release/beta to be > just as introspectable as our nightly builds, and that code seems like the > wrong way to go for introspection purposes. In release/beta build, my concerns are: - This increases a char* size per runnable in memory footprint. (Currently, the Runnable::mName is defined only in RELEASE_OR_BETA as well.) - It will become a performance concern in nsThread::ProcessNextEvent(): http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/xpcom/threads/nsThread.cpp#1241-1259 NI :billm, will we analyze more on release/beta builds?

Address the advice in comment 6. (Thanks for these better advice, btw.)

1. Add non-null string for testing according to the change in part 2. 2. Add some explanation of the tests. 3. Keep the RELEASE_OR_BETA session because the Runnable::mName is only available in RELEASE_OR_BETA as well. (We could remove these if the introspection of the runnable name in RELEASE_OR_BETA is needed in the future.)

The reasons Bevis gave are the ones we used when deciding to disable the naming on beta/release. It's possible that we'll enable this stuff on those branches later on, as needed. In any case, I think it makes sense for the code to be prepared for there to be no name available. However, I don't see why the name needs to be null. "" would be fine too. Maybe I don't understand how this fits together though.

Comment on attachment 8835291 [details] [diff] [review] (v2) Part 2: Overload the NewRunnable APIs with a 'Name' as 1st Parameter. Review of attachment 8835291 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8835300 [details] [diff] [review] 1333997_part3.patch Review of attachment 8835300 [details] [diff] [review]: ----------------------------------------------------------------- The RELEASE_OR_BETA thing is a little weird, but I think not wanting the overhead in the core thread dispatch code is the correct tradeoff. I was thinking about exposing the runnable names to the profiler, but maybe that would be too much additional overhead...

Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/afb92a28b0a9 Part 1: Add Type Aliases for NewRunnable Templates. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/353b10b5959c Part 2: Overload the NewRunnable APIs with a 'Name' as 1st Parameter. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/8f45843772d1 Part 3: Add Test Coverage for Named NewRunnable APIs. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/afb92a28b0a9 https://hg.mozilla.org/mozilla-central/rev/353b10b5959c https://hg.mozilla.org/mozilla-central/rev/8f45843772d1