Closed
Bug 1235598
Opened 9 years ago
Closed 9 years ago
Convert Gecko over to the C++ Tracing API
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8702623 -
Flags: review?(bugs)
Comment 2•9 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 3•9 years ago
|
||
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 | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/55db488f9dfd078a927cb706001d65ef38da3219
Bug 1235598 - Part 1: Add better SpiderMonkey API support for tracing in C++; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf35918cb3baff6e2c67209fa3cf0b0d03771073
Bug 1235598 - Part 2: Use TraceEdge exclusively in Gecko; r=smaug
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da07459ce20c444c070ba8061c687c77bbc5400
Backout Bug 1235598 Part 2 because of merge bustage on a CLOSED TREE
Backed it all out because of build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b8d8e11880
https://treeherder.mozilla.org/logviewer.html#?job_id=19134343&repo=mozilla-inbound
Flags: needinfo?(terrence)
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/80cd10a8b3d7a2e249a7632dd442306e7f0b8890
Bug 1235598 - Part 1: Add better SpiderMonkey API support for tracing in C++; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcd3c276785b20eaa1b3ffac83149ae1d3a8b18
Bug 1235598 - Part 2: Use TraceEdge exclusively in Gecko; r=smaug
Assignee | ||
Comment 10•9 years ago
|
||
This was merge bustage: I fixed the new tracing that Olli landed yesterday, but in the wrong patch.
Flags: needinfo?(terrence)
I had to back these out for apparently turning linux reftests orange:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbe608d70f0
Example failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=19164298&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=19164299&repo=mozilla-inbound
Flags: needinfo?(terrence)
You also seem to have upset about half of the b2g emulator reftests.
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Comment 15•9 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•9 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.
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ed386771f30c922b402a8c5d25f3fa13b98ed6
Bug 1235598 - Part 1: Add better SpiderMonkey API support for tracing in C++; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d27aac8b654a3711c899107e36781a01c165e895
Bug 1235598 - Part 2: Use TraceEdge exclusively in Gecko; r=smaug
Comment 19•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25ed386771f3
https://hg.mozilla.org/mozilla-central/rev/d27aac8b654a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•