Closed Bug 94243 Opened 23 years ago Closed 23 years ago

nsVoidArray/nsSupportsArray: followup changes to users

Categories

(Core :: XPCOM, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9.4

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: perf)

Attachments

(4 files)

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.
I'll take this.
Assignee: kandrot → rjesup
Depends on: 90545
Status: NEW → ASSIGNED
Keywords: patch, perf
Blocks: 92256
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
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.
r=pierre for the changes in nsCSSDeclaration.cpp
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
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
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.
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
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&regexp=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=.
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.
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.
>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!

sspitzer is on vacation, asking mscott for r=
By email got:

looks good, thanks, r/sr =bienvenu
- david

Requesting a= from drivers
a=asa on behalf of drivers.
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Reopening bug... Xlib toolkit wasn't patched. I am going to file a GTK+ toolkit
--> Xlib toolkit patch ...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Filed patch, requesting r=/sr=/a= ...
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
sr=jst
a=asa .
xlib changes committed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: