1266887 - Move Rooted to the Zone

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 6.2_move_rooted_to_zones-v0.diff

We're well on our way to removing JSContext and I'd like to get Rooted off of it before it becomes a blocker. Rather than moving everything to the runtime and making it even more of a global, I'd prefer to move Rooted to the Zone. In theory, Rooted should not cross zones (at least not often), so this could lead to a nice assertion. And eventually we should even be able to take advantage of this to do faster root marking during Compartmental GC's.

I was not able to see any performance difference on octane, so I think the extra indirection will not be troublesome.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8744514 [details] [diff] [review]
6.2_move_rooted_to_zones-v0.diff

Hold on a tick. I didn't count on just how amazingly crazy the browser is during initialization.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 6.2v1_move_rooted_to_zones-v1.diff

Actually, it's trivial to check the zone first and root on the runtime or zone as needed. This should make it much easier to assert that everything lands in the right zone eventually.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8745034 [details] [diff] [review]
6.2v1_move_rooted_to_zones-v1.diff

Review of attachment 8745034 [details] [diff] [review]:
-----------------------------------------------------------------

I can't think of what could go wrong with this, which probably just means I'm having a failure of imagination.

::: js/src/gc/RootMarking.cpp
@@ +79,5 @@
>          trc, stackRoots_[JS::RootKind::Traceable], "Traceable");
>  }
>  
>  static void
>  MarkExactStackRoots(JSRuntime* rt, JSTracer* trc)

Someday, we probably ought to remove the "Exact" part of this name.

::: js/src/vm/Symbol.cpp
@@ +36,5 @@
>  
>  Symbol*
>  Symbol::new_(ExclusiveContext* cx, JS::SymbolCode code, JSString* description)
>  {
> +    JSAtom* atom = nullptr;

Is this change part of this bug?

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e6b95c85a9

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8745034 [details] [diff] [review]
6.2v1_move_rooted_to_zones-v1.diff

Review of attachment 8745034 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Symbol.cpp
@@ +36,5 @@
>  
>  Symbol*
>  Symbol::new_(ExclusiveContext* cx, JS::SymbolCode code, JSString* description)
>  {
> +    JSAtom* atom = nullptr;

Oddly, yes. We create the well-known symbols outside of any compartment, so my initial pass that did not have the JSRuntime fallback needed a different way to handle this case. Turns out we don't even need the Rooted here; probably was from back when Atomized used to GC. I rolled the cases back, but it seemed silly to re-add the unnecessary root.

Comment 6

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edb632ad80c4ad653972056ff78578a129a35506
Bug 1266887 - Store Rooted heads on the Zone; r=sfink

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/edb632ad80c4

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/035b54e9f1be3a1172692aa58352c8bb7f2b1f3d
Backout edb632ad80c4 (Bug 1266887) for regressing performance on windows.

Comment 9

[Terrence Cole [:terrence]]

Looks like we're not going to be able to land this improvement until we actually land the context removal as well.

Comment 10

[Terrence Cole [:terrence]]

Actually, we found that tspaint does not sync against GC, so this may be spurious.

Comment 11

[Terrence Cole [:terrence]]

It may be spurious, or it may be (1) an issue with memory layout, although fairly unlikely, or (2) changed inlining, given MSVC's egregiously stupid heuristics in this area.

I think I'd like to re-land, cutting out the parts of RootLists that are not necessary on Zone to reduce the potential memory impacts and with |inline| sprinkled around all the potentially hot code. Given that this code is already hot on JSContext, I think anything remaining must be spurious.

Comment 12

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a6c4d7f24f0

Comment 13

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/113aed339ad20300dcd8d420b04a759c01f84158
Bug 1266887 - Store Rooted heads on the Zone; r=sfink

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/113aed339ad2