make SchedulerGroup dispatch smarter when dealing with nsINamed runnables

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: froydnj, Assigned: bevis)

Tracking

(Blocks 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(2 attachments)

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 btseng@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c6bc36c3b43c
https://hg.mozilla.org/mozilla-central/rev/0acc77eb92c6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.