The default bug view has changed. See this FAQ.

eliminate the cycle collector's vestigial language agnosticism

RESOLVED FIXED in mozilla15

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

6.05 KB, patch
smaug
: review+
Details | Diff | Splinter Review
32.41 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 613743 [details] [diff] [review]
WIP
Assignee: nobody → continuation
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 738380
(Assignee)

Comment 8

5 years ago
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.

Updated

5 years ago
Depends on: 744332
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 614062 [details] [diff] [review]
part 2: remove fake language agnosticism of NoteRoot
(Assignee)

Updated

5 years ago
Attachment #614062 - Attachment description: remove fake language agnosticism of NoteRoot → part 2: remove fake language agnosticism of NoteRoot
(Assignee)

Updated

5 years ago
Summary: don't use ToParticipant for JS objects → eliminate the cycle collector's vestigial language agnosticism
(Assignee)

Updated

5 years ago
Depends on: 744088
(Assignee)

Comment 12

5 years ago
Created attachment 614113 [details] [diff] [review]
part 1: remove vestigial language agnosticism of NoteScriptChild
Attachment #613743 - Attachment is obsolete: true
Attachment #614054 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 614122 [details] [diff] [review]
part 2: remove vestigial language agnosticism of NoteRoot
Attachment #614062 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 678615
(Assignee)

Comment 17

5 years ago
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.
No longer depends on: 744332
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 744332
(Assignee)

Comment 19

5 years ago
Created attachment 618353 [details] [diff] [review]
part 1: store nsCycleCollectionJSRuntime separately
(Assignee)

Comment 20

5 years ago
Created attachment 618354 [details] [diff] [review]
part 2: change NoteScriptChild to NoteJSChild
Attachment #614113 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 618355 [details] [diff] [review]
part 3: change NoteRoot to NoteJSRoot and NoteNativeRoot
Attachment #614122 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 618357 [details] [diff] [review]
part 4: remove nsCycleCollectionLanguageRuntime
Attachment #614140 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Created attachment 618358 [details] [diff] [review]
part 5: remove remaining internal langIDs from CC
Attachment #614164 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 25

5 years ago
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.
Blocks: 698919
(Assignee)

Updated

5 years ago
Blocks: 749370
(Assignee)

Updated

5 years ago
Attachment #618353 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #618354 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #618355 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #618357 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #618358 - Flags: review?(bugs)
Attachment #618353 - Flags: review?(bugs) → review+
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.
Attachment #618354 - Flags: review?(bugs) → review+
Attachment #618355 - Flags: review?(bugs) → review+
Attachment #618357 - Flags: review?(bugs) → review+
Attachment #618358 - Flags: review?(bugs) → review+
(Assignee)

Comment 27

5 years ago
(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.
(Assignee)

Comment 28

5 years ago
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.
(Assignee)

Comment 29

5 years ago
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.
Target Milestone: --- → mozilla15
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
No longer blocks: 698919
Blocks: 698919
You need to log in before you can comment on or make changes to this bug.