93043 - Fastload landing increased size of principals structs

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Daniel Bratell]

The leak figures on Tinderbox increased after the fastload landing:


--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsXULPDGlobalObject                              56          -
nsSystemPrincipal                                56       7.69%
nsCodebasePrincipal                              64       6.67%
TOTAL                                           176
--NEW-BLOAT-----------------------------------bloat------bloat%-----------------------
nsFastLoadService                                36          -
nsThreadPoolRunnable                             32     100.00%
nsXULPrototypeCache                             188      30.56%
nsThread                                        140      25.00%
nsXULPrototypeDocument                         1344      20.00%
nsJARURI                                       5376      14.38%
nsSystemPrincipal                                56       7.69%
nsCodebasePrincipal                            1216       6.67%
nsAggregatePrincipal                            684       5.56%
nsJSCID                                        3536       4.62%
nsROCSSPrimitiveValue                          5096       2.08%
nsComputedDOMStyle                             3200       2.04%
XPCWrappedNativeProto                          3192       1.79%
xptiInterfaceInfo                            103572       1.05%
nsFactoryEntry                                57816       0.63%
nsGenericFactory                               4420       0.45%
nsHashtable                                   69608       0.06%
nsCParserNode                                861600       0.01%
TOTAL                                       1121112

Comment 1

[Brendan Eich [:brendan]]

I bet the principals are from the XUL script cache.  Since FastLoad is pref'd
off by default, I am not sure why nsFastLoadService is leaking.  Daniel, did you
enable the pref (set nglayout.debug.disable_xul_fastload to false)?

/be

Comment 2

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

No, it's just reporting that the size of the principals is 4 bytes bigger.

The only new leak here is the nsXULPDGlobalObject.  We shouldn't be leaking
that, though...

Comment 3

[Brendan Eich [:brendan]]

I interposed nsISerializable between nsIPrincipal and nsISupports on each
principal type's inheritance chain -- didn't that save another vtbl ptr?  I did
not add data members to any principals concrete class.

/be

Comment 4

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

It's not getting GC'ed.  The leaked GC roots at shutdown look like they're just
the usual |nsXULPrototypeScript::mJSObject|s, although maybe they aren't usual
and I've been looking at too many leaks.

Comment 5

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

So I can find it, fastload is bug 68045.

Comment 6

[Brendan Eich [:brendan]]

So if the XUL prototype cache survives past XPCOM shutdown, we'll leak the
XULPDGlobalObject, and I bet that's now happening.  But why?

/be

Comment 7

[Brendan Eich [:brendan]]

Probably a dup of bug 93146, although why did the principals structs bloat?

/be

Comment 8

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

Yes, the leaks were a dup of bug 93146, so retitling this bug.

Comment 9

[Brendan Eich [:brendan]]

I added an encode function pointer to JSPrincipals, and forgot. That is what
made the nsBasePrincipal subclasses grow by four bytes. Closing as INVALID, not
a bug (not enough bloat to fear).

/be

Comment 10

[Brendan Eich [:brendan]]

I was mindlessly following the pattern established long ago of adding function
pointers to JSPrincipals, but there is no opportunity to vary principals->encode
among several functions, or have several principals each have its own function
pointer different from the others.  The principalsDecoder member of JSRuntime is
another strong clue that only one serialization system at a time can use the
engine's API.

Patch coming up to recover the wasted space.

BTW, the other function pointers in JSPrincipals never vary, and could be moved
to a single "ops" struct (poor man's C vtbl) or to the JSRuntime.  But given the
age of those members, I'm concerned someone out there is using them, and may
even be married to the JSPrincipals API.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed fix: remove encode from JSPrincipals, make rt->principalsDecoder a transcoder

Comment 12

[Brendan Eich [:brendan]]

Attachment: diff -wu version of patch

Comment 13

[Brendan Eich [:brendan]]

Review craved.  The diff -wu is easier to read, as in fusing
nsEncodeJSPrincipals and nsDecodeJSPrincipals in caps/src/nsJSPrincipals.cpp, I
had to indent the code more, and avoid early returns.

/be

Comment 14

[Johnny Stenback (:jst)]

r/sr=jst

Comment 15

[Mike Shaver (:shaver emeritus)]

r=shaver, with the s/Xcode/Transcode/g change from IRC.

Comment 16

[Chris Waterson]

I guess I mentioned this on IRC last night but it must've been missed. I don't
see where the space reduction is coming from in this patch?

Comment 17

[Chris Waterson]

Never mind: shaver showed me.

Comment 18

[Brendan Eich [:brendan]]

Fixed.

/be