Closed
Bug 94243
Opened 23 years ago
Closed 23 years ago
nsVoidArray/nsSupportsArray: followup changes to users
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.4
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: perf)
Attachments
(4 files)
12.96 KB,
patch
|
Details | Diff | Splinter Review | |
15.41 KB,
patch
|
Details | Diff | Splinter Review | |
17.28 KB,
patch
|
Details | Diff | Splinter Review | |
3.30 KB,
patch
|
Details | Diff | Splinter Review |
This is a followup to bug 90545, incorporating the changes to files other than nsVoidArray/nsSupportsArray. Cleans up usage: use the right functions, avoid extra allocations, use nsAutoVoidArray where it's totally non-controversial (see bug 92573, bug 92575, and bug 92576 for member variable changes). Modifies xpcjsruntime.cpp to use SizeTo() to avoid allocations during js GC passes (already has a=dbradley and jband in 90545). I made a new bug to cover these modifications only, to simplify the work of reviewers, reduce people's email load, etc.
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
We need reviews on this. I'm willing to drop individual fixes from the patch if there's objection, but I'd like to get this into the tree. This fixes a bunch of misuses of arrays, and includes a performance improvement for JS garbage collection among others.
Target Milestone: --- → mozilla0.9.4
Assignee | ||
Comment 5•23 years ago
|
||
BTW, the "putting ViewManager into an nsCOMPtr" stuff in nsGfxListControlFrame.cpp wasn't originally intended to be in this patch; I picked it up when updating for bit-rot. I was cleaning up inconsistent uses of ViewManager and one happened to be here.
Comment 6•23 years ago
|
||
r=pierre for the changes in nsCSSDeclaration.cpp
Comment 7•23 years ago
|
||
Why the null vm check? The old code was happy to count on the view manager pointer being non-null. Nits: Here and elsewhere, it looks like you're indenting the comments using tabs that expand to 4-space stops. if (!gGlobalList) { - gGlobalList = new nsFontNodeArray(); + // This may well expand further (families * sizes * styles?), but it's + // only created once. + gGlobalList = new nsFontNodeArray; if (!gGlobalList) { return NS_ERROR_OUT_OF_MEMORY; Also, prevailing stylle for space-after-comma-in-arglist should be followed, e.g. in this code: - mFontMetrics.ReplaceElementAt(metrics, 0); + mFontMetrics.MoveElement(0,cnt); Pick these nits, and sr=brendan@mozilla.org. Let's keep this on the 0.9.4 radar. /be
Keywords: mozilla0.9.4
Comment 8•23 years ago
|
||
Yup, tabs -- see how pasting them into bugzilla's textarea caused them to become magnified horizontally. Please don't put tabs in files. Use vim with |et| set instead of vi, configure Emacs to respect the modeline, etc. Thanks, /be
Assignee | ||
Comment 9•23 years ago
|
||
Ok. Note that files with tab-width=4 (nsViewManager.cpp, the mailnews file, nsSHEntry.cpp, etc) generally have tabs scattered through them already. I removed tabs from my patches. Fixed ',' usage - some files are one way, some are the other (though space-after-comma seems most prevalent), but I'd changed the style in a couple of places. As for viewmanager - 2/3 of the viewmanager references in the codebase use nsCOMPtr's and check that it exists, 1/3 don't. Also, we were crashing on missing viewmanagers due to leaking presshells at one point, and in porkjockeys it was stated that NS_RELEASE should only be being used where it's really required - usually things should be in nsCOMPtr so they don't leak by mistake. In fact, in that file (nsGFXListControlFrame.cpp), the place I changed it uses nsCOMPtr's everywhere except for a large commented out block, which is where most of those changes are (except for one "if (!viewmanager) return NS_ERROR_FAILURE;" because of the aforementioned paranoia about leaking presshells). new patch to be posted momentarily.
Assignee | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
Patches should eliminate tabs, per the indent-tabs-mode: nil part of the Emacs modeline -- or we shall never be free of tabs. Thanks for straightening out that view manager code. Indeed, nsCOMPtr-izing is almost always the right thing. I just was wondering whether you knew of a bug where the result of the getter could be null. /be
Assignee | ||
Comment 12•23 years ago
|
||
From nsViewManager.cpp: /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: true; c-basic-offset: 4 -*- Shall I file a bug against all files with indent-tabs-mode: true? (15 of them from a quick lxr regexp, but I didn't search for modelines that were missing indent-tabs-mode:). http://lxr.mozilla.org/seamonkey/search?string=tabs-mode%3A+%5B%5En%5D®exp=on I have sr=brendan, and r=pierre for the CSS code (plus ok on the js stuff from dbradley & jband). I need r= for the rest and then we can ask drivers for a=.
Comment 13•23 years ago
|
||
All except the ones that I had to fight with rjc over ;-). (I think I also set something like ``stroustrup'' in those...) I don't think you need to file a bug -- fix 'em and call me the super-reviewer, and I'll take any heat for you.
Comment 14•23 years ago
|
||
How about |aNodes->IndexOf(node) >= 0|? That can usually be a single test-and-branch instruction, as opposed to a load followed by a compare... - PRInt32 n = aNodes->Count(); - for (PRInt32 j = 0; j < n; j++) { - if (aNodes->GetElement(j) == node) { - found = 1; - } - } + found = (aNodes->IndexOf(node) != -1); Do you need to cast to |void*| here? - mSelectionCache->InsertElementAt((void*)selected, i); + mSelectionCache->AppendElement((void*)selected); Do we actually have cases where we'd fail to get the view manager? Or is over-paranoid COM disease creep? parentView->GetViewManager(*getter_AddRefs(viewManager)); + if (!viewManager) + return NS_ERROR_FAILURE; + sspitzer should look at the nsMsgFilterList.cpp changes, although they look fine to me. Do you need to cast to |void*|? + + // This implicitly extends the array to include aOffset + mChildren.ReplaceElementAt((void *) aChild, aOffset); + So...r= or sr=waterson, modulo the above nits and getting sspitzer to bless nsMsgFilterList.cpp changes.
Assignee | ||
Comment 15•23 years ago
|
||
>How about |aNodes->IndexOf(node) >= 0|? Sure. >nsGfxListControlFram.cpp: >Do you need to cast to |void*| here? No, we don't; it was in the original code. However, all the uses of nsVoidArray in that file use it; it would be strange to remove it from this one and leave all the others. >Do we actually have cases where we'd fail to get the view manager? Or is >over-paranoid COM disease creep? Both. Normally we shouldn't ever fail to get it. However, if a presshell leaks (as they have in the past), it can happen (which was how I ran into this issue). Since 2/3 of the code uses nsCOMPtrs, and it allows us to get rid of NS_RELEASE's, and the cost of a few "if (ptr) ptr->..." instancea is minimal (should be simply a conditional branch after the pointer is loaded, maybe a test but not too likely), it seems reasonable. Leaking is one thing, but crashing due to a leak is not good. >nsSHEntry.cpp: >Do you need to cast to |void*| here? No, it was in the original code. In this case I'll remove it. I'll get sspitzer to review the mailnews stuff. Thanks!
Assignee | ||
Comment 16•23 years ago
|
||
sspitzer is on vacation, asking mscott for r=
Assignee | ||
Comment 17•23 years ago
|
||
By email got: looks good, thanks, r/sr =bienvenu - david Requesting a= from drivers
Comment 18•23 years ago
|
||
a=asa on behalf of drivers.
Assignee | ||
Comment 19•23 years ago
|
||
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 20•23 years ago
|
||
Reopening bug... Xlib toolkit wasn't patched. I am going to file a GTK+ toolkit --> Xlib toolkit patch ...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 21•23 years ago
|
||
Comment 22•23 years ago
|
||
Filed patch, requesting r=/sr=/a= ...
r=dbaron on attachment 47201 [details] [diff] [review]
Assignee | ||
Comment 24•23 years ago
|
||
I didn't touch xlib because I don't normally build it, and I was targetting spots that showed up frequently when booting or loading a page in my stats. r=rjesup@wgate.com, for what it's worth
Status: REOPENED → ASSIGNED
Comment 25•23 years ago
|
||
sr=jst
Comment 26•23 years ago
|
||
a=asa .
Assignee | ||
Comment 27•23 years ago
|
||
xlib changes committed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•