Replace markAndScanString/Symbol with traverse

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 8596711 [details] [diff] [review]
10.0.2_replace_markAndScanFoo-v0.diff

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 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

4 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

4 years ago
Actually, I should probably just make the changes inline in the eagerlyMarkChildren changes stack. Will do that instead.
https://hg.mozilla.org/mozilla-central/rev/2cb0f73999f4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.