Last Comment Bug 744103 - eliminate the cycle collector's vestigial language agnosticism
: eliminate the cycle collector's vestigial language agnosticism
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew McCreight [:mccr8]
:
:
Mentors:
Depends on: 678615 738380 744088 744332
Blocks: 698919 CleanupCC
  Show dependency treegraph
 
Reported: 2012-04-10 12:03 PDT by Andrew McCreight [:mccr8]
Modified: 2012-05-20 12:35 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (8.01 KB, patch)
2012-04-10 13:39 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
remove langid argument from NoteScriptChild (fix to JS) (28.40 KB, patch)
2012-04-11 10:24 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 2: remove fake language agnosticism of NoteRoot (10.59 KB, patch)
2012-04-11 10:54 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: remove vestigial language agnosticism of NoteScriptChild (33.00 KB, patch)
2012-04-11 12:36 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 2: remove vestigial language agnosticism of NoteRoot (11.20 KB, patch)
2012-04-11 12:46 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 3 - remove nsCycleCollectionLanguageRuntime (20.26 KB, patch)
2012-04-11 13:23 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 4: remove remaining internal langID things (10.95 KB, patch)
2012-04-11 14:09 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: store nsCycleCollectionJSRuntime separately (6.05 KB, patch)
2012-04-25 10:54 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 2: change NoteScriptChild to NoteJSChild (32.41 KB, patch)
2012-04-25 10:55 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 3: change NoteRoot to NoteJSRoot and NoteNativeRoot (10.98 KB, patch)
2012-04-25 10:55 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 4: remove nsCycleCollectionLanguageRuntime (15.73 KB, patch)
2012-04-25 10:56 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 5: remove remaining internal langIDs from CC (10.90 KB, patch)
2012-04-25 10:57 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-04-10 12:03:03 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-10 12:08:46 PDT
Can we remove the vestigial support for other languages?
Comment 2 Olli Pettay [:smaug] (reviewing overload) 2012-04-10 12:09:56 PDT
I thought pyxpcom is still alive.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-10 12:11:41 PDT
Does it have a working (or even non-working cycle collector)?
Comment 4 Olli Pettay [:smaug] (reviewing overload) 2012-04-10 12:13:40 PDT
Don't know, but I don't see why it couldn't have.
Comment 5 Andrew McCreight [:mccr8] 2012-04-10 13:39:46 PDT
Created attachment 613743 [details] [diff] [review]
WIP
Comment 6 Andrew McCreight [:mccr8] 2012-04-10 13:40:50 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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] 2012-04-10 14:07:24 PDT
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] 2012-04-10 16:40:57 PDT
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] 2012-04-11 08:48:10 PDT
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] 2012-04-11 10:24:26 PDT
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.
Comment 11 Andrew McCreight [:mccr8] 2012-04-11 10:54:14 PDT
Created attachment 614062 [details] [diff] [review]
part 2: remove fake language agnosticism of NoteRoot
Comment 12 Andrew McCreight [:mccr8] 2012-04-11 12:36:26 PDT
Created attachment 614113 [details] [diff] [review]
part 1: remove vestigial language agnosticism of NoteScriptChild
Comment 13 Andrew McCreight [:mccr8] 2012-04-11 12:46:55 PDT
Created attachment 614122 [details] [diff] [review]
part 2: remove vestigial language agnosticism of NoteRoot
Comment 14 Andrew McCreight [:mccr8] 2012-04-11 13:23:23 PDT
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.
Comment 15 Andrew McCreight [:mccr8] 2012-04-11 14:09:45 PDT
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.

With all of these patches, JAVASCRIPT, CPLUSPLUS and langID are nowhere to be found in the nsCycleCollector.cpp.
Comment 16 Andrew McCreight [:mccr8] 2012-04-11 14:42:30 PDT
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] 2012-04-12 12:24:45 PDT
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] 2012-04-12 14:13:54 PDT
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] 2012-04-25 10:54:39 PDT
Created attachment 618353 [details] [diff] [review]
part 1: store nsCycleCollectionJSRuntime separately
Comment 20 Andrew McCreight [:mccr8] 2012-04-25 10:55:12 PDT
Created attachment 618354 [details] [diff] [review]
part 2: change NoteScriptChild to NoteJSChild
Comment 21 Andrew McCreight [:mccr8] 2012-04-25 10:55:45 PDT
Created attachment 618355 [details] [diff] [review]
part 3: change NoteRoot to NoteJSRoot and NoteNativeRoot
Comment 22 Andrew McCreight [:mccr8] 2012-04-25 10:56:15 PDT
Created attachment 618357 [details] [diff] [review]
part 4: remove nsCycleCollectionLanguageRuntime
Comment 23 Andrew McCreight [:mccr8] 2012-04-25 10:57:00 PDT
Created attachment 618358 [details] [diff] [review]
part 5: remove remaining internal langIDs from CC
Comment 24 Andrew McCreight [:mccr8] 2012-04-25 11:49:37 PDT
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] 2012-04-25 11:51:35 PDT
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] (reviewing overload) 2012-04-26 17:20:03 PDT
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] 2012-04-26 17:28:01 PDT
(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] 2012-05-02 09:35:03 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.