Closed Bug 1417107 Opened 2 years ago Closed 2 years ago

Switch to add "(unlabeled)" instead of "(labeled)" in SchedulerGroup::Runnable::GetName

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: farre, Assigned: farre)

Details

Attachments

(1 file, 1 obsolete file)

We really don't want to build dynamic strings for the default case.

It is also a good idea to move this out of GetName to nsThread, since then we can also only mark unlabeled runnables that are also not dispatched to the idle queue or the high priority queue.
Assignee: nobody → afarre
Priority: -- → P3
Comment on attachment 8928291 [details] [diff] [review]
0001-Bug-1417107-Use-histogram-key-suffix-unlabeled-inste.patch

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

I think the idea makes total sense--we want to do work in the uncommon case, not the common case!  Punting this review to Eric, though.
Attachment #8928291 - Flags: review?(nfroyd) → review?(erahm)
Comment on attachment 8928291 [details] [diff] [review]
0001-Bug-1417107-Use-histogram-key-suffix-unlabeled-inste.patch

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

Makes sense, just two questions:

1) Should this name only change for the `timer` or everything that uses `name`?
2) Is it okay that "This changes values gathered for telemetry!"? I guess it probably is since you're adding yourself as a triager, but I'm not sure if that's going to cause a slew of telemetry warnings.

::: xpcom/threads/SchedulerGroup.cpp
@@ -377,5 @@
>    if (aName.IsEmpty()) {
>      aName.AssignLiteral("anonymous");
>    }
>  
> -  aName.AppendASCII("(labeled)");

My only concern here is you're changing all the existing runnable names. Is this going to cause some sort of telemetry meltdown if what used to be "Foo(labeled)" is now just "Foo"?

::: xpcom/threads/nsThread.cpp
@@ +987,5 @@
>  
>          if (MAIN_THREAD == mIsMainThread) {
> +          if (!labeled && priority == EventPriority::Normal) {
> +            nsAutoCString unlabeled = name;
> +            unlabeled.AppendASCII("(unlabeled)");

It's slightly more efficient if you just do:

> nsCString unlabeled = name + NS_LITERAL_CSTRING("(unlabeled)");
> // or maybe
> time.emplace(nsCString(name + ...));

I'm not 100% how this is supposed to work, but `name` is used below for the `idleTimer` as well, my guess is you want to use the 'unlabled' variant there as well.

It's also copied into `sMainThreadRunnableName` as well, my guess is the same goes there.

All that said I think what you want is something like:

>   if (MAIN_THREAD == ...) {
>     if (!labeled && ...) {
>       name.AppendLiteral("(unlabled)");
>   }
>   timer.emplace(name);
Attachment #8928291 - Flags: review?(erahm) → feedback+
(In reply to (Out until Dec 5) Eric Rahm [:erahm] (please no mozreview requests) from comment #3)
> Comment on attachment 8928291 [details] [diff] [review]
> 0001-Bug-1417107-Use-histogram-key-suffix-unlabeled-inste.patch
> 
> Review of attachment 8928291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense, just two questions:
> 
> 1) Should this name only change for the `timer` or everything that uses
> `name`?
> 2) Is it okay that "This changes values gathered for telemetry!"? I guess it
> probably is since you're adding yourself as a triager, but I'm not sure if
> that's going to cause a slew of telemetry warnings.

So I tried to make sure that it was for telemetry only, since I initially thought that it'd be used for that only, but you're probably right. It would be useful for debugging the scheduler to see if the runnable is labeled or not. I'll re-arrange.

Since this is used as keys in keyed histograms it /shouldn't/ cause trouble if I understand it correctly. No more trouble than if a new runnable with a new unique name pops up, or if a previously unlabeled runnable becomes labeled. 

> ::: xpcom/threads/SchedulerGroup.cpp
> @@ -377,5 @@
> >    if (aName.IsEmpty()) {
> >      aName.AssignLiteral("anonymous");
> >    }
> >  
> > -  aName.AppendASCII("(labeled)");
> 
> My only concern here is you're changing all the existing runnable names. Is
> this going to cause some sort of telemetry meltdown if what used to be
> "Foo(labeled)" is now just "Foo"?

See above. Shouldn't really.

> ::: xpcom/threads/nsThread.cpp
> @@ +987,5 @@
> >  
> >          if (MAIN_THREAD == mIsMainThread) {
> > +          if (!labeled && priority == EventPriority::Normal) {
> > +            nsAutoCString unlabeled = name;
> > +            unlabeled.AppendASCII("(unlabeled)");
> 
> It's slightly more efficient if you just do:
> 
> > nsCString unlabeled = name + NS_LITERAL_CSTRING("(unlabeled)");
> > // or maybe
> > time.emplace(nsCString(name + ...));

I'll make it so.

> I'm not 100% how this is supposed to work, but `name` is used below for the
> `idleTimer` as well, my guess is you want to use the 'unlabled' variant
> there as well.
> 
> It's also copied into `sMainThreadRunnableName` as well, my guess is the
> same goes there.
> 
> All that said I think what you want is something like:
> 
> >   if (MAIN_THREAD == ...) {
> >     if (!labeled && ...) {
> >       name.AppendLiteral("(unlabled)");
> >   }
> >   timer.emplace(name);

The reason for going to such lengths to only mark normal priority runnables with the unlabeled tag is that I wanted it to coincide with the probe for TIME_BETWEEN_UNLABELED_RUNNABLES_MS. That is, if we're only caring about unlabeled runnables on the normal priority event queue, then it felt reasonable to only mark those. That way, when examining the data, we would get precisely those runnables that the cooperative scheduler cares about.

I realise that I'm unsure of why the scheduler wouldn't want to potentially interrupt idle callbacks. But in that case, the probe for TIME_BETWEEN_UNLABELED_RUNNABLES_MS should be extended to cover normal or idle priority. Bill, am I not understanding something essential here?

Maybe it would be an idea to extend it to also display the priority of the unlabeled?

And I'll make sure to actually spell unlabeled correctly :)
Flags: needinfo?(wmccloskey)
Moved appending the suffix to GetRunnableName, and made it so that all runnables with a priority lower than input gets the suffix.

ni myself to ask for review when Eric is back.
Attachment #8928291 - Attachment is obsolete: true
Attachment #8933322 - Flags: review?(erahm)
Comment on attachment 8933322 [details] [diff] [review]
0001-Bug-1417107-Use-histogram-key-suffix-unlabeled-inste.patch

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

Thanks, looks good.

::: xpcom/threads/nsThread.cpp
@@ +901,5 @@
>      aName.AssignLiteral("anonymous runnable");
>    }
>  
> +  if (!labeled && aPriority > EventPriority::Input) {
> +    aName.AppendASCII("(unlabeled)");

Please use `AppendLiteral`.
Attachment #8933322 - Flags: review?(erahm) → review+
TIME_BETWEEN_UNLABELED_RUNNABLES_MS should indeed also include unlabeled idle runnables. How important this probe will be in the light of not using preemption is not clear, but to begin with it would probably be good if idle runnables were included.
Flags: needinfo?(wmccloskey)
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2cfc8012f1
Use histogram key suffix 'unlabeled' instead of 'labeled'. r=erahm
https://hg.mozilla.org/mozilla-central/rev/4f2cfc8012f1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.