If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Status

()

Core
JavaScript Engine
6 years ago
3 years ago

People

(Reporter: billm, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 years ago
Right now most of our GCs are triggered by timers. I think this is good. However, these GCs only fire if we're allocating GC things (they don't run unless a new chunk has been allocated since the last GC) or if the CC found some objects to unlink.

However, someone reported a bug to me recently where a bunch of canvas objects get allocated. We need a GC followed by a CC to deallocate them. But we never GC because no JS stuff gets allocated. We should have caught this via malloc counters, but they're not used in this case.

The immediate way to fix this bug is to call updateMallocCounter when the canvas is allocated. However, problems like this are going to continue to surface because it's so easy to forget updateMallocCounter.

So I'm thinking that a better way to do this is to GC every 10 or 20 seconds as long as the browser is active. I'm thinking that we can decide whether the browser is active by checking if the ScriptEvaluated callback has happened since the last GC. I'm a bit worried that ScriptEvaluated just gets called constantly, but I'll look into it.

At the same time, I think this would be a good opportunity to start triggering compartmental GCs from timers. The hard part is deciding which compartment to GC, and whether to do a full GC or not. Maybe we could see what compartments we executed scripts in since the last GC, and collect those. This would require us to be able to GC multiple compartments during a compartment GC, but that should be pretty easy.

One important factor is the interaction with the cycle collector. I might be mistaken, but I think this interaction is pretty messed up right now: if we do a compartment GC followed by a CC, then the CC will see objects in non-collected compartments as white. This is okay, because it will just ignore them. But it sucks.

One way to fix this would be to clear the mark bits only for compartments being collected. I think this is sound, but I'm not sure. (We need to know if UnmarkGray still protects us even if there's an intervening GC.) Another way is to make sure that we still do enough full GCs that CCs will be effective. The latter solution is definitely less appealing.

A final consideration is that we need an occasional full GC to clear out the cross compartment wrapper map. Eventually, we might be able to clean this up with the cycle collector.

Comment 1

6 years ago
We have to make sure we don't cause long pause times. We tried a similar strategy before and it was messing with flash videos playing.

Comment 2

6 years ago
(In reply to Bill McCloskey (:billm) from comment #0) 
> The immediate way to fix this bug is to call updateMallocCounter when the
> canvas is allocated. However, problems like this are going to continue to
> surface because it's so easy to forget updateMallocCounter.

Other option is to tweak CC/GC handling in nsJSEnvironment.cpp.
Right now if after running a task there are >1000 suspected CC objects, we
may trigger CC. Then, if the CC has run one or more times, and CC has collected >250 object,
yet GC hasn't run, we trigger GC.
These numbers are sort of random, though they worked reasonable well with certain testcases.
Maybe CC should collect the size of suspected and/or collected objects, and we wouldn't 
rely on the number of objects, but size.


> At the same time, I think this would be a good opportunity to start
> triggering compartmental GCs from timers. The hard part is deciding which
> compartment to GC, and whether to do a full GC or not. Maybe we could see
> what compartments we executed scripts in since the last GC, and collect
> those. This would require us to be able to GC multiple compartments during a
> compartment GC, but that should be pretty easy.
It would be better to run GC for different compartments in different tasks.
So there could be a queue of compartments which need GC, and an interval timer active as long
as the queue is not empty. The timer would then call compartment timer. (Full GC should clear the queue.)


> One important factor is the interaction with the cycle collector. I might be
> mistaken, but I think this interaction is pretty messed up right now: if we
> do a compartment GC followed by a CC, then the CC will see objects in
> non-collected compartments as white. This is okay, because it will just
> ignore them. But it sucks.
Well, CC sees the objects in other compartments in whatever color they happen to have.
(Why would the objects be marked white in non-collected compartments?)

Comment 3

6 years ago
(In general we seem to trigger compartment GC very rarely)
(In reply to Olli Pettay [:smaug] from comment #2)
> Well, CC sees the objects in other compartments in whatever color they
> happen to have.
> (Why would the objects be marked white in non-collected compartments?)

I believe that a compartmental GC clears the mark bits in all other compartments, making them white.  In turn, the CC will treat them as marked and not actually collect cycles that pass through other compartments until the next GC computes accurate mark bits.

Comment 5

6 years ago
uh, that sounds strange. Why would compartmental gc change the state of some objects in
other compartments. I must have misunderstood what compartment GC is about, or it is not
doing at all what it should do. Or both :)
(Reporter)

Comment 6

6 years ago
(In reply to Olli Pettay [:smaug] from comment #2)
> Maybe CC should collect the size of suspected and/or collected objects, and
> we wouldn't 
> rely on the number of objects, but size.

This won't work in the case I described. We need a GC to mark the canvas stuff gray and then a CC to collect it. If we do a CC alone, it won't be able to collect anything, and so it won't trigger a GC.

(In reply to Olli Pettay [:smaug] from comment #5)
> uh, that sounds strange. Why would compartmental gc change the state of some
> objects in
> other compartments. I must have misunderstood what compartment GC is about,
> or it is not
> doing at all what it should do. Or both :)

The GC is broken here. It shouldn't be too hard to fix, but we need to make sure to test thoroughly.
(In reply to Bill McCloskey (:billm) from comment #6)
> The GC is broken here. It shouldn't be too hard to fix, but we need to make
> sure to test thoroughly.
How would you fix that?  Can you have a cross-compartment gray edge?  In that case, a cross-compartment gray edge could become black, but there would be no ungraying.
(Assignee)

Updated

3 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.