Status

()

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

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
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()?
(Assignee)

Comment 3

3 years ago
Created attachment 8612617 [details] [diff] [review]
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.
Attachment #8612617 - Flags: review?(sphink)
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.
Attachment #8612617 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/ac4a2f953524
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.