53627 - reduce number of attributes in thread-pane XUL template

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[Chris Waterson]

Reducing the number of attributes in the thread-pane will reduce footprint and
reduce the time it takes to construct content. Using the "class" attribute
rather than arbitrary attributes for style rules will reduce the amount of time
required to match style rules.

Current (2000-09-21) profiling shows that about 25% of the time in mail
scrolling is spent matching style rules, and 15% of the time is spent creating
the "base" XUL content (not including anonymous content created from XBL).

Comment 1

[Chris Waterson]

Attachment: first cut at reduction, classic skin only

Comment 2

[Chris Waterson]

You'll need to apply the last patch in bug 46134 to try out these changes.

Comment 3

[Chris Waterson]

Here is what I see:
                                      Before      After    Change
SelectorMatches (calls)              363,545    231,010       36%
nsXULElement::SetAttribute (calls)     5,958      5,188       13%
nsXULElement::GetAttribute (calls)   247,598    116,147       53%
style resolution (usec)              369,593    184,020       50%
content construction (usec)          184,020    160,948       12%

This is doing "page down" scrolling from the top to the bottom of an unthreaded
message pane with 107 messages in it. It's hard for me to estimate the "real
gain" here because I'm not sure of a good metric for measuring scrolling
performance.

Some comments:

- We reduce the calls to SelectorMatches by ~1/3 because we no longer need
  to enumerate rules: they're all hashed by class.

- Reducing calls to SetAttribute means less bloat, because we store
  fewer strings.

- Reducing calls to GetAttribute() is a win with respect to the amount
  of string copying that we do.

Comment 4

[David :Bienvenu]

This seems really cool. Is there any chance this will get into NS 6.0 (e.g., an
exception from the PDT)? I'll try it out anyway.

Comment 5

[scottputterman]

I agree that this is great.  I'll nominate for rtm. Thanks for looking at this.

Comment 6

[selmer (gone)]

Can someone make a stab at estimating the benefit?  How perceptible is the
improvement?  (Maybe it's most noticable on Linux where we need the most help.)
 I think we can get a plus on this if we have a reasonable story about the benefit.

Comment 7

[Alec Flett]

once chris fixes the multi-value attribute thing, this is a very low-impact fix 
that will likely have a minor benefit, but a benefit none the less. Risk is very 
low - possible odd highlighting or display in the threadpane, which will be very 
obvious. I think we should take it. Thanks a ton for doing the work, chris.

Comment 8

[selmer (gone)]

rtm+ to get on PDT radar, not sure the benefit is justified though.

Comment 9

[Chris Waterson]

Attachment: Patch for blue, modern, and classic/mac, too

Comment 10

[Chris Waterson]

I think the above patch covers the remaining skins. I've checked the fix for bug
46134 in on the trunk.

Comment 11

[Chris Waterson]

I did some very unscientific "end user" measurement: timed how long it took me
to page down through an unthreaded news group with 3,100 message (n.p.m.mac). On
my Linux box, I saw time drop from 102s to 93s, or about a 9% improvement.

Comment 12

[Michael La Guardia]

Marking rtm++.  Please land this as soon as possible so that we have time to fix
any problems this might cause.

Comment 13

[scottputterman]

Assuming I applied the patches correctly there looks to be a problem.  The
thread pane icon only works with the last attribute in the class. With the
current patch, imapDeleted is the only icon that shows up correctly.  If I move
MessageType to the end, it works but then you can't see the new state icons
because the Status attribute isn't working.

The easiest way to see this is if you open up a news folder, it will have the
same icon as a mail folder. Also, if you send yourself a new mail, it should
have the "new mail" icon, and it currently doesn't.

Comment 14

[scottputterman]

reassigning to waterson.

Comment 15

[Chris Waterson]

Downgrading status b/c scottip sez still problems with the patch.

Comment 16

[Chris Waterson]

Sure enough, there's a bug: it's in string code. I've filed bug 55143 and added
a dependency. I can work around the problem with the patch I'm about to attach.

Comment 17

[Chris Waterson]

Attachment: Work around broken nsPromiseFlatString

Comment 18

[Chris Waterson]

Attachment: work around broken iterator AND fix IsVarInSet()

Comment 19

[Chris Waterson]

Sure enough, yet another bug in the template builder! When >1 variable is in the
attribute, we were "forgetting" that an earlier variable was impacted by a
change. Need to logically-or the result from IsVarInSet() with previous values.

Comment 20

[David Hyatt]

a = hyatt

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

oo.. sexy.

a=ben (XUL stuff)

Comment 22

[Chris Waterson]

Fixes checked in to the trunk.

Comment 23

[Michael La Guardia]

re-marking rtm++

Comment 24

[Chris Waterson]

fixes checked in on branch

Comment 25

[esther]

Using branch build dated 10-16-00 on win98, mac and linux
Not sure how I can test this bug fix, so I will use performace results for
scrolling through a 500 msg POP account:
Windows:
On 9-21  arrow= 61.19 seconds, dragging =1.86 seconds.
On 10-16 arrow= 86.84 seconds, dragging =2.73 seconds.
Linux:
On 8-30 arrow= 62.47, dragging = 3.61
On 10-16 arrow=93.14, dragging = 2.24
Mac:
On 8-30 arrow=73.80, dragging = 2.35
On 10-16 arrow=72.87, dragging =1.72

Per these performance results there is some improvement on Mac, but win and
linux is worse.  Not sure if other bugs caused this performance issue
it. If I should test this in another way, please provide a test scenario.

Comment 26

[Chris Waterson]

Yeah, I think your testing may be confounded by some other tree widget changes
from hyatt and evaughan that have gone in. There is a known performance
regression with scrolling; can't recall the bug number though.

Comment 27

[Chris Waterson]

esther: if you're bored ;-), you could try comparing branch builds from
10-10-2000 and 10-11-2000 (since the fix went into the branch on 10-10-2000). I
don't think that any of the new tree stuff went into the branch until later...

Comment 28

[John Morrison]

The other bug is http://bugzilla.mozilla.org/show_bug.cgi?id=56683, although
it is a two-headed beast right now: one part is specific to scrolling in an
HTML page (slower due to line-height scrolled), and the other is a win98/win2k
scrolling speed difference in an IMAP message folder. 

However, for the same build, it's hard to reconcile the numbers I got on 
that bug, with the numbers posted here. We should get together to figure 
out the reason for the difference.

Comment 29

[esther]

I'll add the url to the Performance results so you can check the numbers.  We're
not bored, we have lots to do, but we need to try to verify the rtm++ bugs. This
one is a tough one because we don't have specific numbers for the poor
performance, and the time lapse between fix and verification has allowed other
bugs to creep in.   Now we just need to know that the performance is acceptable
for rtm per performance test results for mail.

Comment 30

[esther]

The difference in the times for scrolling is due to system config.  Our testing
for performance is with a 166 mhz 64MB RAM, the test morrison did is with a
500mhz 128MB RAM.  Bug 56683 mentioned above is for scrolling speed in both the
browser and 3-pane mail window it was logged on 10-14.  I will verify this one
since 56683 is more current.