189713 - Ts regression due to toolbar grippy restoration

Status: RESOLVED INCOMPLETE

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The grippy restoration caused startup to slow down by 5% on mecca, luna, and
comet.  I can't believe that the grippies require that much time to do, so I
have to suspect that something else that you changed was changed incorrectly. 
Whatever it was, please fix it.

Comment 1

[Jan Varga [:janv]]

That's why I originally removed them.
I didn't change anything, it was plain backout.

Comment 2

[Jan Varga [:janv]]

huh 5% ?
It's only 1-2% (as before)

Comment 3

[Hixie (not reading bugmail)]

A 1% performance regression like this is totally unacceptable.

Comment 4

[R.K.Aa.]

I remember there was a performance win from removing the grippies.
It was also mentioned in bug 112534 comment 44, right after grippies were removed.
Quote:

perf wins:
Txul: 2-3%
Ts: 1-2%

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  The point where I looked at it seemed to have unusual swing.  It's
about 3%.

Still, the fact that the grippies cost 3% of startup just tells me that
something in the implementation is seriously flawed.  Most likely, one of the
CSS rules involved....

In particular, the change to xul.css that introduces a "slow" rule (the
*[moz-collapsed="true"] selector) seems extremely suspicious to me.  I'd be
willing to bet that a lot of the cost is due to that...

Comment 6

[Jan Varga [:janv]]

Let's back out autocomplete widget too, it costs us almost 10% of Txul.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Where did I say to back this out?  I said to fix the performance hit, which is
ridiculously disproportionate...  Then I actually went and read the patch and
pointed you to the most likely culprit (if that doesn't pan out, I would be
willing to read the patch again looking for other possible culprits, but I
suspect that won't be needed).  I suppose I could try to do the perf
investigation myself too; it's just that I have no good way to run the Ts tests
(they are always too noisy when I run them).  Have you tried removing that one
CSS rule I pointed out and testing the impact?

If autocomplete takes 10% of Txul then we should figure out why and _fix_ it. 
Not sure what that has to do with this bug.

Comment 8

[Jan Varga [:janv]]

Ok, here are some numbers:

Txul (grippies fully functional)
med avg
387 401
392 398
389 398
389 399
-------
389

Txul (grippies removed)
med avg
379 390
378 389
381 390
380 390
-------
379.5 (2.4%)


Txul (w/o moz-collapsed CSS rule)
med avg
387 399
386 396
387 396
394 400
-------
388.5 (0.1%)

Txul (w/o <constructor>s in menubar and toolbar)
med avg
386 393
393 403
387 397
387 397
-------
388.3 (0.2%)

Txul (w/o CSS/themes part)
med avg
387 401
384 395
387 396
384 393
385.5 (0.9%)

Txul (w/o <toolbargrippy> in menubar and toolbar anonymous <content>)
med avg
382 396
382 392
392 398
382 391
-------
384.5 (1.2%)

Txul (w/o XBL part)
med avg
380 395
378 395
377 384
380 387
-------
378.8 (2.6%)


Note, that you can't just sum this numbers (a part of the implementation affects
other parts too) and there is also noise.

So it seems that moz-collapsed is not involved here at all.
XBL part covers most of perf hit (about 10KB of additional code in toolbar.xml,
not active though).

When I compared Phoenix's and Mozilla's autocomplete some time ago, I noticed
that autocomplete.xml in Phoenix is much smaller than the Mozilla's one.
According my measurements I've done, autocomplete in Phoenix takes only 4% and
almost 12% in Mozilla (sorry I provided 10% before, that was not correct)

So it seems like a penalty we pay for using XBL so much.
Probably, we need to initialize or something all the bindings after a window is
opened.

My $0.02

Comment 9

[Jan Varga [:janv]]

And sorry for being sarcastic in previous comment.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Jan, thanks for doing that testing...  It's too bad that the problem seems to be
a lot more complicated than I hoped.  :(

I'll try to ping hyatt for some info/ideas...

Comment 11

[Chris McAfee]

Jan, you are welcome to test this on one of the slower machines on testerbox,
your S/N ratio will be a lot better there.  

Comment 12

[Jan Varga [:janv]]

So here are more accurate numbers:

                            Txul,Ts    Perf hit
baseline                    2093,5933
moz-collapsed removed       2093,5932  0%
CSS rules removed           2082,5869  0.52%, 1%
XBL removed                 2058,5851  1.67%, 1.38%
grippy removed from toolbar 2087,5907  0.29%, 0.44%

Thanks mcafee for testing my patches on tbox.

Comment 13

[Jan Varga [:janv]]

Attachment: patch to removed moz-collapsed

Comment 14

[Jan Varga [:janv]]

Attachment: patch to remove CSS

Comment 15

[Jan Varga [:janv]]

Attachment: patch to remove XBL part

Comment 16

[Jan Varga [:janv]]

Attachment: patch to remove grippy from toolbar and menubar

Comment 17

[Chris McAfee]

Note to comment #12, each patch was tested independently, and I returned to the
baseline for a few cycles between each patch to make sure I didn't leave any
patches in.

Comment 18

[Robert Kaiser]

Hmm, it looks for me that backing out grippies won't resolve the real issue we
have there. I heard some times that XBL is costly sometimes, I didn't expect it
to be that bad thouhg :(

XBL is a cool technology, I really love it, but it looks bad to me when it eat
that much performance. What's that expensive there? Parsing the XBL definition?
Loading the XBL file?
It seems there's some potential here to improve performance on the whole thing,
if we find out what's really causing XBL to be a perf hog here...

Comment 19

[Pierre Chanial]

fwiw, in Phoenix I converted the personal toolbar code from js into XBL, and we
had a 3% perf hit.

Comment 20

[Jan Varga [:janv]]

Yeah, I remember that, thanks for pointing that out

Comment 21

[Shawn Wilsher :sdwilsh]

I'm going through and marking old performance regression bugs as INCOMPLETE that are likely too old to be valid or get any traction on them.

Please re-open if you have more information or can demonstrate the regression still exists.