Closed
Bug 1157829
Opened 10 years ago
Closed 10 years ago
Replace markAndScanString/Symbol with traverse
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
These two bizarrely custom methods are now identical to traverse + one assertion, so we can just do that now.
Attachment #8596711 -
Flags: review?(sphink)
Comment 1•10 years ago
|
||
Comment on attachment 8596711 [details] [diff] [review]
10.0.2_replace_markAndScanFoo-v0.diff
Review of attachment 8596711 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/gc/Marking.cpp
@@ +583,5 @@
> } // namespace js
>
> template <typename T>
> +void
> +GCMarker::traverseOwnedEdge(JSObject* source, T* target)
I know I'm dense, but what does "owned edge" mean here? I mean, I know about regular edges, and things like owned shapes, but I don't quite understand what it means here. Should it be traverseSameZoneEdge? (Though I guess that doesn't cover the atom case.) traverseCheckedEdge? Or just traverseChildEdge? (If this "owned" is different from shapes' "owned", I'd really rather not use the same term.)
No change necessary, really, just an explanation in the bug. Though if there *is* a useful comment to be made (or just a different function name), that would be good.
Attachment #8596711 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 2•10 years ago
|
||
Huh, I just meant "owned" in the traditional dictionary sense: e.g. belonging to. Normal traversals just specify the target. The callers I'm replacing here also pass the gcthing (really only objects) that logically contains the edge to the thing being traversed. Ideally we'd use this sort of traverse(owner, target) interface everywhere and make comprehensive cross-compartment edge checks on all edges, but that would be mildly inconvenient from most places so we currently only do it in the one place where we still have a stack reference to the owner: processMarkStackTop. Actually, now that I'm reflecting on it, that last sentence was a total lie. All the places we eagerlyMarkChildren we could trivially switch over to this interface. Also, the name is wrong. Hurm. :-/
I think it would be worth updating the name and checking this in as-is, as it doesn't change behavior. I'll file a follow-up to see how practical it would be to just make traverse take the source unconditionally.
Assignee | ||
Comment 3•10 years ago
|
||
Actually, I should probably just make the changes inline in the eagerlyMarkChildren changes stack. Will do that instead.
Assignee | ||
Comment 4•10 years ago
|
||
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•