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.
Can we remove the vestigial support for other languages?
I thought pyxpcom is still alive.
Does it have a working (or even non-working cycle collector)?
Don't know, but I don't see why it couldn't have.
Created attachment 613743 [details] [diff] [review]
(In reply to Kyle Huey [:khuey] (firstname.lastname@example.org) 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.
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.
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.
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.
Created attachment 614054 [details] [diff] [review]
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.
Created attachment 614062 [details] [diff] [review]
part 2: remove fake language agnosticism of NoteRoot
Created attachment 614113 [details] [diff] [review]
part 1: remove vestigial language agnosticism of NoteScriptChild
Created attachment 614122 [details] [diff] [review]
part 2: remove vestigial language agnosticism of NoteRoot
Created attachment 614140 [details] [diff] [review]
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.
Created attachment 614164 [details] [diff] [review]
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.
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.
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.
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: *** [check] Error 134
make -C external check
make: Entering directory `/builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/tests/external'
make: 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.
Created attachment 618353 [details] [diff] [review]
part 1: store nsCycleCollectionJSRuntime separately
Created attachment 618354 [details] [diff] [review]
part 2: change NoteScriptChild to NoteJSChild
Created attachment 618355 [details] [diff] [review]
part 3: change NoteRoot to NoteJSRoot and NoteNativeRoot
Created attachment 618357 [details] [diff] [review]
part 4: remove nsCycleCollectionLanguageRuntime
Created attachment 618358 [details] [diff] [review]
part 5: remove remaining internal langIDs from CC
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.
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 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.
(In reply to Olli Pettay [:smaug] from comment #26)
> Please ask still PyXPCOM devs whether they are or would like to use this
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.
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.
Thanks for the reviews!
Landed with some minor rebasing in XPCWrappedNative probably due to CPG.