Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

2 years 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

2 years ago
No longer blocks: Labeling
Assignee

Updated

2 years ago
Blocks: Labeling

Updated

2 years ago
Whiteboard: [gfx-noted]
Assignee

Comment 1

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

Comment 2

2 years ago
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

2 years 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

2 years ago
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

2 years 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

2 years 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

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