83624 - The nsAutoVoidArray constructor clears the memory which is expensive

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Bratell]

I quantified the style system after hyatt's landing of the redesigned system and 
noticed that 42% of the time in CSSRuleProcessor::RulesMatching and 3% overall 
is spent in the nsAutoVoidArray constructor. The reason this is expensive is 
because it clears the full array with memset. I don't see the reason for this. 
It seems like an awful easy fix to just remove the memset.

All the figures are from the colour table stress case

Comment 1

[Edward Kandrot]

It does seem kind of weird, because looking at the code, it appears to be
setting none of them to zero (mImpl->mCount is zero by the time ::memset is
called).  From what it looks like to me, I would say we could remove that line,
since it currently does nothing.

Comment 2

[Daniel Bratell]

You are right. So that line is without any meaning at all (yet it showes on 
profiles). Do you want a patch? :-)

Comment 3

[Edward Kandrot]

Sure, if you have a patch, post it and I will r= it.  I believe that the tree
will be closed except for crasher bugs for a bit, bit I will check it in when it
does open.  Thanks.

Comment 4

[Daniel Bratell]

Attachment: One line fix

Comment 5

[Daniel Bratell]

I attached the patch. I have run with it for a week without noticing anything 
bad.

Comment 6

[Edward Kandrot]

r=kandrot.  sr=?  a=?

Does it look to be any faster with this patch?  Did the times drop as predicted?
 Just curious.

Comment 7

[Chris Waterson]

sr=waterson

Comment 8

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 9

[Edward Kandrot]

I have checked in the change.  Thanks.  (though if you have the performance
numbers, please post them to this bug, thanks)

Comment 10

[Daniel Bratell]

The numbers I have is from Quantify in the table stress case.

nsAutoVoidArray::nsAutoVoidArray 30000 calls
  Before: F+D 71 ms (1.47% of total time)
  After:  F+d  1 ms (0.02% of total time)

Comment 11

[Chris Waterson]

Good lord, is that milliseconds? (I'm guessing you meant to write 71us...)

Comment 12

[Daniel Bratell]

No, Quantify says 70907,32 us (microseconds) 1.47% of Focus.

Then I'm not sure if Quantify is right since the memset is timed rather than 
profiled by instruction count.

The After time is 977,86 us.

I'm marking this VERIFIED since the memset is very gone now (code inspection).