106792 - remove nsISizeOfHandler

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should remove nsIAtom::SizeOf.  Any debugging code that's using it is
probably way over-accounting for the atoms anyway, since they're shared.  That's
why trace-malloc and friends are better.

Comment 1

[Doug Turner (:dougt)]

Can we also get rid of nsISizeOfHandler?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd *love* to (since modifying layout data structures always requires spending
double the time modifying the SizeOf methods), but attinasi seems to think that
people still use it and wants it to stay (at least when I asked back in July),
although maybe we should be evangelizing trace-malloc more.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, changing this bug into being on removing nsISizeOfHandler.  trace-malloc is
far better (especially since it doesn't contain bias introduced by programmer
error in writing SizeOf methods) and requires no code maintainance.

Any objections?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

cc:ing a few more people to see if they care about nsISizeOfHandler.

The one objection I heard is from Bernd, who suggested that nsISizeOfHandler is
an easier way to see how much a specific change helped.  However, I'd think you
could easily figure that out (if you really need accurate numbers) using a
printf printing the sizeof() of the object and a simple counter (just find the
max of a static gInstanceCnt variable) to see how many instances of the object
were created at one time.  In most cases I'd think approximate numbers for that
kind of thing are good enough, though.

Comment 6

[Johnny Stenback (:jst)]

I say kill it, Kill it, KILL IT! IMO nsISizeOfHandler is useless since most
people (me included) don't take the time to make sure it does the right thing
when making changes to classes, and I don't think that'll change...

Comment 7

[Bernd]

as I wrote in my email my objection was a recall what Marc said and I have no
interest in keeping it.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

merged

Comment 9

[Doug Turner (:dougt)]

dbaron, do you see any footprint savings?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's practically all debug code.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113300 [details] [diff] [review]
patch

Ah, the smell of fresh code removal on a Saturday morning... it warms the
heart.	;)

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 113300 [details] [diff] [review]
patch

<smile size='huge'>
sr=jst
</smile>

:-)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-02-22 07:34 PST (with dlldeps.cpp later).

Comment 14

[Alec Flett]

I hate to bring this up, but wasn't nsIAtom frozen?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's not marked frozen, and it needs other changes before it is (it shouldn't be
scriptable, since wrapping can break it).

Comment 16

[Alec Flett]

sorry my bad.. I could have SWORN this was frozen..