Convert Gecko over to the C++ Tracing API

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8702622 [details] [diff] [review]
use_TraceEdge_from_embeddng-sm-v0.diff

I only had to beef up support on SpiderMonkey's side a little bit. There were two main things I touched.

First, now that adding a new trace variant is trivial, I added JS::TraceNullableEdge. This made a fairly large number of places simpler, so I'm confident that it's a clear net win.

Secondly, I ensured that JS::Trace* are only instantiated for externally exposed types. This is more important than it seems: if we accidentally expose a new object sub-class without adding manual barrier overrides to implement post-barriers, we have a latent sec-crit bug. With this change, we will now fail to build -- a link failure, but better than nothing -- if that occurs. It's a bit indirect, but plugs an important gap in our type coverage that I've been worried about for awhile now.
Attachment #8702622 - Flags: review?(sphink)
(Assignee)

Comment 1

2 years ago
Created attachment 8702623 [details] [diff] [review]
use_TraceEdge_from_embeddng-gk-v0.diff
Attachment #8702623 - Flags: review?(bugs)

Comment 2

2 years ago
Comment on attachment 8702623 [details] [diff] [review]
use_TraceEdge_from_embeddng-gk-v0.diff

 IdToObjectMap::trace(JSTracer* trc)
 {
-    for (Table::Range r(table_.all()); !r.empty(); r.popFront()) {
-        DebugOnly<JSObject*> prior = r.front().value().get();
-        JS_CallObjectTracer(trc, &r.front().value(), "ipc-object");
-    }
+    for (Table::Range r(table_.all()); !r.empty(); r.popFront())
+        JS::TraceEdge(trc, &r.front().value(), "ipc-object");

I wouldn't drop {}, but I guess this file is using some odd coding style.
Attachment #8702623 - Flags: review?(bugs) → review+
Comment on attachment 8702622 [details] [diff] [review]
use_TraceEdge_from_embeddng-sm-v0.diff

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

This is good stuff.
Attachment #8702622 - Flags: review?(sphink) → review+
(Assignee)

Updated

2 years ago
Blocks: 1235923
(Assignee)

Comment 10

2 years ago
This was merge bustage: I fixed the new tracing that Olli landed yesterday, but in the wrong patch.
Flags: needinfo?(terrence)
You also seem to have upset about half of the b2g emulator reftests.
(Assignee)

Comment 15

2 years ago
Well, there's basically no way that a tracing change could result in an image comparison failure (as opposed to a crash). I now have two try runs at [1] and [2], one before rebasing and one after, that don't show those errors. Or at least not in the same place. It's hard to tell what's going on with all the default orange in try runs recently.

1 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=216179f9d9ba
2 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf94c00e677
Flags: needinfo?(terrence)
Oh, I thought I had followed up in here. These failures turned out to be from the libmesa upgrade that was rolled out to ec2 instances. That upgrade was rolled back due to all of the fallout. Feel free to reland these whenever.
(Assignee)

Comment 17

2 years ago
(In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment #16)
> Oh, I thought I had followed up in here. These failures turned out to be
> from the libmesa upgrade that was rolled out to ec2 instances. That upgrade
> was rolled back due to all of the fallout. Feel free to reland these
> whenever.

Great! Thought I was going crazy there for a bit.

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25ed386771f3
https://hg.mozilla.org/mozilla-central/rev/d27aac8b654a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.