Closed Bug 1378930 Opened 6 years ago Closed 6 years ago

make SchedulerGroup dispatch smarter when dealing with nsINamed runnables


(Core :: XPCOM, enhancement)

Not set



Performance Impact low
Tracking Status
firefox56 --- fixed


(Reporter: froydnj, Assigned: bevis)


(Blocks 1 open bug)



(2 files)

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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
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.
Whiteboard: [qf] → [qf:p3]
Assignee: nobody → btseng
Blocks: 1382172
Remove setName from nsINamed.idl.
Note: SetName() is still available in Runnable class for NewXxxRunnableMethod() to set name internally.
Attachment #8890261 - Flags: review?(wmccloskey)
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.
Attachment #8890261 - Flags: review?(wmccloskey) → review+
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 :-).
Attachment #8890262 - Flags: review?(wmccloskey) → review+
(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.
Pushed by
Part 1: Remove nsINamed::SetName(). r=billm
Part 2: Remove the aName parameter from SchedulerGroup/DocGroup/DispatcherTrait. r=billm
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in before you can comment on or make changes to this bug.