Closed
Bug 615536
Opened 14 years ago
Closed 14 years ago
nsIFrame::GetMouseThrough need to be optimized
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: romaxa, Assigned: romaxa)
Details
Attachments
(1 file, 8 obsolete files)
17.17 KB,
patch
|
roc
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
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 1•14 years ago
|
||
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.
Comment 2•14 years ago
|
||
Also, shouldn't UpdateMouseThrough sometimes remove bits? Except for button boxes and titlebars or something?
Assignee | ||
Comment 3•14 years ago
|
||
Assignee: nobody → romaxa
Attachment #493967 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #494106 -
Flags: feedback?(roc)
Attachment #493967 -
Flags: feedback?(roc)
Assignee | ||
Comment 4•14 years ago
|
||
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?
Assignee | ||
Comment 6•14 years ago
|
||
> > 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
Assignee | ||
Comment 7•14 years ago
|
||
(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)
Assignee | ||
Comment 8•14 years ago
|
||
>
> 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
Comment 9•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Assignee | ||
Comment 12•14 years ago
|
||
> 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?
Assignee | ||
Comment 13•14 years ago
|
||
> 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()).
Assignee | ||
Comment 14•14 years ago
|
||
Also would not it be better just remove GetMouseThrough from nsIFrame and make it as static function in nsDisplayList only?
Comment 15•14 years ago
|
||
> 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?).
Assignee | ||
Comment 16•14 years ago
|
||
> 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.
Comment 17•14 years ago
|
||
nsBoxFrame::Init is called for |this| an nsButtonBoxFrame. And this happens after the ctor. And it'll remove that flag the ctor added.
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #494117 -
Attachment is obsolete: true
Attachment #494312 -
Flags: review?(bzbarsky)
Attachment #494117 -
Flags: review?(roc)
Assignee | ||
Comment 19•14 years ago
|
||
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)
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #494312 -
Attachment is obsolete: true
Attachment #494315 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 21•14 years ago
|
||
Attachment #494315 -
Attachment is obsolete: true
Attachment #494319 -
Flags: review?(bzbarsky)
Attachment #494315 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #494319 -
Flags: review?(roc)
Assignee | ||
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 24•14 years ago
|
||
>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.
Assignee | ||
Comment 26•14 years ago
|
||
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"
Assignee | ||
Comment 28•14 years ago
|
||
Attachment #494523 -
Attachment is obsolete: true
Attachment #494636 -
Flags: review?(roc)
Attachment #494523 -
Flags: review?(roc)
Attachment #494636 -
Flags: review?(roc)
Attachment #494636 -
Flags: review+
Attachment #494636 -
Flags: approval2.0+
Assignee | ||
Comment 29•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/cb897c736451
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•