68103 - 21% speedup in Seamonkey with a fix in nsXULElement!

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[David Hyatt]

Preventing style resolution on the value attribute gives us a colossal speedup.

Comment 1

[David Hyatt]

Setting to moz0.9

Comment 2

[David Hyatt]

Attachment: the patch.

Comment 3

[Brendan Eich [:brendan]]

Patch looks good to me.  Cc'ing likely reviewers.  Should we get this into
mozilla0.8?

/be

Comment 4

[Brian 'netdragon' Bober]

Brendan: If there is a 21% speed up then I surely hope it is.

Comment 5

[Brian 'netdragon' Bober]

I am applying this patch as we speak and I will tell you the results in a sec.

Comment 6

[Peter "jag" Annema]

Nit:
nsCOMPtr<nsIPresShell> shell(dont_AddRef(mDocument->GetShellAt(0)));

Looks good otherwise. Could there be any drawback here?

Comment 7

[HJ]

You have my vote, can this be done for 0.8?

Comment 8

[Håkan Waara]

neat! hyatt, how did you measure this?

Comment 9

[Brian 'netdragon' Bober]

When I added the patch and pulled, I got this error: 

###!!! ASSERTION: Filtered insertion point wasn't properly constructed.
: 'Error', file C:\Programming\netscape\mozilla\layout\xbl\src\nsXBLBinding.cpp,
 line 671

I don't know if its relevant.

Comment 10

[James Green]

For the 3rd time (bloody mid-air collisions) adding perf keyword and ccing self.
Shouldn't this be OS/All?

Comment 11

[Brian 'netdragon' Bober]

I also got a lot of these: 

XML Error in file 'file:///C|/WINDOWS/USER%20PROFILES%20AND%20SETTINGS/BOBERB/AP
PLICATION%20DATA/Mozilla/Users50/MozillaDebug/41qg4eeq.slt/localstore.rdf', Line
 Number: 49, Col Number: 191, Description: not well-formed
Source Line:     <RDF:li>|javascript:"mail:window".split(":")[0].big().big().big
().big()| code, source: |&lt;big&gt;&lt;big&gt;&lt;big&gt;&lt;big&gt;mail&lt;/bi
g&gt;&lt;/big&gt;&lt;/big&gt;&lt;/big&gt;...&#9835; |</RDF:li>


RDF: NS_NewLocalStore::Refresh() failed.

sorry for the spam.

Comment 12

[Brian 'netdragon' Bober]

Ok, the second posting wasn't relevant. I installed a new profile and it went 
away. 

WEBSHELL+ = 3
Enabling Quirk StyleSheet
###!!! ASSERTION: Filtered insertion point wasn't properly constructed.
: 'Error', file C:\Programming\netscape\mozilla\layout\xbl\src\nsXBLBinding.cpp,
 line 671

This still happens.

Comment 13

[Brian 'netdragon' Bober]

BTW (sorry for the spam but we really have to get this bug nailed out quickly) 
I am on Win2k build: today.

Comment 14

[Brian 'netdragon' Bober]

errrgh just forgot, optimized build.

Comment 15

[Peter "jag" Annema]

This is all/all, marking so. Brian, I believe those assertions you're seeing are
the result of another checkin. You could confirm that by undoing this patch and
checking if you still see the assertions.

Patch works fine here on linux.

Comment 16

[Brian 'netdragon' Bober]

I backed it out and got the same error. I guess that means that it is something 
else with the build. I'll see if there is a bug on that.

Comment 17

[Chris Waterson]

I don't want to be a stick in the mud, but...

I think we should hold off on this for 0.8 (or, put it on the branch but not the 
trunk). I'm worried that checking this in will take the pressure off getting the 
other two performance regressions fixed (see Necko bug 66516 and attinasi's 
frame & style resolution investigation, 
news://news.mozilla.org/[e-mail]), and will merely return 
us to our previous (M18) level of page-load performance.

I'd rather see us fix the regressions, then land this and have a big party. I 
know jrgm shares my sentiment.

Also, FWIW, vidur suggests a cleaner way to do this:

  news://news.mozilla.org/[e-mail]

Comment 18

[Mike Shaver (:shaver emeritus)]

I agree with Waterson, and with Vidur: there's probably a cleaner way to do
this, and we should make sure that our other performance hits are resolved
before we take this (masking) win.

We could certainly use a new style-change notification, but I think there's
fertile territory in finishing the work described in 38378 (tracking the element
type as well as the attribute to determine which attribute changes should cause
a reresolution). This would let people write style rules that really did involve
"value" values, without having everyone pay the price.

While we're in there, we could chip away at some bloat by using PLHashTables or
PLDHashTables instead of nsHashtable.

It might be a win to finish the work for HTML, too, once we get the list of
attribute->style-rule mappings sorted out.

Comment 19

[Darin Fisher]

As far as bug 66516 is concerned, I have a figure for the performance loss
attributable to my changes.  So, it doesn't matter to me what happens on the
trunk; I will still be able to clearly know when I have solved the problem
I introduced.

Comment 20

[Jason Eager]

Please land this fix for the Mozilla 0.8 branch (keeping it unlanded on the trunk).

Comment 21

[Brian 'netdragon' Bober]

Ok, how about this? We land it for .8 and then back it out afterwards as 
incentive for people to work harder.

Comment 22

[Brian 'netdragon' Bober]

How about this? We stick the patch in there. Then, after the release of the 
milestone, we back it out and don't stick it back in after performance is back 
to top performance?

Comment 23

[Brian 'netdragon' Bober]

Sorry about the repeat, my connection is all messed up.

Comment 24

[Håkan Waara]

No. Let's decide whether to check it in or not, and then settle with that.

I just tried this patch for a while, I stress tested the browser and played
around with various different features and everything seems normal to me. I have
no idea on how to perf-test it thought.

Comment 25

[David Hyatt]

Attachment: the real patch (1 of 2)

Comment 26

[David Hyatt]

Attachment: the real patch (2 of 2)

Comment 27

[Marc Attinasi]

David, I like this new, more general patch much better. Most Excellent.

Comment 28

[Mike Shaver (:shaver emeritus)]

I like this better, but I'm still interested in hearing if we can do a better,
more general job with http://bugzilla.mozilla.org/show_bug.cgi?id=68198 . 
(Also, the second part of your patch to nsCSSFrameConstructor.cpp uses a brace
style inconsistent with that of the code you're patching.  Nyah.)

Comment 29

[Azrael AOD]

Checking this in or not shouldn't be decided on the basis of incentive to 
others to fix bugs, etc..
If this patch is _good_ then it should go in. If it fucks things up, it 
shouldn't.
And as far as I can work out from what I read and people have said, this patch 
does the bussiness and should be checked in.
Incentive?? Perhaps seeing a 21% speed up gives an incentive for more fixes ... 
than people just resting on their laurels and not bothering.

Comment 30

[Stephan Niemz]

adding topperf keyword

Comment 31

[Asa Dotzler [:asa]]

We're gonna take hyatt's real patch on the mozilla0.8 branch, once it has been
cut.  Anyone object?

Let me also ask again for clear pro/con statements about checking that patch
into the trunk for 0.9, to keep sources and benchmark data consistent (even if
it masks other regressions).

/be

Comment 32

[Chris Waterson]

I think everyone's happy with it. The only arguments against it are ``people 
won't work as hard to fix other problems'', which are probably lame arguments. 
(I was initially worried that people would think the other problems ``went 
away'', but it looks like attinasi and darin are on the case...)

Comment 33

[John Morrison]

Forgive me, but I am a bear of little brain. The test results may change
due to simple noise, or real code changes, or factors external to the test
(hopefully kept to minimum). It seemed to me that with 3 large 'chunks'
of code related causes on the table (darin, attinasi, hyatt), it would be
best to 'meter' how they are resolved. It is not an incentive argument. But
if darin, attinasi and hyatt have a good handle on the scope of their changes,
then it's fine with me. (But note that hyatt's scope and attinasi's scope are
not orthogonal; they will interact). 

Comment 34

[Marc Attinasi]

I have to find about a 10% slowdown in FrameConstruction. Currently I have no
idea where that performance regression came from, but I understand my target.
John is correct that Hyatt's change may impact performance in this area, so I
propose that I test how much affect Hyatt's changes have on FrameConstruction
timings before I make any changes, assuming that I can find something to change.
I can also simply remove his change (essentially back-out patch 2) to verify my
own improvements when I am ready to quantify them.

To reiterate, I am confident that checking this change in will not impact my
ability to analyze the slowdown in FrameConstruction.

Comment 35

[Jason Eager]

The branch has been made. Can we land this in the branch now?

Comment 36

[David Hyatt]

I'm off for the weekend.  Will land it on Monday.

Comment 37

[Daniel Bratell]

I don't understand this thing about landing this on the branch. All looks fine 
but are we sure that there is nothing that depends on the old behaviour? I don't 
think this looks like a simple, riskless patch to be landed on a release branch 
just a day or two before release. This looks like something that should live for 
a while in nightly builds to get at least some regression testing.

But maybe I just don't understand what the patch does.

Comment 38

[Asa Dotzler [:asa]]

I tested this on a clean branch and found no tree selection problems in mail or
bookmarks.  checked into the MOZILLA_0_8_BRANCH branch. 

Comment 39

[David Hyatt]

Fix checked in to trunk.

Comment 40

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

So, um, this was a 21% speedup of what test?  I suspect this optimization is no
longer needed thanks to bug 163556, and I've filed bug 211371 to investigate.

Comment 41

[John Morrison]

dbaron: speed up of pageload numbers.