label AvailableRunnable

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Recent telemetry has this runnable in the #7 spot:

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLQuery.cpp?q=%22AvailableRunnable%22&redirect_type=direct#20

It doesn't look too hard to redirect this event into the correct queue; I think for offscreen canvases, we'll have to just accept that it's going in the SystemGroup, though.
(Assignee)

Updated

a year ago
No longer blocks: 1321812
(Assignee)

Updated

a year ago
Blocks: 1321812

Updated

a year ago
Whiteboard: [gfx-noted]
(Assignee)

Comment 1

a year ago
Created attachment 8890040 [details] [diff] [review]
part 1 - centralize AvailableRunnable dispatching

This change will make labeling AvailableRunnable simpler, as we'll only
have to modify one location.
Attachment #8890040 - Flags: review?(jgilbert)
(Assignee)

Comment 2

a year ago
Created attachment 8890042 [details] [diff] [review]
part 2 - label WebGLQuery's AvailableRunnable

If we have an associated canvas element, the query should go in the
queue of the associated document.  If not, the system group should be
fine, as the query can't touch content.

I'm unsure of this justification because this is code that I'm not very
familiar with...Jeff, in what circumstances do we have a WebGLContext that
doesn't have an associated canvas element?  Can we use WebGL in a worker...?
If so, is there an associated global/document that we could attach ourselves
to?
Attachment #8890042 - Flags: review?(wmccloskey)
Attachment #8890042 - Flags: review?(jgilbert)
Comment on attachment 8890042 [details] [diff] [review]
part 2 - label WebGLQuery's AvailableRunnable

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

::: dom/canvas/WebGLQuery.cpp
@@ +63,5 @@
>  
>  void
>  WebGLQuery::DispatchAvailableRunnable()
>  {
> +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);

Is there any reason to do this instead of just using new?

@@ +70,5 @@
> +        canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other,
> +                                     runnable.forget());
> +        return;
> +    }
> +    SystemGroup::Dispatch("AvailableRunnable", TaskCategory::Other,

I would feel safer doing a normal (unlabeled) dispatch in this case.
Attachment #8890042 - Flags: review?(wmccloskey) → review+
Comment on attachment 8890040 [details] [diff] [review]
part 1 - centralize AvailableRunnable dispatching

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

Prefer keeping implementation details out of the header.

::: dom/canvas/WebGLQuery.cpp
@@ +61,5 @@
>      LinkedListElement<WebGLQuery>::removeFrom(mContext->mQueries);
>  }
>  
> +void
> +WebGLQuery::DispatchAvailableRunnable()

static void
AvailableRunnable::Dispatch(WebGLQuery*)

::: dom/canvas/WebGLQuery.h
@@ +49,5 @@
>  
>      // WebGLRefCountedObject
>      void Delete();
>  
> +    void DispatchAvailableRunnable();

Doesn't need to be a member.

::: dom/ipc/ContentParent.cpp
@@ +2762,5 @@
>        Unused << SendFlushMemory(nsDependentString(aData));
>    }
>    // listening for remotePrefs...
>    else if (!strcmp(aTopic, "nsPref:changed")) {
> +    // We know prefs are ASCII here. 

Accidental EOL whitespace here
Attachment #8890040 - Flags: review?(jgilbert) → review+
Comment on attachment 8890042 [details] [diff] [review]
part 2 - label WebGLQuery's AvailableRunnable

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

::: dom/canvas/WebGLQuery.cpp
@@ +63,5 @@
>  
>  void
>  WebGLQuery::DispatchAvailableRunnable()
>  {
> +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);

I agree with billm. Use new.

@@ +65,5 @@
>  WebGLQuery::DispatchAvailableRunnable()
>  {
> +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);
> +
> +    if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) {

Assigning in a sub-expression is bad enough, but declaring? Absolutely not.

@@ +66,5 @@
>  {
> +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);
> +
> +    if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) {
> +        canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other,

Add a WebGLContext::GetOwnerDoc, and have it assert if there's not canvas element for now.
Attachment #8890042 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 6

a year ago
Thanks for the review!

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> @@ +65,5 @@
> >  WebGLQuery::DispatchAvailableRunnable()
> >  {
> > +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);
> > +
> > +    if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) {
> 
> Assigning in a sub-expression is bad enough, but declaring? Absolutely not.

At least with declaring, you don't have possible =/== confusion.  But will change.

> @@ +66,5 @@
> >  {
> > +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);
> > +
> > +    if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) {
> > +        canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other,
> 
> Add a WebGLContext::GetOwnerDoc, and have it assert if there's not canvas
> element for now.

Given the "Get" prefix, I'm assuming you expect it to be written like:

nsIDocument*
WebGLContext::GetOwnerDoc()
{
  MOZ_ASSERT(mCanvasElement);
  if (!mCanvasElement) {
    return nullptr;
  }
  return mCanvasElement->OwnerDoc();
}

Is that a correct assumption, or are you suggesting to dispense with the !mCanvasElement check entirely?
Flags: needinfo?(jgilbert)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> Thanks for the review!
> 
> (In reply to Jeff Gilbert [:jgilbert] from comment #5)
> > @@ +65,5 @@
> > >  WebGLQuery::DispatchAvailableRunnable()
> > >  {
> > > +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);
> > > +
> > > +    if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) {
> > 
> > Assigning in a sub-expression is bad enough, but declaring? Absolutely not.
> 
> At least with declaring, you don't have possible =/== confusion.  But will
> change.
> 
> > @@ +66,5 @@
> > >  {
> > > +    RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this);
> > > +
> > > +    if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) {
> > > +        canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other,
> > 
> > Add a WebGLContext::GetOwnerDoc, and have it assert if there's not canvas
> > element for now.
> 
> Given the "Get" prefix, I'm assuming you expect it to be written like:
> 
> nsIDocument*
> WebGLContext::GetOwnerDoc()
> {
>   MOZ_ASSERT(mCanvasElement);
>   if (!mCanvasElement) {
>     return nullptr;
>   }
>   return mCanvasElement->OwnerDoc();
> }
> 
> Is that a correct assumption, or are you suggesting to dispense with the
> !mCanvasElement check entirely?

That sounds good.
Flags: needinfo?(jgilbert)
(Assignee)

Comment 8

a year ago
Created attachment 8895095 [details] [diff] [review]
part 2 - label WebGLQuery's AvailableRunnable

Thanks for the explanation.  I went with billm's suggestion of continuing to
dispatch to the current thread.
Attachment #8895095 - Flags: review?(jgilbert)
Attachment #8895095 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8890042 - Attachment is obsolete: true
Attachment #8895095 - Flags: review?(jgilbert) → review+
revised the patches according to reviewer's comments and wait for the test result from treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=954664240e0d2a0eaf4f279a451fc945afd7662f

Comment 10

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b144a76336
part 1 - centralize AvailableRunnable dispatching; r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a28654493547
part 2 - label WebGLQuery's AvailableRunnable; r=jgilbert,billm

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98b144a76336
https://hg.mozilla.org/mozilla-central/rev/a28654493547
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.