refactor nsXPConnect::Traverse

RESOLVED FIXED in mozilla16

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

5 years ago
This function (which traces through JS objects for the cycle collector) is pretty big, but there are a few separate steps it takes, so I'd like to break it up a bit.  There's probably also some other cleanup I can do at the same time.

This will also make it easier for me to alter its behavior for CompartmentParticipant in bug 754495.
(Assignee)

Comment 1

5 years ago
Created attachment 633608 [details] [diff] [review]
part 1: add redundant vars to enable outlining

This causes slightly more work, but only when we're logging.
Assignee: nobody → continuation
(Assignee)

Comment 2

5 years ago
Created attachment 633610 [details] [diff] [review]
part 2: split Traverse into multiple pieces

This will be bit rotted by bug 616262, but only in a trivial way.
(Assignee)

Comment 3

5 years ago
Created attachment 633611 [details] [diff] [review]
part 3: remove Cisms from GC thing traversal
(Assignee)

Comment 4

5 years ago
Created attachment 633612 [details] [diff] [review]
part 4: minor fiddly cleanup
(Assignee)

Comment 5

5 years ago
Created attachment 633613 [details] [diff] [review]
part 5: make * conform to JS style consistently
(Assignee)

Comment 6

5 years ago
Comment on attachment 633608 [details] [diff] [review]
part 1: add redundant vars to enable outlining

Please review the rest of the patches, too, I just think there's no need to spam the bug by setting the review flag on every piece.
Attachment #633608 - Flags: review?(wmccloskey)
(Assignee)

Comment 7

5 years ago
I'll leave the main thread cleanup stuff for another patch, after XPC singlethreading lands.  Or maybe it will just fix it.  Either way.
No longer depends on: 763773
Comment on attachment 633608 [details] [diff] [review]
part 1: add redundant vars to enable outlining

Is this all you wanted me to review?

As an aside, I think it would be nice if we could replace uses of |clazz| in xpconnect with |clasp|, which is what we use in the JS engine.
Attachment #633608 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 9

5 years ago
Sure, I can do that for the code I touch in these patches.
(Assignee)

Comment 10

5 years ago
Created attachment 634172 [details] [diff] [review]
part 6: clazz to clasp
Attachment #634172 - Flags: review?(wmccloskey)
Attachment #633610 - Flags: review+
Attachment #633611 - Flags: review+
Attachment #633612 - Flags: review+
Attachment #633613 - Flags: review+
Comment on attachment 634172 [details] [diff] [review]
part 6: clazz to clasp

Excellent, thanks!
Attachment #634172 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 12

5 years ago
Thanks for the reviews!

Try run looked good.  Landed the patch in one chunk.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9118e53405c7
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/9118e53405c7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.