nsVoidArray -> nsAutoVoidArray changes for Layout

RESOLVED FIXED in mozilla0.9.5

Status

()

Core
Layout
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({perf})

Trunk
mozilla0.9.5
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

17 years ago
Taken from bug 90545.  I _really_ want to see these committed ASAP.  The patch
will require that the phase 1 patch from that bug be applied/committed first.
(Assignee)

Comment 1

17 years ago
I'm taking this bug (it's a split off from my bug 90545).
Assignee: karnaze → rjesup
Depends on: 90545
Keywords: patch, perf
(Assignee)

Comment 2

17 years ago
Created attachment 43779 [details] [diff] [review]
Just the layout, ma'am.  Patch for nsVoidArray->nsAutoVoidArray for layout/*
(Assignee)

Updated

17 years ago
Blocks: 92256
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5

Comment 3

17 years ago
Review Comments: 

In nsCellMap.cpp, the method OrderRowGroups should be changed to take and
nsAutoVoidArray reference, and the following cast should be eliminated:
 -  aTableFrame.OrderRowGroups(orderedRowGroups, numRowGroups);
 +  aTableFrame.OrderRowGroups((nsVoidArray &)orderedRowGroups, numRowGroups);
It seems bad to cast the nsVoidArray to an nsAutoVoidArray like that.

Other than that, the change looks good to me, but <paranoias> since table code
is so heavily involved, it might be good to run the table regression tests
</paranoias> [s]r=attinasi
(Assignee)

Comment 4

17 years ago
Given a review from attinasi, I'll try to get this for 0.9.4.  Risk is low.
Target Milestone: mozilla0.9.5 → mozilla0.9.4

Comment 5

17 years ago
Randell, what about my comments?
(Assignee)

Comment 6

17 years ago
Sorry, I was going to post an updated patch, but I was waiting for it to compile
and test it first.

I simply removed the cast; it was a leftover and was irrelevant.  C++ does the
type-conversion automatically.
(Assignee)

Comment 7

17 years ago
Ran every testcase I could find; no errors that weren't present before the patch
(I ran two side-by-side, one with one without).
(Assignee)

Comment 8

17 years ago
Created attachment 47286 [details] [diff] [review]
Updated patch with cast removed

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Updated

17 years ago
Attachment #47286 - Flags: superreview+
Attachment #47286 - Flags: review+

Comment 9

17 years ago
r=kin@netscape.com
(Assignee)

Comment 10

17 years ago
Kin asked about possible bloat due to nsHTMLReflowCommands getting bigger.  Here
are the stats I gathered:

nsHTMLReflowCommand->mPath stats:
	Number created:    368
	Number in-use max:  10
	Num max size == 0:   0
	Num max size <= 8: 298
	Num max size  > 8:  70

So the worst possible bloat is 32*10 = 320 bytes, but since ~80% of them use no
more than 8 items, and none use 0, the likely bloat is more like 64 bytes.
(This was loading and resizing each of mozilla.org, cnn.com, yahoo.com and
espn.com.)

I'm comitting this patch now.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

17 years ago
*** Bug 80210 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.