74771 - [perf]nsFont and nsStyleFont are expensive to create due to nsString used for the name

Status: RESOLVED DUPLICATE of bug 78695

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Daniel Bratell]

In the colour table stress case a couple of percent of the total time is spent 
creating 56000 nsFonts inside 28000 nsStyleFonts. The cost is 99% from copying 
strings and my stomach tells me that it is the same string (or maybe 2-3) in all 
those 56000 nsFonts. It should be possible to use a better string representation 
or maybe something completely different. Pointers to a global table? That might 
also save something like half a MB of memory if all those 56000 strings are 
living at the same time.

The declaration right now is:
struct NS_GFX nsFont {
  // The family name of the font
  nsString name;
...
}

That COW string that people has been talking about maybe?

Comment 1

[Daniel Bratell]

Reassigning to attinasi who I think have made a lot of work in this area.

Comment 2

[Marc Attinasi]

nsFont should probably use an atom for the name - then style could use atoms for
the font face names and we could avoid all of the copying and such.

I'm not really a font guy at all - good try though ;) Can somebody suggest who
wight be able to help out with this analysis?

Comment 3

[Daniel Bratell]

Styles, fonts it's all becoming blury.... And the wheel is stopping on ... 
[e-mail]? Erik, is this something you've looked at?

Comment 4

[Daniel Bratell]

Btw, only 540 nsStyleFonts are being created in the black&white table. Why that 
difference, when the only thing that differs is that the colour one has 
the bgcolor attribute on the table cells?

Comment 5

[Marc Attinasi]

Daniel, to answer your last question: if every cell is the same color, then each
frame can share the same style context. If each cell is a different color, then
each cell needs its own unique style context. That is the big difference.

Comment 6

[Marc Attinasi]

Updating summary to reflect the problem with nsFont. Reassigning to erik - once
the nsFont is updated the nsStyleFont can easily be updated as well.

Comment 7

[Erik van der Poel]

Just adding a note that bug 38993 is related since it also talks about hashing
(related to the discussion of atoms in this report).

Comment 8

[Frank Tang]

Marc- could you be clear what need to be done for nsFont? erik is leaving. it 
will be very helpful if you can be clear what should we do for this bug and how 
important so I can find the right guy to take over. 

Comment 9

[Marc Attinasi]

nsFont could store an ATOM for the name member instead of an nsString. Once the
nsFont is changed, then callers can provide ATOM instances to nsFont and avoid
string copying, allocations, etc.

Comment 10

[Pierre Saslawsky]

Reassigned to myself, hoping that someone will fix it while I'm away.

Comment 11

[rbs]

brattell, my font changes over at bug 74186 have reduced the size of 
nsStyleFont by half -- the bare minimum. This could sizeably impact
the dreaded table stress case. Care to have a look?

Comment 12

[Daniel Bratell]

I just looked at a recent Quantify run and I'm sorry (?) to say that this bug is
quite deprecated by Hyatt's redesign of the style system. Where there used to be
28000 nsStyleFonts there are now only 8. 56000 nsFonts have been replaced by 16. 

I guess that the hard test for this would be a page with lots of different fonts
(and font sizes?) and whatever else lives in the nsStyleFont. I don't have such
a page among the two pages I use for testing.
That is, an nsStyleFont is still expensive to create compared to other style
structures but since there are so few of them, it might not matter. Total time
for the table stress case is only 0.1 ms. Total size would be < 1KB I guess.

Comment 13

[rbs]

So pierre, should this be marked as FIXED/DUPLICATE or what?

Comment 14

[Daniel Bratell]

First, what kind of page would make it impossible to share nsStyleFonts? Can
there exist such a page with thousands (or tens or even hundred of thousands) of
different nsStyleFonts? If not, there is no need to keep this bug around. If it
can, we should keep this bug as a reminder.

Comment 15

[Pierre Saslawsky]

Daniel, thanks for the verification.  This bug can be seen as fixed by the rule 
tree landing. 


*** This bug has been marked as a duplicate of 78695 ***