Closed Bug 1710484 Opened 4 years ago Closed 4 years ago

30 second cycle collection, mostly spent in jit::TraceCacheIRStub

Categories

(Core :: JavaScript Engine: JIT, defect, P2)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1715512

People

(Reporter: jbuck, Unassigned)

References

Details

Basic information

Steps to Reproduce: I was sorting through emails in Gmail & opening some Google Docs when I experienced a 30s hang where no tab would render or respond to my input. I was able to switch between the hung tabs, but couldn't do much else. Unlike the last time I experienced this, I was able to capture a profile and it looks like a large amount of CC/GC is the culprit


More information

Profile URL: https://share.firefox.dev/3w5INZF

Basic systems configuration:

OS version: Mac OS 10.15.7

GPU model: Intel Iris Plus Graphics 655 1536 MB

Number of cores: 4

Amount of memory (RAM): 16GB

Andrew, could you maybe have a look at the attached profile?

Component: Performance → JavaScript: GC
Flags: needinfo?(continuation)

That's a pretty weird profile. It looks like the hang is due to cycle collection, but the bulk of the time is being spent in TraceCacheIRStub. Either there's some very severe leak of JIT data structures, or there's some kind of quadratic behavior when the CC is traversing these data structures. (We've seen that before with shapes, but shape tracing is probably much weirder.)

Jan, any idea what might be going wrong here?

Component: JavaScript: GC → JavaScript Engine: JIT
Flags: needinfo?(continuation) → needinfo?(jdemooij)
Summary: Tab hang for 30s during long CC/GC → 30 second cycle collection, mostly spent in jit::TraceCacheIRStub

Looking at the profile again, it does appear that the time being spent is in shapes being held by these IR stubs, so some kind of quadratic behavior seems like the most likely cause.

We can optimize Shape tracing in TraceCycleCollectorChildren a bit (see here), to only trace the BaseShape's children when the BaseShape is different from the previously traced BaseShape (most of the time, all shapes in the shape lineage have the same BaseShape). This should eliminate a lot (but not all) of the NoteJSChild calls in that profile. I'll post a patch for this.

We removed ObjectGroup in bug 1689413 and BaseShapes now store the object's prototype and global.

There seems to be an underlying issue (this bug and similar bugs in the past) where the CC can spend a lot of time tracing JSScript => JitScript => IC stubs => many Shapes. I don't know if there's anything we can do to optimize that better?

Flags: needinfo?(jdemooij)
Depends on: 1710627

I haven't looked at this in a bit, but I think the basic idea is that there are so many shapes that we can't add them to the CC graph. Instead, if a CCed thing holds onto a shape, we immediately trace through the entire set of reachable shapes, looking for stuff the CC cares about. But each shape can be referred to by many things, so retracing them over and over can be expensive.

I just had a similar experience with a few docs and gmail tabs open: https://share.firefox.dev/3bjcHS3

Severity: -- → S3
Priority: -- → P2

(In reply to Andrew McCreight [:mccr8] from comment #5)

I haven't looked at this in a bit, but I think the basic idea is that there are so many shapes that we can't add them to the CC graph.

I wonder how bad it would be to add shapes to the CC graph. I would expect there to be many more objects that shapes, and these are already present in the graph.

(In reply to Jon Coppeard (:jonco) from comment #7)

(In reply to Andrew McCreight [:mccr8] from comment #5)

I haven't looked at this in a bit, but I think the basic idea is that there are so many shapes that we can't add them to the CC graph.

I wonder how bad it would be to add shapes to the CC graph. I would expect there to be many more objects that shapes, and these are already present in the graph.

Wait... so if I understand this correctly, because the shapes are not represented in the CC graph, we don't know whether we've traced a given shape or not? So if we have 1000 same-shape objects reachable from the CC graph, each with 10 properties, then we'll re-trace all of the shapes (so 10000 shape visits, and lots of base shape visits as well before Jan's same-base optimization)? It does seem like tracking the shapes (most straightforwardly, by adding them to the CC graph) might make sense. But I guess I don't understand why there are so many shapes in the first place.

If I understand correctly, the ReShape work should improve the equation here considerably -- we'll only need one Shape per, uh, shape of object, rather than one Shape per property. That might either make the current setup work ok, or would make it feasible to add the shapes to the CC graph.

The other idea, instead of or in addition to what was mentioned above, would be to add a flag to a Shape saying that there's no need to trace any parent shapes: they all have the same prototype and there are no accessors or other GC pointers that could form a cycle.

(In reply to Steve Fink [:sfink] [:s:] from comment #8)

Wait... so if I understand this correctly, because the shapes are not represented in the CC graph, we don't know whether we've traced a given shape or not? So if we have 1000 same-shape objects reachable from the CC graph, each with 10 properties, then we'll re-trace all of the shapes (so 10000 shape visits, and lots of base shape visits as well before Jan's same-base optimization)?

Yes, that is correct.

It does seem like tracking the shapes (most straightforwardly, by adding them to the CC graph) might make sense. But I guess I don't understand why there are so many shapes in the first place.

Adding things to the CC graph is expensive. IIRC when I last looked at this (a long time ago!) it made the CC graph like a third larger or something ridiculous.

Maybe we could just add base shapes to the CC graph if there aren't as many of them? I'm not sure what part of redoing the tracing over and over is expensive.

My understanding is that this should now be fixed by the ReShape work (bug 1704430).

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.