744103 - eliminate the cycle collector's vestigial language agnosticism

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

nsXPConnect::ToParticipant does a redundant check, then returns XPConnect.

We also end up checking in NoteScriptChild that JS is a valid langID, that has a runtime.

With a little bit of cleanup, we can avoid all of this, while still leaving in the vestigial support for other languages.

We could also avoid the language validity checks in NoteRoot, but that would require adding new variants of NoteRoot which is kind of gross, and they aren't virtual so they probably are not that bad.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we remove the vestigial support for other languages?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I thought pyxpcom is still alive.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Does it have a working (or even non-working cycle collector)?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Don't know, but I don't see why it couldn't have.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: WIP

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Can we remove the vestigial support for other languages?

Possibly, but that will require modifying all Trace functions.  Mostly hidden behind macro gunk, of course.  I thought I saw some place where it might be a problem, with some kind of script context, but I don't see it now.

Comment 7

[Andrew McCreight [:mccr8]]

Oh that explains it!  Bug 738380 removed the problem, but I haven't updated my tree since that landed.  That makes me feel better.  Maybe it is doable after all.

Comment 8

[Andrew McCreight [:mccr8]]

Ugh, the one place where we can't turn NoteScriptChild to just always be JS is nsXULPrototypeScript, which in theory could be other things.  I think it is always only JS, but it will require a bit of patching to get to the point where that could be done.

Comment 9

[Andrew McCreight [:mccr8]]

I think I'll just add a local check in nsXULPrototypeScript, and just do nothing if it isn't JS.  Later if 744322 gets fixed, the check can be removed.

I looked at PyXPCOM.  They do use some of the script context things (but not all of them?) but I don't see any kind of cycle collector integration, so it should be okay to remove this support.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: remove langid argument from NoteScriptChild (fix to JS)

This is on top of the other patch, though it makes more sense to fold them together.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 2: remove fake language agnosticism of NoteRoot

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 1: remove vestigial language agnosticism of NoteScriptChild

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 2: remove vestigial language agnosticism of NoteRoot

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 3 - remove nsCycleCollectionLanguageRuntime

In this patch, I eliminate nsCycleCollectionLanguageRuntime and the array of runtimes the cycle collector carries around.  Instead, there's a pointer to a nsCycleCollectionJSRuntime, which gets all of the methods of nsCycleCollectionLanguageRuntime that we need for JS.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 4: remove remaining internal langID things

This removes the mLangID field from DEBUG_CC PtrInfo, and all of the passing around of langIDs to support that.  The only place it is used is in "pinfo->mLangID == nsIProgrammingLanguage::CPLUSPLUS", but we know that anything in the purple buffer must be C++, so the check is redundant.

With all of these patches, JAVASCRIPT, CPLUSPLUS and langID are nowhere to be found in the nsCycleCollector.cpp.

Comment 16

[Andrew McCreight [:mccr8]]

I downloaded PyXPCOM, and the sources don't contain the string "nsCycleCollectionLanguageRuntime" anywhere, which I think means they can't have implemented cycle collection, so they can't be reporting any Python objects to the CC, so this shouldn't affect them.

Comment 17

[Andrew McCreight [:mccr8]]

We don't really need bug 744332 for this, as I have just patched around that one problem.  There aren't many script prototype objects in the CC graph, so it shouldn't be a big deal.

Comment 18

[Andrew McCreight [:mccr8]]

try run on Linux: https://tbpl.mozilla.org/?tree=Try&rev=915c3dbfe6ce

Interestingly it triggers an assertion during make check in the build process:

Should Release and destroy one |TestRefObject|:
  Release to 0 on TestRefObject 0x1ff24ab0.
  Destroying TestRefObject 0x1ff24ab0.
Assertion failure: mJSParticipant, at /builds/slave/try-lnx64-dbg/build/xpcom/base/nsCycleCollector.cpp:1719
/bin/sh: line 1:  8428 Aborted                 XPCOM_DEBUG_BREAK=stack-and-abort /builds/slave/try-lnx64-dbg/build/obj-firefox/dist/bin/run-mozilla.sh ../../dist/bin/$f
make[2]: *** [check] Error 134
make -C external check
make[3]: Entering directory `/builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/tests/external'
make[3]: Nothing to be done for `check'.

During the GCGraphBuilder constructor, apparently XPConnect isn't set up.  Maybe this test doesn't use it somehow?  I'll have to be more generous with how I detect that.  I guess we do need to be able to run the CC without JS being active.

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 1: store nsCycleCollectionJSRuntime separately

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: part 2: change NoteScriptChild to NoteJSChild

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: part 3: change NoteRoot to NoteJSRoot and NoteNativeRoot

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: part 4: remove nsCycleCollectionLanguageRuntime

Comment 23

[Andrew McCreight [:mccr8]]

Attachment: part 5: remove remaining internal langIDs from CC

Comment 24

[Andrew McCreight [:mccr8]]

part 1: Store a copy of the JS CC runtime pointer separately from mRuntimes.  mRuntimes is going to be removed in part 4.  Doing this first simplifies part 2.

part 2: This is the largest patch, but it is mostly mechanical removal of the langID argument from various callbacks.  In addition, nsXPConnect.cpp, xpcprivate.h, nsCycleCollector.h mostly just define GetParticipant.  nsCycleCollector.cpp has a number of changes, of course.

Comment 25

[Andrew McCreight [:mccr8]]

These patches eliminate a number of checks, so it should have at least some impact on CC speed.  On the other hand, the checks always succeed, so maybe it won't matter much.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 618354 [details] [diff] [review]
part 2: change NoteScriptChild to NoteJSChild

Please ask still PyXPCOM devs whether they are or would like to use this stuff.

Comment 27

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #26)
> Please ask still PyXPCOM devs whether they are or would like to use this
> stuff.

Yeah, it wouldn't be a big deal to just leave these hooks around if there is actually somebody who might want them, and still use the "fast path" things for JS.

Comment 28

[Andrew McCreight [:mccr8]]

I wrote to Todd Whiteman and he said they probably aren't going to work on Python-CC integration in the next 12 months, and the chances weren't high they would ever do it.  I'll go ahead and land this when the tree opens again (though I do wonder if this could reduce memory usage by PGO ;).  It wouldn't be that hard to add some special support for an additional scripting language without reverting most of these changes to the handling of JS.

Comment 29

[Andrew McCreight [:mccr8]]

Thanks for the reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fca7e83d16b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e637e514910b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0956a736bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d47a68dbe56
https://hg.mozilla.org/integration/mozilla-inbound/rev/c52b99b9a15d

Landed with some minor rebasing in XPCWrappedNative probably due to CPG.

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fca7e83d16b
https://hg.mozilla.org/mozilla-central/rev/e637e514910b
https://hg.mozilla.org/mozilla-central/rev/6b0956a736bc
https://hg.mozilla.org/mozilla-central/rev/1d47a68dbe56
https://hg.mozilla.org/mozilla-central/rev/c52b99b9a15d