Closed Bug 489636 Opened 13 years ago Closed 7 years ago

Heavy property tree forking should trigger GC

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: graydon, Assigned: terrence)

Details

(Keywords: hang)

Attachments

(1 file)

Given a case such as:

function f()
{
  for (var i = 0; i < 3; ++i){}
  var a1 = eval;
  delete eval;
  eval = a1;
  var a3 = toString;
  delete toString;
  toString = a3;
}
for (var a = 0; a < 100000; ++a) {
  f();
}

The property tree forks (due to conflicting entries) on every iteration. This causes the property tree to fill up with dead nodes.

This can be solved by inserting a GC trigger in the forking path of js_AddScopeProperty.
Flags: blocking1.9.1?
Given by Brendan, the hang which happens for the testcase on bug 484693 could be the same root cause. I'll reference it here so we can check if Graydon's patch will help us too. If not, I'll file a new bug later. For now raising severity and adding hang keyword.
Severity: normal → critical
Keywords: hang
A fuzz-test is not enough to make the hang critical. We've had intentional bad performance on pathological delete use for years (7+, in this property tree respect) but no real-world bug.

However, dbaron mentioned a bug to-do with the graph server, which could be the same as this one, or related. I don't remember its number, though.

/be
(In reply to comment #2)
> However, dbaron mentioned a bug to-do with the graph server, which could be the
> same as this one, or related. I don't remember its number, though.

bug 488559
It's not clear to me that the synthetic case is really performing wrong, now that I look at it. It's generating a bunch of sprops per iteration -- because it's doing a middle-delete and conflicting add -- but otherwise this is just a "generating lots of garbage is expensive" bug. Plus the perhaps surprising fact that deleting and re-adding a property generates garbage. Users probably don't expect that, but it's not something easy to change in SM.
doesn't look like something that can block, given the comments here
Flags: blocking1.9.1? → blocking1.9.1-
This may not be directly actionable, but the testcase does make a great GC u-bench benchmark.
Assignee: graydon → terrence
Status: NEW → ASSIGNED
Attachment #8703165 - Flags: review?(sphink)
Comment on attachment 8703165 [details] [diff] [review]
add_property_deletion_gc_ubench-v0.diff

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

Looks good, but how is it with the default settings? I try to give the tests settings that cause some amount of noticeable pauses on my machine, without lagging things too much (totally unscientific). For example, the events one is

    defaultGarbagePerFrame: "100K",
    defaultGarbageTotal: "8",
Attachment #8703165 - Flags: review?(sphink) → review+
It seems just about perfect on my machine: ggc every half second or so with major gc's every 4-6 seconds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/31edd1840c5f651b5dbf182fdb7f04fe98c88d86
Bug 489636 - Add a GC u-bench test for property tree splitting via deletion; r=sfink
https://hg.mozilla.org/mozilla-central/rev/31edd1840c5f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.