1378930 - make SchedulerGroup dispatch smarter when dealing with nsINamed runnables

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

I noticed this when starting to label things through SystemGroup, but I think the problem extends to all SchedulerGroups.

Virtually all of our runnables inherit from mozilla::Runnable, which also implements nsINamed.  If we're passing such a runnable into SystemGroup::Dispatch, we're forced to provide a name for it...which is silly for a couple of reasons:

1) It's duplicate information.
2) We'll eventually wind down into SchedulerGroup::UnlabeledDispatch, which does an unnecessary QI to nsINamed and sets the name, which is unnecessary overhead, because this runnable probably already has a name.

We should be providing some sort of faster path for things that we know already implement nsINamed, or perhaps just a faster path for mozilla::Runnable, since we know those things already have sensible names.

Does that sound reasonable, Bill?

Tenatively marking this as [qf], because it could impact perf measurements on Nightly, and it would probably make things slightly faster on release regardless.

Comment 1

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

Yes, I think this was a mistake in the design of the API. I think we should remove the name argument from the Dispatch method and just provide runnable names through nsINamed::GetName. Then we could remove the nsINamed::SetName method.

Comment 2

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

It occurred to me that we could avoid the runtime cost by just ignoring the name that's passed into Dispatch. Perhaps in DEBUG builds we could assert that there already is a name. But that would be a separate change from actually removing the name argument.

Comment 3

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

Attachment: (v1) Part 1: Remove nsINamed::SetName()

Remove setName from nsINamed.idl.
Note: SetName() is still available in Runnable class for NewXxxRunnableMethod() to set name internally.

Comment 4

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

Attachment: (v2) Part 2: Remove the aName parameter from SchedulerGroup/DocGroup/DispatcherTrait

Comment 5

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

Comment on attachment 8890261 [details] [diff] [review]
(v1) Part 1: Remove nsINamed::SetName()

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

::: xpcom/threads/nsThreadUtils.h
@@ +432,4 @@
>    explicit Runnable(const char* aName) : mName(aName) {}
>  #endif
>  
> +  nsresult SetName(const char* aName);

I think this should now be unused. Please remove it and the definition in the .cpp file.

Comment 6

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

Comment on attachment 8890262 [details] [diff] [review]
(v2) Part 2: Remove the aName parameter from SchedulerGroup/DocGroup/DispatcherTrait

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

Thanks! This is awesome!

::: layout/tables/nsTableFrame.cpp
@@ +4254,2 @@
>      printf("\n col frame cache ->");
> +     for (colIdx = 0; colIdx < numCols; colIdx++) {

This is still indented over too far :-).

Comment 7

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

(In reply to Bill McCloskey (:billm) from comment #6)
> ::: layout/tables/nsTableFrame.cpp
> @@ +4254,2 @@
> >      printf("\n col frame cache ->");
> > +     for (colIdx = 0; colIdx < numCols; colIdx++) {
> 
> This is still indented over too far :-).

Oops! My bad. 
My editor automatically changed the indentation in some of this files before saving. I'll update them before landing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bc36c3b43c
Part 1: Remove nsINamed::SetName(). r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/0acc77eb92c6
Part 2: Remove the aName parameter from SchedulerGroup/DocGroup/DispatcherTrait. r=billm

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6bc36c3b43c
https://hg.mozilla.org/mozilla-central/rev/0acc77eb92c6