1169097 - Remove CountHeap

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

This is ~160 lines of incredibly old crufty code that was causing me some grief today. Grepping for users, we have 5 tests using it. Two users are actual tests that are adding arguable value, although not a great deal of it. On the other hand, three are from fuzz bugs where adding a new GC feature broke countHeap. CountHeap is demonstrably not worth the maintenance effort of retaining and should be removed accordingly.

Comment 1

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

I would guess that some uses of countHeap could probably be replaced with finalizeCount().

That's for asserting that something *was* finalized. I suppose for asserting that something was not finalized, you could use it as a key in a WeakMap and then use nondeterministicWeaqkMapKeys()?

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=483b0d73894e

Comment 3

[Terrence Cole [:terrence]]

Attachment: remove_count_heap-v0.diff

I think CountHeap's problem is not conceptual, it is just the interface. CountHeap's contract is something like: for the subgraph a->b for any a or all roots if not a to any b or all edges if not b, return edge counts grouped by AAAAAAAHHHHH. I'm sure its a swell tool if you're smart enough to use it. Unfortunately that set is Jeff and Niko. Note that that set does not include the interface's author.

I've added a new mechanism, hasChild(parent, child), that returns true iff child is a child of parent. This seems to handle the only useful test using it. The other test is a leak check for Watchpoints that I'm more than happy to just drop.

Comment 4

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

Comment on attachment 8612617 [details] [diff] [review]
remove_count_heap-v0.diff

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

::: js/src/tests/js1_8/extensions/regress-394709.js
@@ -45,5 @@
> -    throw "A leaky watch point is detected";
> -
> -  function runtest () {
> -    var obj = { b: 0 };
> -    obj.watch('b', watcher);

It would be pretty easy to switch this over to finalizeCount()/makeFinalizeObserver():

  var obj = { b: makeFinalizeObserver() };

and replacing countHeap() with finalizeCount(). I guess I'd rather you do that, assuming it works. If it's any trouble, I'm fine with dropping the test.

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=483b0d73894e

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4a2f953524

Comment 7

[Carsten Book [:Tomcat]]

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