Closed Bug 615536 Opened 9 years ago Closed 9 years ago

nsIFrame::GetMouseThrough need to be optimized

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

Details

Attachments

(1 file, 8 obsolete files)

GetMouseThrough used always while scrolling fennec content/UI
and it is currently consuming ~1.75% of whole profile.

2  0.0649  libxul.so  fennec nsBox::GetMouseThrough() const
54 1.7532  libxul.so  fennec nsBoxFrame::GetMouseThrough() const

See 606672#c47

with attached patch it consume only:
0.1801 libxul %
Attachment #493967 - Flags: feedback?(roc)
Comment on attachment 493967 [details] [diff] [review]
Not tested with mozilla try, but logically should work, also works with fennec

Those bit values are already in use for boxes.
Also, shouldn't UpdateMouseThrough sometimes remove bits?  Except for button boxes and titlebars or something?
Attached patch Updated (obsolete) — Splinter Review
Assignee: nobody → romaxa
Attachment #493967 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #494106 - Flags: feedback?(roc)
Attachment #493967 - Flags: feedback?(roc)
Those bit values are already in use for boxes

> Also, shouldn't UpdateMouseThrough sometimes remove bits? 
what do you mean? it was originally const and not supposed to change any internal state...

by default there are no bits, which mean previous mMouseThrough = unset, and 
NS_FRAME_MOUSE_THROUGH_NEVER, NS_FRAME_MOUSE_THROUGH_ALWAYS are always and never accordingly
+  if (!IsFrameOfType(nsIFrame::eXULBox))
+    return PR_FALSE;

Do we need this? Why not get rid of it?

+    switch (mContent->FindAttrValueIn(kNameSpaceID_None,
+              nsGkAtoms::mousethrough, strings, eCaseMatters)) {
+      case 0: AddStateBits(NS_FRAME_MOUSE_THROUGH_NEVER); break;
+      case 1: AddStateBits(NS_FRAME_MOUSE_THROUGH_ALWAYS); break;

You need to remove the bit for case 2

+  RemoveStateBits(NS_FRAME_MOUSE_THROUGH_ALWAYS);
+  RemoveStateBits(NS_FRAME_MOUSE_THROUGH_NEVER);

You don't need these here, the bits start off 0

How much faster does this make things?
> > Also, shouldn't UpdateMouseThrough sometimes remove bits? 
> what do you mean? it was originally const and not supposed to change any
> internal state...
oh I'm totally wrong here
Attached patch Update to roc comment (obsolete) — Splinter Review
(In reply to comment #5)
> +  if (!IsFrameOfType(nsIFrame::eXULBox))
> +    return PR_FALSE;

hmm probably that is useful for fast exit from functions without while, 2x GetStateBits and one GetParentBox
Attachment #494106 - Attachment is obsolete: true
Attachment #494117 - Flags: review?(roc)
Attachment #494106 - Flags: feedback?(roc)
> 
> How much faster does this make things?

0.0649  libxul.so  fennec nsBox::GetMouseThrough() const
+
1.7532  libxul.so  fennec nsBoxFrame::GetMouseThrough() const
vs
0.1801 libxul
Are the numbers in comment 8 with the fixed patch that no longer runs into the bit reuse issue?

roc, do we want these bits living on all frames?  If they're supposed to be box-specific, then they need to be in the frame-specific range and we do need the frame type check (or better yet IsBoxFrame()).

Can UpdateMouseThrough be called for titlebar and buttonbox frames?
(In reply to comment #9)
> Are the numbers in comment 8 with the fixed patch that no longer runs into the
> bit reuse issue?
I'll retest that with updated patch tomorrow
(In reply to comment #9)
> roc, do we want these bits living on all frames?  If they're supposed to be
> box-specific, then they need to be in the frame-specific range and we do need
> the frame type check (or better yet IsBoxFrame()).

Yes, that does seem like a better idea.
> Can UpdateMouseThrough be called for titlebar and buttonbox frames?

it was not before.. can you explain why it is needed? and not in separate bug?
> roc, do we want these bits living on all frames?  If they're supposed to be
> box-specific, then they need to be in the frame-specific range and we do need

Did not get exactly what do you mean here, do you want that frameBits definition to be moved into nsBoxFrame.h/nsBox.h, or make GetMouseThrough method pure Box/BoxFrame specific and before calling GetMouseThrough always check and cast frame to nsBoxFrame/nsBox ?


> the frame type check (or better yet IsBoxFrame()).
Also would not it be better just remove GetMouseThrough from nsIFrame and make it as static function in nsDisplayList only?
> it was not before.

Are you sure?  They don't _implement_ it, which means the superclass version is called if someone calls it.  But the superclass version does the wrong thing with your patch (makes it possible to mouse through those frames), no?  So if that function is ever called for those concrete classes (which btw it is; that's what I asked you to check so I wouldn't have to waste time doing it) you get wrong behavior.  Which the nonexistent tests in the patch should have caught...

> do you want that frameBits definition to be moved into nsBoxFrame.h/nsBox.h,

I mean that if the bits are specific to boxframe, then they need to be in the frame-class-specific bit range.  If they're meant to apply to all frames, they're fine where they are (but then we should just use MOUSE_THROUGH_NEVER on non-box frames instead of adding the extra check up front in GetMouseThrough, right?).
> called if someone calls it.  But the superclass version does the wrong thing
> with your patch (makes it possible to mouse through those frames), no?  So if
hmm without my patch nsButtonBoxFrame.h were returning PR_FALSE
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsButtonBoxFrame.h#61

with my patch it is adding 
AddStateBits(NS_FRAME_MOUSE_THROUGH_NEVER); in nsButtonBoxFrame ctor, so nsIFrame::GetMouseThrough will return PR_FALSE in first check and does not allow to mouse through those frames..

// fix me if I'm totally wrong here.
nsBoxFrame::Init is called for |this| an nsButtonBoxFrame.  And this happens after the ctor.  And it'll remove that flag the ctor added.
Attached patch Adressed comments 615536#c9 (obsolete) — Splinter Review
Attachment #494117 - Attachment is obsolete: true
Attachment #494312 - Flags: review?(bzbarsky)
Attachment #494117 - Flags: review?(roc)
Comment on attachment 494312 [details] [diff] [review]
Adressed comments 615536#c9

(In reply to comment #17)
> nsBoxFrame::Init is called for |this| an nsButtonBoxFrame.  And this happens
> after the ctor.  And it'll remove that flag the ctor added.

Oh, yes, you right.
Attachment #494312 - Flags: review?(bzbarsky)
Attached patch Fixed comment #9 (obsolete) — Splinter Review
Attachment #494312 - Attachment is obsolete: true
Attachment #494315 - Flags: review?(bzbarsky)
Attachment #494315 - Attachment is obsolete: true
Attachment #494319 - Flags: review?(bzbarsky)
Attachment #494315 - Flags: review?(bzbarsky)
Attachment #494319 - Flags: review?(roc)
With latest patch 
 nopatch      patch
 3.0511   vs  1.4444 

almost 2x improvement.
I would put the test for IsBoxFrame in GetMouseThrough.

+#define NS_FRAME_MOUSE_THROUGH_ALWAYS    NS_FRAME_STATE_BIT(32)
+#define NS_FRAME_MOUSE_THROUGH_NEVER     NS_FRAME_STATE_BIT(33)

These bit choices incorrect. See nsIFrame.h:
// Bits 20-31 and 60-63 of the frame state are reserved for implementations.
#define NS_FRAME_IMPL_RESERVED                      nsFrameState(0xF0000000FFF00000)

The rest looks OK, except you're changing the nsIFrame vtable and apparently we haven't decided if that's currently allowed or not.
Attached patch Fixed bits number 60-61 (obsolete) — Splinter Review
>haven't decided if that's currently allowed or not.
why do we care? it is internal API isn't it?
Attachment #494319 - Attachment is obsolete: true
Attachment #494501 - Flags: review?(roc)
Attachment #494319 - Flags: review?(roc)
Attachment #494319 - Flags: review?(bzbarsky)
You haven't done this:
> I would put the test for IsBoxFrame in GetMouseThrough.

The rest looks good, apart from the nsIFrame issue.
Attached patch Moved check into function (obsolete) — Splinter Review
hmm I hoped that check for boxFrame outside will help to avoid entering into GetMouseThrough function in most cases... but with gcc inliner it does not make much sense.

What should we do about vtable? make empty vtable entry without implementation or what?
Attachment #494501 - Attachment is obsolete: true
Attachment #494523 - Flags: review?(roc)
Attachment #494501 - Flags: review?(roc)
Yeah, that's the best option for now. Just leave the function in nsIFrame.h with a big XXX comment "take this out after we've branched"
Attachment #494523 - Attachment is obsolete: true
Attachment #494636 - Flags: review?(roc)
Attachment #494523 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/cb897c736451
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.