130778 - txul slowed down by 1.3%

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Cathleen]

spin off of bug 130524

we have proof of bryner's change caused the 2nd jump (of all 3) on txul.

tested on a win98 200MHz box.

txul w/o bryner's change 1700 ms
txul w   bryner's change 1760 ms
                  --------------
                    diff   60 ms  or 3.52% jump
              

tinderbox facedown 1069->1630  +21ms  or  1.30% jump
tinderbox mecca    1754->1731  +23ms  or  1.32% jump
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1015968240&maxdate=1015971659
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1015968480&maxdate=1015972739

Comment 1

[Cathleen]

bryner, any status?

Comment 2

[Brian Ryner (not reading)]

Ok, after painstaking profiling on mecca, here's what I came up with:

With my patch applied, a fair amount (perhaps as much as 27% of the time spent
in the RuleProcessorData ctor) was spent in calls to xpconnect (calls of the
form XPTCStubBase::StubNNN).  With my section of code removed from the
RuleProcessorData ctor, this disappears, and the total number of hits in this
function decreases accordingly.

So, I think it's safe to say that calls to XPConnect during style resolution is
going to be a drag for slow systems and we should avoid this.

Comment 3

[Brian Ryner (not reading)]

Attachment: get the menuactive state from the frame instead

This patch goes back to the idea of getting the menuactive state from the
frame, not the content node, since the only way to get it from the content node
is through XBL (and is slow).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 75105 [details] [diff] [review]
get the menuactive state from the frame instead

r=dbaron

Comment 5

[David Hyatt]

sr=hyatt

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 75105 [details] [diff] [review]
get the menuactive state from the frame instead

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 7

[Brian Ryner (not reading)]

checked in.

Comment 8

[Brian Ryner (not reading)]

fix was backed out.  new fix coming.

Comment 9

[Brian Ryner (not reading)]

Attachment: use a c++ interface and content node to store the state

This introduces a new c++-only interface, nsIMenuElement, and a new content
node class, nsXULMenuElement (created for menubar and menupopup elements).  The
interface is implemented on XULMenuElement, HTMLOptGroupElement, and
HTMLSelectElement.  The interface allows access to a single data member storing
the active item inside this element.

Comment 10

[John Keiser (jkeiser)]

Comment on attachment 75534 [details] [diff] [review]
use a c++ interface and content node to store the state

>>> accessible/src/xul/nsXULMenuAccessible.cpp
+    if (content == menuEl->GetActiveItem())
	 *_retval |= STATE_FOCUSED;

Spacing.

>>> nsIMenuElement.h
Need a comment describing the class as a whole, perhaps describing briefly what
menuactive is (corresponds to :active, happens on hover or when a submenu is
open, etc.)

>> content/html/content/src/nsHTMLOptGroupElement.cpp

Need to add nsIMenuElement to the interface map

With that, r=jkeiser

Comment 11

[Brian Ryner (not reading)]

Attachment: address jkeiser's concerns

This fixes the problems jkeiser pointed out in the bug, and also fixes a
potential crash by ensuring that the active item is nulled out if the frame is
being destroyed.

Comment 12

[John Keiser (jkeiser)]

Comment on attachment 75722 [details] [diff] [review]
address jkeiser's concerns

r=jkeiser

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 75722 [details] [diff] [review]
address jkeiser's concerns

- In nsIMenuElement.h:

+  /**
+    * Get the menuactive state for this element.
+    */
+  virtual const nsIContent* GetActiveItem() const = 0;
+
+  /**
+    * Set the menuactive state for this element.
+    * @ param aIsActive Whether the element is in the menuactive state.
+    */
+  virtual void SetActiveItem(const nsIContent*) = 0;

Those comments don't seem to describe the API you're defining here.

sr=jst if you fix that.

Comment 14

[Brian Ryner (not reading)]

Indeed, I was originally designing it differently.  Thanks.

Comment 15

[Scott Collins]

Comment on attachment 75722 [details] [diff] [review]
address jkeiser's concerns

a=scc

Comment 16

[Brian Ryner (not reading)]

checked in.

Comment 17

[Cathleen]

thanks bryer! txul improved on tinderbox, when your fix landed.  :-)

mecca:  1728 -> 1688  ( 40ms or 2.31%)
facedown: 1583 -> 1562  ( 21ms or 1.32% ) 

Comment 18

[Brian Ryner (not reading)]

Oops, I forgot a file in the patch which fixes the crash I mentioned in comment 11.

Comment 19

[Brian Ryner (not reading)]

Attachment: patch to nsMenuFrame.cpp

Comment 20

[Brian Ryner (not reading)]

Attachment: don't leak

Comment 21

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 76072 [details] [diff] [review]
don't leak

r=pink

Comment 22

[Simon Fraser [no longer active]]

Comment on attachment 76072 [details] [diff] [review]
don't leak

sr=sfraser

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 76072 [details] [diff] [review]
don't leak

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 24

[Asa Dotzler [:asa]]

bryner, can you land this if you haven't? thanks.

Comment 25

[Brian Ryner (not reading)]

Ok, the original patch has been backed out.  Txul should be back to where it
started.