1333997 - Define named-versioned NewRunnableMethod for better debugging

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 1: Add Type Aliases for NewRunnable Templates.

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.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 2: Overload the NewRunnable APIs with a 'Name' as 1st Parameter.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 3: Add Test Coverage for Named NewRunnable APIs.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

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 6

[Nathan Froyd [:froydnj]]

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 7

[Nathan Froyd [:froydnj]]

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 8

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 9

[Nathan Froyd [:froydnj]]

(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.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(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?

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 2: Overload the NewRunnable APIs with a 'Name' as 1st Parameter.

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

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: 1333997_part3.patch

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.)

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 14

[Nathan Froyd [:froydnj]]

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 15

[Nathan Froyd [:froydnj]]

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...

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Carsten Book [:Tomcat]]

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