If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

code coverage analysis for xpcom.dll

RESOLVED FIXED in Future

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
17 years ago
11 years ago

People

(Reporter: dprice (gone), Unassigned)

Tracking

({helpwanted})

Trunk
Future
x86
Windows 2000
helpwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
According to pure coverave, a profile build (optimized with symbols) of winembed
is only running 49.9% of the code in xpcom.dll  Let's look for dead code and see
if we can't clean it out, or factor it into a new home.

Pure Coverage isn't able to tell us how much of the code is actually being run.
 It only gives us percentages based on the number of lines executed.  Let's put
together something that uses dumpbin to determine exactly what percentage of the
generated code is executed.
(Reporter)

Comment 1

17 years ago
I'm having problems attaching the dumps from pure coverave to the bug.  I think
they are too big.  I'm putting them in /u/dprice/coverage.  I suggest opening
winEmbed_short.txt in excell or one of the .cfy files in pure coverage.
(Reporter)

Comment 2

17 years ago
I'm finding that none of the methods in y:\mozilla\xpcom\ds\nsObserver are being
called.  The methods are:

NS_NewObserver(nsIObserver * *,nsISupports *)
nsObserver::AddRef(void)
nsObserver::AggregatedQueryInterface(nsID const&,void * *)
nsObserver::Create(nsISupports *,nsID const&,void * *)
nsObserver::Internal::AddRef(void)
nsObserver::Internal::QueryInterface(nsID const&,void * *)
nsObserver::Internal::Release(void)
nsObserver::Observe(nsISupports *,WORD const*,WORD const*)
nsObserver::QueryInterface(nsID const&,void * *)
nsObserver::Release(void)
nsObserver::nsObserver(nsISupports *)
nsObserver::~nsObserver(void)
(Reporter)

Comment 3

17 years ago
other files with no methods being called are:
xpcom\ds\nsConjoiningEnumerator.cpp
xpcom\ds\nsProperties.cpp

xpcom\ds\nsSizeOfHandler.cpp

xpcom\ds\nsStatistics.cpp

xpcom\ds\nsSupportsArrayEnumerator.cpp

xpcom\ds\nsTextFormatter.cpp

xpcom\ds\nsVoidBTree.cpp
xpcom\proxy\src\nsProxyEventPrivate.h
xpcom\reflect\xptinfo\xptiZipItem.cpp
xpcom\base\tracerefcnt.cpp
xpcom\io\nsFileSpecStreaming.cpp
xpcom\io\nsFileStream.cpp
xpcom\io\nsIFileStream.cpp
xpcom\reflect\xptcall\src\xptcall.cpp

Comment 4

17 years ago
nsVoidBTree can be removed from the build. It is not used.
(Reporter)

Comment 5

17 years ago
milestone = .9
Target Milestone: --- → mozilla0.9

Comment 6

17 years ago
->0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 7

17 years ago
moving this off the 0.9.1 list
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Comment 8

17 years ago
pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
(Reporter)

Comment 9

16 years ago
-> kandrot 

he's working on this
(Reporter)

Comment 10

16 years ago
this time I mean it...
Assignee: dprice → kandrot

Comment 11

16 years ago
I have the list of xpcom dead code in another bug.  I am future this one that
was assigned to me, since even though there is a lot of code that is not called
that is listed here, most of it can be called by other code, so it is not really
dead.  The info in here should be combined with that other bug, bug 88957.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.3 → Future

Comment 12

16 years ago
reassign all kandrot xpcom bug.
Assignee: kandrot → dougt
Status: ASSIGNED → NEW
Target Milestone: Future → ---

Updated

16 years ago
Blocks: 98284

Comment 13

16 years ago
*** Bug 88957 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Keywords: helpwanted
Target Milestone: --- → mozilla1.0

Comment 14

16 years ago
Do not think that this is required for mozilla 1.0.  

I have spoke with dp about looking at this and he does not think that this would
be a big performance/footprint win.  Moving to future.
Target Milestone: mozilla1.0 → Future

Comment 15

16 years ago
*** Bug 124250 has been marked as a duplicate of this bug. ***

Comment 16

16 years ago
ok, I just filed that other bug.. here's the comment I was going to post:

I have a script that shows you all the unused exports from a dll, given a set of
dlls that will use it.

I'll check the script into tools/module-deps and attach the output from running
it on XPCOM

Comment 17

16 years ago
great!! I am looking forward to checkin out the results!!

Comment 18

16 years ago
Created attachment 68391 [details]
exports that are never used

There are about 2900+ exports from xpcom.dll

This is a list of 1692 exports for which there are no importers

Note, that this includes functions on exported classes that nobody calls.. i.e.
if I say

class NS_COM foo {
  foo();
  foo(const char *);
}

and there is a consumer of foo::foo() but not foo::foo(const char *), then only
the latter will show up. I'll try to tweak the script to show all classes that
are not used in their entirety.

Comment 19

16 years ago
actually, I'm noticing that there are a number of copy-constructors that are
being exported! I wonder if there's any way to eliminate them

Comment 20

16 years ago
Created attachment 68394 [details]
exports that have only one consumer

many functions are only called by one dll.. we could probably clean that up. 

the attached list includes the DLL that imports the function

Comment 21

16 years ago
Created attachment 68660 [details] [diff] [review]
stop exporting nsPersistentFileDescriptor, nsSimpleCharString

For example, I found that there was only one real consumer of
nsPersistentFileDescriptor, and that that consumer didn't even need to be using
it. I also found that nsSimpleCharString shouldn't be used by anyone anywhere.
So I removed NS_COM from both of them.

Can I get reviews here? this kills 115 exports from xpcom (out of a total of
about 2970 exports)

Comment 22

16 years ago
Comment on attachment 68660 [details] [diff] [review]
stop exporting nsPersistentFileDescriptor, nsSimpleCharString

r=dougt.  what about makefile.win?
Attachment #68660 - Flags: review+

Comment 23

16 years ago
there's nothing to change in makefile.win.. all we're doing is not EXPORTING
these classes


in related work, I'm working to make nsSupportsArray and
nsSupportsArrayEnumerator a private class, only creatable via NS_NewISupportsArray()
Status: NEW → ASSIGNED

Comment 24

16 years ago
> there's nothing to change in makefile.win

alecf: someone is confused. The patch shows you removing two files (or at
minimum gutting their contents) and removing references from Makefile.in and
MANIFEST. I'm with dougt: what about makefile.win?

Comment 25

16 years ago
Comment on attachment 68660 [details] [diff] [review]
stop exporting nsPersistentFileDescriptor, nsSimpleCharString

oh, I see! I forgot that I was removing nsFileSpecStreaming.h in this patch. 

I've removed references too it in my local tree.. sorry about that. I was using
win32 gmake and thus wasn't using makefile.win :)

so, one more try, can I get a super-review on that?

Comment 26

16 years ago
Created attachment 68883 [details] [diff] [review]
stop exporting nsSupportsArray

this cuts down on a bunch more exports.
I had to poke the tree a bit because a few people were using nsSupportsArray
directly.
I tried to do my best to match the style of the code around each usage.

reviews?

Comment 27

16 years ago
Oh, I'm noticing some overlap between that patch and the patch for bug 123041 -
I'll land the other one first, with the changes that overlap into this patch
(since it's safe to convert from nsSupportsArray to nsISupportsArray without the
changes to xpcom/ds/nsSupportsArray.h)

Comment 28

16 years ago
Created attachment 69144 [details] [diff] [review]
export report - from the script

wow, this is very interesting. Check out the output from this script, which
lists exports with little or no consumers

The output is divided into two sections - one for methods (Which includes
classes) and one that focuses on classes, showing classes that are never used,
etc. This should make things MUCH easier to clean up.

Comment 29

16 years ago
the script is in tools/module-deps/find-unused-exports.pl

use the -c option to get the "class" results.

Comment 30

16 years ago
Created attachment 69154 [details] [diff] [review]
just the classes that are never consumed

I filtered out the classes that are never used - this is incredible! Check out
this garbage - half of these can be immediately ripped from XPCOM

Comment 31

16 years ago
Comment on attachment 68883 [details] [diff] [review]
stop exporting nsSupportsArray

looks good.
Attachment #68883 - Flags: review+

Comment 32

16 years ago
Created attachment 69251 [details]
complete class report

ok, here's a much more interesting report. It is a list of classes who match at
least one of the following criteria
1) never imported by any dll
2) only used by one dll
3) less than 50% of the class's methods are imported by anyone

It also lists suggestions (like "move XXX class into XXX.dll")
Attachment #69144 - Attachment is obsolete: true
Attachment #69154 - Attachment is obsolete: true
Cc'ing jag -- see alecf's wonderful attachment 69251 [details], especially the ns*Str*
stuff (nsStr is not used).

/be

Comment 34

16 years ago
would someone PLEASE super review these? I've e-mailed various people, and
gotten no response.

Comment 35

16 years ago
add shaver for possible sr=

Updated

16 years ago
Attachment #68394 - Attachment is patch: false
Comment on attachment 68660 [details] [diff] [review]
stop exporting nsPersistentFileDescriptor, nsSimpleCharString

Bring the toing-and-froing!

(Also, hem and haw on the Mac first, if you haven't yet.  Then: sr=shaver.)
Attachment #68660 - Flags: superreview+

Comment 37

16 years ago
Comment on attachment 68660 [details] [diff] [review]
stop exporting nsPersistentFileDescriptor, nsSimpleCharString

sr=jag

Comment 38

16 years ago
Comment on attachment 68883 [details] [diff] [review]
stop exporting nsSupportsArray

rs=jag
Attachment #68883 - Flags: superreview+
jband, you gonna look at attachment 68883 [details] [diff] [review], the xpcprivate.h change and related?
 Instead of a nested nsSupportsArray member, we have a separate allocation and a
(smart, of course!) pointer.  Is this gonna hurt our un-girlish footprint figure?

/be

Comment 40

16 years ago
Yeah, sorry. I gave it a quick once over before, sighed quietly, and neglected
to post any comments.

This stuff looks OK to me. The changed use in xpconnect is not on any critical
path and involves only short-lived enumerator objects. It won't hurt anyone. 

I do sort of wonder if we are going too far with modularity by saying that *no*
components can be closely enough coupled with the xpcom core to be able to use
these containers without the extra space/time overhead.

I'm also concerned that we might get into trouble by trying to pull stuff out of
the core and push it to the periphery in cases where it *appears* that only one
client is using some bit of code or another. This might cause more harm than good.

Do you guys really imagine that we are going to ferret out that much dead code
here? Or mostly just find bits that are used internally, but don't need to be
exported? Are the projected savings very big in the latter case? Because,
really, I have very little faith in the assumptions of the original posting to
the bug. Getting rid of unnecessary exports may help the linker trim some
unnecessary code. But, most of the stuff I see in the first attachment list are
implmentations of interface methods. The linker won't whack them for us. And, it
looks like the later (interesting) lists don't show the uses within xpcom. SO,
htye help with showing what might not need exporting, but not so much with what
might not need to exist at all.

Comment 41

16 years ago
well, my original goal was to go after classes like nsFileSpec and their helper
classes.. but then I noticed how many exports nsSupportsArray had, and figured
it was kind-of-low-hanging-fruit.

If people seriously object to the nsSupportsArray stuff, I can back off of it...
I guess I'm looking at 2900+ exports and trying to trim out the noise...

Comment 42

16 years ago
Alec, I'm not against it. Just wondering how much the expected gain is and where
you might be going with this bug. If you try to hide nsVoidArray folks may start
chasing you with weapons :)

Comment 43

16 years ago
Created attachment 69656 [details] [diff] [review]
remove nsVoidBTree from the build

nobody is using nsVoidBTree - this patch removes it from the makefiles
(I'll remove the files as well, but didn't include that in the patch)
Comment on attachment 69656 [details] [diff] [review]
remove nsVoidBTree from the build

sr=shaver.
Attachment #69656 - Flags: superreview+

Comment 45

16 years ago
Comment on attachment 69656 [details] [diff] [review]
remove nsVoidBTree from the build

r=jag
Attachment #69656 - Flags: review+

Comment 46

16 years ago
Created attachment 69685 [details] [diff] [review]
Why be half-assed? removing LOTS of dead code

Ok, this is a much more comprehensive patch. it removes lots of dead classes,
as well as removing NS_COM from lots of other classes. I think this removes
150+ exports from xpcom.dll, as well as reducing it in size by about 40k (not
much, but it was easy)
Attachment #69656 - Attachment is obsolete: true

Comment 47

16 years ago
Comment on attachment 69685 [details] [diff] [review]
Why be half-assed? removing LOTS of dead code

r=jag
Attachment #69685 - Flags: review+
Comment on attachment 69685 [details] [diff] [review]
Why be half-assed? removing LOTS of dead code

sr=shaver.
Attachment #69685 - Flags: superreview+

Comment 49

16 years ago
Comment on attachment 69685 [details] [diff] [review]
Why be ****? removing LOTS of dead code

Removing the two lines about PROG19 without removing the one that requires it
should result in a build failure :-) -- oh, it's so much fun actually knowing
these files.

Since you're making core changes I hope you try a clobber build to make sure
they work.
Attachment #69685 - Flags: needs-work+

Comment 50

16 years ago
Comment on attachment 69685 [details] [diff] [review]
Why be half-assed? removing LOTS of dead code

whatever. 
I've tested these, and they work. the $(PROG19) line just evaluated to an empty
string, so it didn't result in a build failure. 

I've removed the final PROG19 from the makefile.win, and so the patch, with
that change, is what will land if I ever get an open tree.
Attachment #69685 - Flags: needs-work+

Comment 51

16 years ago
over to alec, he has clearly been working on this.
Assignee: dougt → alecf
Status: ASSIGNED → NEW
Target Milestone: Future → ---

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0

Comment 52

16 years ago
Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla1.0 → mozilla1.2

Comment 53

16 years ago
With Build 2002050807 before the first Mozilla start during updating Mozilla and
during about:plugins i get a error message about a missing entry point nsAString
(?Mid@nsAString@@QBETAAV1@II@Z) in xpcom.dll.

Comment 54

16 years ago
sounds like you've got a mixed up build - that has nothing to do with this bug.

Updated

16 years ago
Depends on: 154265

Updated

16 years ago
Depends on: 154280

Updated

15 years ago
Priority: -- → P3
Target Milestone: mozilla1.2alpha → mozilla1.2beta

Comment 55

15 years ago
mozilla 1.1alpha is more or less done, so I'm moving non-critical mozilla1.2beta
bugs out to the next milestone to make room for the mozilla1.1alpha bugs that
didn't make it.
Target Milestone: mozilla1.2beta → mozilla1.3alpha

Comment 56

15 years ago
moving non-critical 1.3alpha bugs to 1.4alpha
Target Milestone: mozilla1.3alpha → mozilla1.4alpha

Comment 57

15 years ago
futuring this stuff. We've done a pretty good job with this on the minimo
branch, which should be landing next week.
Target Milestone: mozilla1.4alpha → Future

Updated

12 years ago
QA Contact: kandrot → alecf
Assignee: alecf → nobody
Status: ASSIGNED → NEW
QA Contact: alecf → xpcom

Comment 58

11 years ago
Comment on attachment 69685 [details] [diff] [review]
Why be ****? removing LOTS of dead code

mozilla/xpcom/build/dlldeps.cpp 	1.71
mozilla/xpcom/components/nsComponentManagerUtils.h 	3.18
mozilla/xpcom/components/nsIServiceManagerUtils.h 	3.2
mozilla/xpcom/ds/MANIFEST 	3.46
mozilla/xpcom/ds/Makefile.in 	1.62
mozilla/xpcom/ds/makefile.win 	1.62
mozilla/xpcom/ds/nsSupportsArrayEnumerator.h 	1.6
mozilla/xpcom/io/MANIFEST 	3.23
mozilla/xpcom/io/nsBinaryStream.h 	1.7
mozilla/xpcom/io/nsFastLoadFile.h 	3.10
mozilla/xpcom/io/nsFastLoadService.h 	3.6
mozilla/xpcom/io/nsFileStream.cpp 	1.41
mozilla/xpcom/io/nsFileStream.h 	1.46
mozilla/xpcom/macbuild/xpcomPPC.xml 	1.11
mozilla/xpcom/tests/Makefile.in 	1.71
mozilla/xpcom/tests/makefile.win 	3.42

the following isn't listed in this patch:
mozilla/xpcom/io/nsStorageStream.h 	1.7
mozilla/xpcom/glue/nsServiceManagerUtils.h 	3.2
Attachment #69685 - Attachment is obsolete: true

Comment 59

11 years ago
alecf did work on this. any new work should use a new bug.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.