Last Comment Bug 792296 - popup.xml's state getter should try to avoid flushing frames
: popup.xml's state getter should try to avoid flushing frames
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla19
Assigned To: Neil Deakin (away until Oct 3)
:
Mentors:
Depends on: 807404
Blocks: tabopen 752496
  Show dependency treegraph
 
Reported: 2012-09-18 19:56 PDT by :Ehsan Akhgari
Modified: 2012-10-31 13:18 PDT (History)
13 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't flush when retrieving the state (1.60 KB, patch)
2012-09-19 08:20 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
patch that doesn't flush for other methods (4.01 KB, patch)
2012-09-21 13:09 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
patch, v2 (4.05 KB, patch)
2012-09-24 10:39 PDT, Neil Deakin (away until Oct 3)
neil: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-09-18 19:56:18 PDT
I've seen this in tons of profiles.  popup.xml's state getter is unbelieably expensive since it flushes by calling nsBoxObject::GetFrame(false) which flushes frames <http://dxr.allizom.org/mozilla-central/layout/xul/base/src/nsBoxObject.cpp#l112>.

Can we somehow avoid flushing there?
Comment 1 Neil Deakin (away until Oct 3) 2012-09-19 08:20:08 PDT
Created attachment 662571 [details] [diff] [review]
don't flush when retrieving the state
Comment 2 :Ehsan Akhgari 2012-09-19 10:35:44 PDT
Thanks for your patch, Neil!  That was fast  :-)

So here's a question, there's a bunch of other places in this code where we call GetFrame.  Can we avoid this in some of those other cases as well?
Comment 3 Neil Deakin (away until Oct 3) 2012-09-19 12:43:13 PDT
Probably only matters for setConsumeRollupEvent (newer code should use the consumeoutsideclicks attribute anyway) and outerScreenRect, as the former is expected to be called before opening the popup and the latter to be consistent with other coordinate getting methods.

The remainder you probably wouldn't call while the popup is open.
Comment 4 :Ehsan Akhgari 2012-09-20 12:35:35 PDT
(In reply to comment #3)
> Probably only matters for setConsumeRollupEvent (newer code should use the
> consumeoutsideclicks attribute anyway) and outerScreenRect, as the former is
> expected to be called before opening the popup and the latter to be consistent
> with other coordinate getting methods.

Can you please file bugs on those?  I'm not really familiar with what those names mean.  :-)
Comment 5 Neil Deakin (away until Oct 3) 2012-09-21 13:09:52 PDT
Created attachment 663530 [details] [diff] [review]
patch that doesn't flush for other methods
Comment 6 Neil Deakin (away until Oct 3) 2012-09-21 17:47:56 PDT
The patch above causes some failures in the private browsing tests, but these are fixed by the patch in bug 621944, which by a wild coincidence, I happened to be working on the same day.
Comment 7 neil@parkwaycc.co.uk 2012-09-22 16:26:21 PDT
Comment on attachment 663530 [details] [diff] [review]
patch that doesn't flush for other methods

> NS_IMETHODIMP
> nsPopupBoxObject::GetAutoPosition(bool* aShouldAutoPosition)
> {
>-  nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false));
>+  nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr;
>   if (menuPopupFrame) {
>     *aShouldAutoPosition = menuPopupFrame->GetAutoPosition();
>   }
> 
>   return NS_OK;
> }
Shouldn't this have a default value? The other getters seem to have one already.
Comment 8 Neil Deakin (away until Oct 3) 2012-09-24 10:37:03 PDT
(In reply to neil@parkwaycc.co.uk from comment #7)
> Comment on attachment 663530 [details] [diff] [review]
> patch that doesn't flush for other methods
> 
> > NS_IMETHODIMP
> > nsPopupBoxObject::GetAutoPosition(bool* aShouldAutoPosition)
> > {
> >-  nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false));
> >+  nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr;
> >   if (menuPopupFrame) {
> >     *aShouldAutoPosition = menuPopupFrame->GetAutoPosition();
> >   }
> > 
> >   return NS_OK;
> > }
> Shouldn't this have a default value? The other getters seem to have one
> already.

I will add one.
Comment 9 Neil Deakin (away until Oct 3) 2012-09-24 10:39:15 PDT
Created attachment 664118 [details] [diff] [review]
patch, v2
Comment 10 neil@parkwaycc.co.uk 2012-10-19 16:53:04 PDT
Comment on attachment 664118 [details] [diff] [review]
patch, v2

Sorry for the delay.

>-  nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false));
>+  nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr;
...
>-  nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false));
>+  nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(mContent->GetPrimaryFrame());
I assume that this one should get the ?: treatment too? r=me with that fixed.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-10-23 19:59:08 PDT
https://hg.mozilla.org/mozilla-central/rev/ddfc1c5c777a

Note You need to log in before you can comment on or make changes to this bug.