Closed Bug 1350828 Opened 7 years ago Closed 7 years ago

gfx-labeling Label for CompositableFowarder

Categories

(Core :: Graphics: Layers, enhancement, P1)

Product:
Core
Component:
Graphics: Layers
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kechen, Assigned: kechen)

References

Details

(Whiteboard: [QDL][TDC-MVP][GFX])

Attachments

(1 file)

Bug 1350828 - Label CompositorForwarder;
59 bytes, text/x-review-board-request
bevis
: review+
kats
: review+
Details 
According to [1], we open a bug for CompositableForwarder labeling.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1341537#c2
Assignee: nobody → kechen
Whiteboard: [QDL][TDC-MVP][GFX]
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review127162

This patch is incomplete yet and shall depends on the work of bug 1345464 I am currently working on.
The "CompositableForwarder" runnable in the telemetry data is actually the ActiveResourceTracker:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/gfx/layers/ipc/ShadowLayers.cpp#196
which inherits from nsExpirationTracker (There is a internal timer in nsExpirationTracker that was labeled with the given aName ("CompositableForwarder").

::: gfx/layers/ipc/ShadowLayers.cpp:220
(Diff revision 1)
>  
>  ShadowLayerForwarder::~ShadowLayerForwarder()
>  {
>    MOZ_ASSERT(mTxn->Finished(), "unfinished transaction?");
>    delete mTxn;
> +  dom::TabGroup* tabGroup = mClientLayerManager->GetOwningTab()->TabGroup();

Could we make an assertion here to tabGroup if you think it will never be nullptr.
Attachment #8852329 - Flags: review?(btseng)
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review127186

Mostly looks ok, would like to see a new version of the patch after the comment below is addressed.

::: gfx/layers/client/ClientLayerManager.h:67
(Diff revision 1)
> +  TabChild* GetOwningTab() { return mWidget->GetOwningTabChild(); }
> +

I would rather make this return the TabGroup than the TabChild. Also move the method implementation into ClientLayerManager.cpp and add null checks or MOZ_ASSERTs. Depending on when this is called, mWidget or the TabChild might be null I think.
Attachment #8852329 - Flags: review?(bugmail) → review-
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review127588

::: gfx/layers/ipc/ShadowLayers.cpp:242
(Diff revision 2)
>  
>    if (!NS_IsMainThread()) {
> -    NS_DispatchToMainThread(
> -      new ReleaseOnMainThreadTask<ActiveResourceTracker>(mActiveResourceTracker));
> +    RefPtr<ReleaseOnMainThreadTask<ActiveResourceTracker>> event =
> +      new ReleaseOnMainThreadTask<ActiveResourceTracker>(mActiveResourceTracker);
> +    if (tabGroup) {
> +      tabGroup->Dispatch("ActiveResourceTracker::!ActiveResourceTracker",

s/::!/::
Attachment #8852329 - Flags: review?(btseng)
Attachment #8852329 - Flags: review?(bugmail)
As explained in comment 2, please resume the review until the labeling of ActiveResourceTracker related to bug bug 1345464 is done, thanks!
Priority: P3 → P1
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review131248

Please see my comments below, thanks!

::: gfx/layers/ipc/ShadowLayers.cpp:200
(Diff revision 3)
>  {
>    mTxn = new Transaction();
> -  mActiveResourceTracker = MakeUnique<ActiveResourceTracker>(1000, "CompositableForwarder");
> +  if (TabGroup* tabGroup = mClientLayerManager->GetTabGroup()) {
> +    mEventTarget = tabGroup->EventTargetFor(TaskCategory::Other);
> +  }
> +  mActiveResourceTracker = MakeUnique<ActiveResourceTracker>(

Per offline discussion, according to the call flow of the construction of ShadowLayerForwarder, it seems that the TabGroup will always be alive and so as its event targets.

Hence, it will be more clear to have MOZ_(RELEASE_)ASSERT(mEvenTarget) check here instead of have nullptr check everytime when accessing mEventTarget.

::: gfx/layers/ipc/ShadowLayers.cpp:228
(Diff revision 3)
>    if (mShadowManager) {
>      mShadowManager->SetForwarder(nullptr);
>      if (NS_IsMainThread()) {
>        mShadowManager->Destroy();
>      } else {
> +      if (mEventTarget) {

ditto

::: gfx/layers/ipc/ShadowLayers.cpp:243
(Diff revision 3)
> +  }
>  
>    if (!NS_IsMainThread()) {
> -    NS_DispatchToMainThread(
> -      new ReleaseOnMainThreadTask<ActiveResourceTracker>(mActiveResourceTracker));
> +    RefPtr<ReleaseOnMainThreadTask<ActiveResourceTracker>> event =
> +      new ReleaseOnMainThreadTask<ActiveResourceTracker>(mActiveResourceTracker);
> +    if (mEventTarget) {

ditto
Attachment #8852329 - Flags: review?(btseng) → review-
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9)
> Comment on attachment 8852329 [details]
> Bug 1350828 - Label CompositorForwarder.
> 
> https://reviewboard.mozilla.org/r/124602/#review131248
> 
> Please see my comments below, thanks!
> 
> ::: gfx/layers/ipc/ShadowLayers.cpp:200
> (Diff revision 3)
> >  {
> >    mTxn = new Transaction();
> > -  mActiveResourceTracker = MakeUnique<ActiveResourceTracker>(1000, "CompositableForwarder");
> > +  if (TabGroup* tabGroup = mClientLayerManager->GetTabGroup()) {
> > +    mEventTarget = tabGroup->EventTargetFor(TaskCategory::Other);
> > +  }
> > +  mActiveResourceTracker = MakeUnique<ActiveResourceTracker>(
> 
> Per offline discussion, according to the call flow of the construction of
> ShadowLayerForwarder, it seems that the TabGroup will always be alive and so
> as its event targets.
> 
> Hence, it will be more clear to have MOZ_(RELEASE_)ASSERT(mEvenTarget) check
> here instead of have nullptr check everytime when accessing mEventTarget.
> 
> ::: gfx/layers/ipc/ShadowLayers.cpp:228
> (Diff revision 3)
> >    if (mShadowManager) {
> >      mShadowManager->SetForwarder(nullptr);
> >      if (NS_IsMainThread()) {
> >        mShadowManager->Destroy();
> >      } else {
> > +      if (mEventTarget) {
> 
> ditto
> 
> ::: gfx/layers/ipc/ShadowLayers.cpp:243
> (Diff revision 3)
> > +  }
> >  
> >    if (!NS_IsMainThread()) {
> > -    NS_DispatchToMainThread(
> > -      new ReleaseOnMainThreadTask<ActiveResourceTracker>(mActiveResourceTracker));
> > +    RefPtr<ReleaseOnMainThreadTask<ActiveResourceTracker>> event =
> > +      new ReleaseOnMainThreadTask<ActiveResourceTracker>(mActiveResourceTracker);
> > +    if (mEventTarget) {
> 
> ditto

Hello Bevis,
I didn't aware that ClientLayerManager can also exist in Chrome process; therefore, we might still need these null checks, but I can add an assertion for content side.
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review131284

r=me after the assertion is removed.
Please also ask a gfx peer to review this change, thanks!

::: gfx/layers/ipc/ShadowLayers.cpp:200
(Diff revision 4)
>  {
>    mTxn = new Transaction();
> -  mActiveResourceTracker = MakeUnique<ActiveResourceTracker>(1000, "CompositableForwarder");
> +  if (TabGroup* tabGroup = mClientLayerManager->GetTabGroup()) {
> +    mEventTarget = tabGroup->EventTargetFor(TaskCategory::Other);
> +  }
> +  MOZ_ASSERT(mEventTarget || !XRE_IsContentProcess());

We don't need this anymore if nullptr check will always be done.
Otherwise, this assertion makes too much assumption on the implementation in other modules.
Attachment #8852329 - Flags: review?(btseng) → review+
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review131376

::: gfx/layers/client/ClientLayerManager.h:34
(Diff revision 4)
>  class nsDisplayListBuilder;
>  
>  namespace mozilla {
>  namespace layers {
>  
> +using dom::TabGroup;

This file doesn't include TabGroup.h directly nor does it forward-declare the TabGroup class. It should do at least one of the two to avoid unified compilation failures later on. (My preference is to do the forward-declaration). If you include TabGroup.h here then you don't need to #include it in the .cpp file since that already includes this header.

::: gfx/layers/ipc/ShadowLayers.cpp:200
(Diff revision 4)
>  {
>    mTxn = new Transaction();
> -  mActiveResourceTracker = MakeUnique<ActiveResourceTracker>(1000, "CompositableForwarder");
> +  if (TabGroup* tabGroup = mClientLayerManager->GetTabGroup()) {
> +    mEventTarget = tabGroup->EventTargetFor(TaskCategory::Other);
> +  }
> +  MOZ_ASSERT(mEventTarget || !XRE_IsContentProcess());

I don't understand Bevis' comment here. The MOZ_ASSERT makes sense to me, and serves as a point of documentation (in effect, it says that mEventTarget may be null here if we are not in the content process, which is true).
Attachment #8852329 - Flags: review?(bugmail) → review+
Comment on attachment 8852329 [details]
Bug 1350828 - Label CompositorForwarder;

https://reviewboard.mozilla.org/r/124602/#review131376

> I don't understand Bevis' comment here. The MOZ_ASSERT makes sense to me, and serves as a point of documentation (in effect, it says that mEventTarget may be null here if we are not in the content process, which is true).

m.. because it doesn't good enough to explain that the mEventTarget will be valid only if XRE_IsContentProcess() is true if my understanding is correct. :(
It's fine if this still serves a point of documentation.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b624d605aadf -d afef2f99d59b: rebasing 389604:b624d605aadf "Bug 1350828 - Label CompositorForwarder; r=bevistseng,kats" (tip)
merging gfx/layers/client/ClientLayerManager.cpp
merging gfx/layers/client/ClientLayerManager.h
merging gfx/layers/ipc/ShadowLayers.cpp
merging gfx/layers/ipc/ShadowLayers.h
warning: conflicts while merging gfx/layers/ipc/ShadowLayers.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2c304e12e34
Label CompositorForwarder; r=bevistseng,kats
Blocks: 1343754
https://hg.mozilla.org/mozilla-central/rev/d2c304e12e34
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: