1324002 - Mark atoms separately in each zone

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

The attached patch modifies the GC to be able to collect atoms even when not doing a full GC.  Each zone has its own bitmap of atoms which it might reference, and atoms are only destroyed when they are not referenced by any zone.  This is pretty non-invasive for the marking/etc. GC code --- we mark the atoms normally during the GC, then translate the chunk mark bits into a bitmap to update the zone's mark bitmap with, and finally fill in the chunk mark bits with additional atoms referenced by zones that weren't collected before sweeping.  The per-zone bitmaps use a different representation from chunk mark bits that only requires one bit per atom.

From an API perspective the main thing this changes is that atoms need to be marked when they are moved around across zone boundaries.  We have checks for this when doing cross-compartment assertions and when marking atoms in a zone.  This patch fixes places where atoms move between zones in the JS engine (well, enough to get jit-tests to pass) but I haven't tested the browser yet.  I also still need to measure performance in the browser and look into whether it would be worthwhile to split up the OMT atoms sweeping tasks that are being done.  I'm posting this patch now to see if this approach looks good, before doing these final steps.

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8819315 [details] [diff] [review]
patch

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

This looks really good.  I'm a bit concerned about the representation of the arena mark bits using interleaved bitmaps though.  It seems complicated and I'm wondering whether the efficiency gain is worth the extra complexity.

On a related note, the patch doesn't seem to take account of the gray mark bits (every cell has two mark bits).  With the current scheme these will need to be masked off when storing the bits otherwise they may interfere with data for another arena.

It would be great if there was a way to automatically mark atoms when they were transferred to another compartment or at least make it more obvious when this is necessary.  I guess we'll catch violations during testing given the assertions you added though.

::: js/src/gc/AtomMarking.cpp
@@ +89,5 @@
> +    MOZ_ASSERT(arena->zone->isAtomsZone());
> +    MOZ_ASSERT(arena->zone->runtimeFromAnyThread()->currentThreadHasExclusiveAccess());
> +
> +    // We need to find a range of bits from the atoms bitmap for this arena.
> +    LockGuard<Mutex> lock(freeArenaMutex);

Both registerArena and unregisterArena are called with the GC lock held, so we may not need this lock.

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> On a related note, the patch doesn't seem to take account of the gray mark
> bits (every cell has two mark bits).  With the current scheme these will
> need to be masked off when storing the bits otherwise they may interfere
> with data for another arena.

I don't entirely understand what you are saying here (and I haven't looked at this patch), but I'd just make the point that it should be okay to never mark atoms gray, because (I assume) they never hold alive C++ (directly or indirectly) and thus the CC should never care about them. (Though you have to deal with atoms being held alive only by gray objects, and still getting marked black, which might be weird.)

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] (PTO-ish) from comment #2)
> (In reply to Jon Coppeard (:jonco) from comment #1)
> > On a related note, the patch doesn't seem to take account of the gray mark
> > bits (every cell has two mark bits).  With the current scheme these will
> > need to be masked off when storing the bits otherwise they may interfere
> > with data for another arena.
> 
> I don't entirely understand what you are saying here (and I haven't looked
> at this patch), but I'd just make the point that it should be okay to never
> mark atoms gray, because (I assume) they never hold alive C++ (directly or
> indirectly) and thus the CC should never care about them. (Though you have
> to deal with atoms being held alive only by gray objects, and still getting
> marked black, which might be weird.)

This is what I thought too but it's not quite right.  Arenas are reclaimed after sweeping if none of their GCThings are marked, and if random non-GCThing mark bits in the arena are set then when the arena is repurposed for some other zone and thing kind, the contents of the arena might look like they are marked black or gray.

This patch does deal with the gray bit marking issue, though, to fix the above problem.  The ThingMaskCache structure is used to construct masks for the valid black mark bits in an arena of a given thing size, and this mask is used by AddBitmapToChunkMarkBits to make sure we only set black mark bits in arenas we are augmenting.

I'm still working on getting this patch to look good on try, and will then do some measuring on real world test cases to see how large the atom bitmaps get and whether it is worth it to keep the complexity of this interleaving.  (The only good alternative I think is to just have copies of the chunk's mark bits for each arena; trying to compact down adjacent GCThings into adjacent mark bits is at least as complex as this approach and will be a lot slower.)

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

This patch is (finally) pretty green on try.  I added a lot number more places where atoms need to be marked when they move across zones (I don't see a way to make the necessity of this more obvious, since the problem is an extension of the cross compartment wrapping issues we already face).  I also beefed up the atom marking assertions, and removed the atoms bitmap interleaving logic (it is buggy), so that we use a simpler bitmap representation which uses ArenaBitmapWords words worth of space for each atoms zone arena (or one bit per Cell in these arenas; this is the same as for chunk mark bitmaps).

I also did some measuring for the time and space used for the atom mark bitmaps.  For all these tests I loaded up 10 different sites in different tabs.

To measure time, after loading the tabs above I loaded about:memory and triggered some full GCs.  The longest pause I saw for sweeping atoms was 5.2ms, vs. 4.9ms for a recent firefox.  These numbers are noisy but I think indicate that the extra time now taken in atom sweeping for updating zone and chunk mark bitmaps will not have much of an effect compared to the time taken to go through all the atoms zone arenas to look for things that need sweeping.

To measure space, after loading the tabs above I loaded about:memory, minimized memory usage, then measured.  In the content process:

atoms-table: 8.10 MB
atoms zone: 10.49 MB
atoms-mark-bitmaps (total across all zones): 1.91 MB

So the presence of the bitmaps increased the total size used by atoms by about 10%.  Breaking this down, the content process has (I think) 18 zones in the main runtime and each zone's mark bitmap uses at most 130KB of allocated data.  (Some little-used zones have a smaller bitmap, so the total size of the bitmaps is less than 18 * 130KB.)  The bitmaps have enough usable length for 120KB worth of bits --- or about a million bits --- so there isn't much allocation slop.

Now, the atoms table in this runtime only has 165K entries, so if we were making optimal use of the bits we could potentially reduce the size of the mark bitmaps by more than a factor of five, and reduce the atom size overhead of the bitmaps from 10% to 2%.  I think this is definitely worth doing, but it might be better off in a followup bug due to the extra complexity and potential impact on atoms sweep times.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8821894 [details] [diff] [review]
patch

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

Sorry it took a while to get to this.

The SM parts look good to me but I have a few questions and comments.

I'm tempted to suggest renaming all the MarkAtom methods to WrapAtom instead. They don't wrap anything, but the operation is analogous to wrapping and happens in the same situation of using a GC thing in a different compartment so I think it would be easier to explain and for people to remember to do it. What do you think?

::: js/src/gc/AtomMarking.cpp
@@ +92,5 @@
> +AtomMarkingRuntime::unregisterArena(Arena* arena)
> +{
> +    MOZ_ASSERT(arena->zone->isAtomsZone());
> +
> +    if (!freeArenaIndexes.emplaceBack(arena->atomBitmapStart())) {}

This needs a comment if we're intentionally leaking it on OOM.

@@ +173,5 @@
> +    // the chunk mark bitmaps. If this allocation fails then fall back to
> +    // updating the chunk mark bitmaps separately for each zone.
> +    Bitmap markedUnion;
> +    if (EnsureBitmapLength(markedUnion, allocatedWords)) {
> +        for (size_t i = 1; i < runtime->gc.zones.length(); i++) {

You can use this to iterate all zones excluding the atoms zone:

  for (ZonesIter zone(runtime, SkipAtoms); !zone.done(); zone.next())

Also, I think we only need to do this for zones that are not being collected so we can check |zone->isCollecting()| here.

@@ +182,5 @@
> +        }
> +        AddBitmapToChunkMarkBits(runtime, markedUnion);
> +    } else {
> +        for (size_t i = 1; i < runtime->gc.zones.length(); i++) {
> +            Zone* zone = runtime->gc.zones[i];

Ditto above.

@@ +201,5 @@
> +static bool
> +ThingIsPermanent(TenuredCell* thing)
> +{
> +    JS::TraceKind kind = thing->getTraceKind();
> +    if (kind == JS::TraceKind::String && reinterpret_cast<JSString*>(thing)->isPermanentAtom())

Please use static_cast here and below.

@@ +243,5 @@
> +{
> +    if (value.isGCThing()) {
> +        Cell* thing = value.toGCThing();
> +        if (thing && !IsInsideNursery(thing))
> +            markAtom(cx, static_cast<TenuredCell*>(thing));

Please use |&thing.asTenured()| here.

@@ +278,5 @@
> +        return true;
> +
> +    JS::TraceKind kind = thing->getTraceKind();
> +    if (kind == JS::TraceKind::String) {
> +        JSAtom* atom = reinterpret_cast<JSAtom*>(thing);

Please use static_cast.

We should probable add an as<T>() method on Cell that casts and asserts the kind.  I'll file a bug.

::: js/src/jscntxtinlines.h
@@ +87,5 @@
>          MOZ_ASSERT(!str->isMarked(gc::GRAY));
> +        if (str->isAtom()) {
> +            MOZ_ASSERT_IF(compartment,
> +                          compartment->runtimeFromAnyThread()->atomMarking()
> +                              .atomIsMarked(compartment->zone(), str));

Please common up these assertions and add a message/comment explaining the missing call to MarkAtom.

::: js/src/jsgc.cpp
@@ +4881,5 @@
>  /* virtual */ void
>  SweepAtomsTask::run()
>  {
> +    AtomMarkingRuntime::Bitmap marked;
> +    if (runtime->atomMarking().computeBitmapFromChunkMarkBits(runtime, marked)) {

I guess this is safe if we fail due to OOM here since this call will only ever unset bits from the bitmap.  Is that right?  It could do with a comment to explain that.

@@ +4882,5 @@
>  SweepAtomsTask::run()
>  {
> +    AtomMarkingRuntime::Bitmap marked;
> +    if (runtime->atomMarking().computeBitmapFromChunkMarkBits(runtime, marked)) {
> +        for (GCZoneGroupIter zone(runtime); !zone.done(); zone.next())

I think we want to do this for all collected zones, in which case you need GCZonesIter here rather than GCZoneGroupIter (the latter will iterate over all zones in the group currently being swept which in an incremental GC will likely only only contain the atoms zone).

Annoyingly there seems to be an assertion that prevents this from working at the moment.  I've filed bug 1328967 for that.

::: js/src/vm/Runtime.h
@@ +1057,5 @@
>      js::SymbolRegistry symbolRegistry_;
>  
> +    // State used for managing atom mark bitmaps in each zone. Protected by the
> +    // exclusive access lock.
> +    js::gc::AtomMarkingRuntime atomMarking_;

This is GC state so it should probably live in GCRuntime.

@@ +1071,5 @@
>          return *atoms_;
>      }
> +    js::AtomSet& unsafeAtoms() {
> +        return *atoms_;
> +    }

Are these only used during GC?  If so we can call them atomsUnderGC() or similar and assert |CurrentThreadIsPerformingGC()|.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Brian Hackett (:bhackett) from comment #4)
> So the presence of the bitmaps increased the total size used by atoms by
> about 10%.

Oh, and this doesn't sound too bad, but we should look at ways of reducing it.  Did you have something in mind?  A bitmap with one bit per atom would be much better from a memory point of view and might not be too slow.  Even better might be some kind of sparse bitmap because we could avoid process areas where there were no bits set.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> I'm tempted to suggest renaming all the MarkAtom methods to WrapAtom
> instead. They don't wrap anything, but the operation is analogous to
> wrapping and happens in the same situation of using a GC thing in a
> different compartment so I think it would be easier to explain and for
> people to remember to do it. What do you think?

Well, WrapAtom would imply an inout sort of operation (it doesn't just take the atom, it also produces a new one) which isn't the case here.  What about MarkCrossZoneAtom?

> ::: js/src/jsgc.cpp
> @@ +4881,5 @@
> >  /* virtual */ void
> >  SweepAtomsTask::run()
> >  {
> > +    AtomMarkingRuntime::Bitmap marked;
> > +    if (runtime->atomMarking().computeBitmapFromChunkMarkBits(runtime, marked)) {
> 
> I guess this is safe if we fail due to OOM here since this call will only
> ever unset bits from the bitmap.  Is that right?  It could do with a comment
> to explain that.

Yes, this is correct.

> @@ +4882,5 @@
> >  SweepAtomsTask::run()
> >  {
> > +    AtomMarkingRuntime::Bitmap marked;
> > +    if (runtime->atomMarking().computeBitmapFromChunkMarkBits(runtime, marked)) {
> > +        for (GCZoneGroupIter zone(runtime); !zone.done(); zone.next())
> 
> I think we want to do this for all collected zones, in which case you need
> GCZonesIter here rather than GCZoneGroupIter (the latter will iterate over
> all zones in the group currently being swept which in an incremental GC will
> likely only only contain the atoms zone).

SweepAtomsTask is constructed by GCRuntime::beginSweepingZoneGroup, which seems to be spawned for each swept zone group (i.e. it has its own GCZoneGroupIter calls).  Since the atoms task is both spawned and joined within this method, shouldn't it be ok to use GCZoneGroupIter here?

> @@ +1071,5 @@
> >          return *atoms_;
> >      }
> > +    js::AtomSet& unsafeAtoms() {
> > +        return *atoms_;
> > +    }
> 
> Are these only used during GC?  If so we can call them atomsUnderGC() or
> similar and assert |CurrentThreadIsPerformingGC()|.

No, AtomIsPinnedInRuntime also uses this method, though in that case it has exclusive access but is unable to supply an AutoLockForExclusiveAccess& to satisfy JSRuntime::atoms().  TBH this method is really a placeholder.  Handling of the exclusive access lock during GC is not great since CurrentThreadIsPerformingGC can be true for multiple threads simultaneously and we don't have any checks to make sure that they aren't racing on data protected by the lock.  As part of bug 1323066 I want to beef up and clarify the synchronization properties of the engine in a way that more straightforwardly guarantees exclusive access.

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Jon Coppeard (:jonco) from comment #6)
> (In reply to Brian Hackett (:bhackett) from comment #4)
> > So the presence of the bitmaps increased the total size used by atoms by
> > about 10%.
> 
> Oh, and this doesn't sound too bad, but we should look at ways of reducing
> it.  Did you have something in mind?  A bitmap with one bit per atom would
> be much better from a memory point of view and might not be too slow.  Even
> better might be some kind of sparse bitmap because we could avoid process
> areas where there were no bits set.

The original WIP patch (whose complexity you were concerned about in comment 1) uses one bit per atom.  It might not get the bitmap overhead all the way down to 2% but it should be close, and with that working we shouldn't need to use a sparse bitmap (which would bring an array of performance/complexity issues and should be avoided I think).

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 8821894 [details] [diff] [review]
patch

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

(In reply to Brian Hackett (:bhackett) from comment #7)

> Well, WrapAtom would imply an inout sort of operation (it doesn't just take
> the atom, it also produces a new one) which isn't the case here.  What about
> MarkCrossZoneAtom?

Agreed.  Yes, MarkCrossZone atom sounds good.

> SweepAtomsTask is constructed by GCRuntime::beginSweepingZoneGroup, which
> seems to be spawned for each swept zone group (i.e. it has its own
> GCZoneGroupIter calls).  Since the atoms task is both spawned and joined
> within this method, shouldn't it be ok to use GCZoneGroupIter here?

SweepAtomsTask is constructed there, but the task is only started if the current zone group contains the atoms zone so SweepAtomsTask::run is only executed once per GC.  In which case, I think you still need GCZonesIter here.

> Handling of the exclusive access lock during GC is not great since
> CurrentThreadIsPerformingGC can be true for multiple threads simultaneously
> and we don't have any checks to make sure that they aren't racing on data
> protected by the lock.  As part of bug 1323066 I want to beef up and clarify
> the synchronization properties of the engine in a way that more
> straightforwardly guarantees exclusive access.

That's true.  Any ideas for improvement are welcome.

> The original WIP patch (whose complexity you were concerned about in comment
> 1) uses one bit per atom.  It might not get the bitmap overhead all the way
> down to 2% but it should be close, and with that working we shouldn't need
> to use a sparse bitmap (which would bring an array of performance/complexity
> issues and should be avoided I think).

Do you think it would be too slow to use a bitmap with one bit per atom?  The cell sizes / offsets are known at compile time so we can have functions templated on the thing kind to read/write this data.

I was concerned about the interleaving of bitmaps because of the potential for data from unrelated arenas to affect each other if there is a bug and because it's a surprising (to me at least) implementation.  I guess maybe it's OK.  It would need some unit tests exercising this functionality directly though and assertions that we're not setting and bits that we don't expect to.

Anyway, r=me for the js parts of this patch.

Comment 10

[Brian Hackett [Laid off!]]

Sorry for the delay in getting back to this, bug 1325050 took longer than I was hoping for.

(In reply to Jon Coppeard (:jonco) from comment #9)
> Comment on attachment 8821894 [details] [diff] [review]

> Do you think it would be too slow to use a bitmap with one bit per atom? 
> The cell sizes / offsets are known at compile time so we can have functions
> templated on the thing kind to read/write this data.

Well, the division done when setting bits in the atom mark bitmaps is only part of the slowness (and templates would indeed fix this).  When we take the atom mark bitmaps and use them to set bits in the chunk bitmaps, we would need to scale those bits back up (if the atom mark bitmap is 1011 and the thing is 24 bytes, the chunk bitmap pattern is 100000100100).  AFAIK this can only be done by iterating through the atom mark bitmap bit-by-bit, and it would be awfully nice to do it word-by-word instead, which is possible with the interleaved bit representation.

> I was concerned about the interleaving of bitmaps because of the potential
> for data from unrelated arenas to affect each other if there is a bug and
> because it's a surprising (to me at least) implementation.  I guess maybe
> it's OK.  It would need some unit tests exercising this functionality
> directly though and assertions that we're not setting and bits that we don't
> expect to.

OK, sure.  It is a strange implementation but at least it's pretty self contained.

I'll land this patch as is (well, after the non-JS parts get reviewed) and then file a followup to look into a more efficient bit representation.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: rebased

The full patch, rebased and updated per review comments.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: DOM bindings changes

Comment 13

[Brian Hackett [Laid off!]]

Attachment: NPRuntime changes

Comment 14

[Brian Hackett [Laid off!]]

Attachment: XPConnect changes

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8828860 [details] [diff] [review]
NPRuntime changes

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

So this JS_MarkCrossZoneId() is basically used like a "wrap" you'd do for an object when you enter a new compartment? Would it be possible to add something like the compartment assertions we have to check for this property, on methods like JS_HasPropertyById and JS_GetPropertyById. Basically, so that we get an assertion any time somebody crosses a zone without calling this method, rather than only if a GC is triggered at just the right point, and then later we get a mysterious use-after-sweep error or something. Unless that's already the case.

Comment 16

[Andrew McCreight [:mccr8]]

Honestly, it kind of feels like you could just add some JS_MarkCrossZoneId calls inside these various JS API functions and avoid having API users worry about sprinkling around these calls in the right place. Is there ever a time we don't want to call JS_MarkCrossZoneId on the first and third argument to JS_HasPropertyById? Is it too expensive to just do unconditionally?

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 8828862 [details] [diff] [review]
XPConnect changes

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

I guess this is fine, but it would be nice if this API was harder to use incorrectly.

Comment 18

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Comment on attachment 8828860 [details] [diff] [review]
> NPRuntime changes
> 
> Review of attachment 8828860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this JS_MarkCrossZoneId() is basically used like a "wrap" you'd do for an
> object when you enter a new compartment? Would it be possible to add
> something like the compartment assertions we have to check for this
> property, on methods like JS_HasPropertyById and JS_GetPropertyById.
> Basically, so that we get an assertion any time somebody crosses a zone
> without calling this method, rather than only if a GC is triggered at just
> the right point, and then later we get a mysterious use-after-sweep error or
> something. Unless that's already the case.

Yes, the cross compartment assertions will now check that atoms are marked in the target zone.  There are also a lot of internal assertions in the JS engine that we never encounter an unmarked atom while tracing, and aren't putting unmarked atoms in places like shape ids and object properties.

(In reply to Andrew McCreight [:mccr8] from comment #16)
> Honestly, it kind of feels like you could just add some JS_MarkCrossZoneId
> calls inside these various JS API functions and avoid having API users worry
> about sprinkling around these calls in the right place. Is there ever a time
> we don't want to call JS_MarkCrossZoneId on the first and third argument to
> JS_HasPropertyById? Is it too expensive to just do unconditionally?

It would be fine to always call JS_MarkCrossZoneId at these API entry points, but, yeah, I was concerned about the overhead.  As you point out, cross zone atom marking is directly analogous to cross compartment wrapping.  For this API I was mainly just following along with what we already do for cross compartment wrappers, i.e. require the caller to do the wrapping but have robust checks in the API they have done so.

The main difference between this and wrapping is that the atom pointers will be the same in the source and target zone/compartment.  If the atom just happens to already be marked in the target zone, then the atom marking assertion won't fire even if the caller is missing a JS_MarkCrossZoneId call that they should have.  No bad behavior will result from this, but it might make atom marking bugs harder to find in testing.

It seems like it would be nice --- for both this and cross compartment wrappers --- if we had different variants of the APIs that had more or less input validation / fixing up.  It shouldn't be that expensive for the JS engine to do the atom marking itself at an API entry point (it is just some pointer chasing and bitwise operations) but it would be measurable on heavy workloads.  If we made JS_HasPropertyById and friends both mark their input ids and wrap their input objects/values into the context's compartment, using the method would be less error prone.  We could then have a variant where the burden is on the caller to make sure the compartments and atom marking are correct (the callee will still check #ifdef JS_CRASH_DIAGNOSTICS) and which can be used only in places that are demonstrably hot.  Maybe:

enum MaybeValidateInputs {
  DontValidateInputs,
  ValidateInputs
};

template <MaybeValidateInputs = ValidateInputs>
JS_HasPropertyById(...);

Comment 19

[Andrew McCreight [:mccr8]]

I guess we can just leave it as is for now, and if it becomes a big pain for people in practice, something fancier can get added to do stuff automatically.

Comment 20

[Jan de Mooij [:jandem]]

We should at the very least audit all APIs that take a jsid/atom/symbol, and make sure it's passed to assertSameCompartment, right?

I added some assertSameCompartment calls to various APIs recently and it found a number of bad bugs, but I didn't check ids/atoms because assertSameCompartment used to be a no-op for these types...

Comment 21

[Brian Hackett [Laid off!]]

Sure, I'll scan the external APIs to make sure assertSameCompartment is used.

> grep JS_.*_API *.cpp */*.cpp | wc
873

Yeesh...

Comment 22

[Brian Hackett [Laid off!]]

Attachment: add barrier

This patch adds a barrier that fixes an intermittent error that showed up during testing.  The problem is that during an incremental GC, if an atom reference moves from a zone that is not being collected to one that is being collected, we are effectively reading from an untraced location and need to trigger a read barrier on the atom.  This wasn't a problem before this patch because we wouldn't mark the atoms at all if we weren't collecting from all zones.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7311c06a7271
Mark atoms separately in each zone, r=jonco,mccr8,peterv.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7311c06a7271

Comment 25

[Nicholas Nethercote [inactive]]

bhackett: this patch added measurement of atomsMarkBitmaps to the Runtime, even though those structures are stored within Zones. So it looks like the Runtime-level measurement is the sum of all the zone measurements. Is there a reason you didn't just report the measurements at the Zone level? Thanks.

Comment 26

[Brian Hackett [Laid off!]]

(In reply to Nicholas Nethercote [:njn] from comment #25)
> bhackett: this patch added measurement of atomsMarkBitmaps to the Runtime,
> even though those structures are stored within Zones. So it looks like the
> Runtime-level measurement is the sum of all the zone measurements. Is there
> a reason you didn't just report the measurements at the Zone level? Thanks.

I measured things this way because the atom bitmaps are managed by a runtime-wide structure, AtomMarkingRuntime, and because (originally, anyways) the bitmaps were about the same size in each zone so any memory regression would be of a death-by-a-thousand-cuts nature and easier to catch with a runtime wide measurement.  The latter has changed since bug 1341006 (which was, incidentally, much more obvious with the runtime wide measurement) so it might make more sense now to use a per zone measurement.

Comment 27

[Jon Coppeard (:jonco)]

Comment on attachment 8821894 [details] [diff] [review]
patch

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

::: js/src/jsgc.cpp
@@ +3819,5 @@
>       * main thread.
> +     *
> +     * Otherwise, we always schedule a GC in the atoms zone so that atoms which
> +     * the other collected zones are using are marked, and we can update the
> +     * set of atoms in use by the other collected zones at the end of the GC.

I just came across this comment again.  Did you intend the atoms zone to be collected in every GC?  The code still requires the atoms zone to be scheduled to be collected, although it can now be collected in non-full GCs.

Comment 28

[Brian Hackett [Laid off!]]

(In reply to Jon Coppeard (:jonco) from comment #27)
> Comment on attachment 8821894 [details] [diff] [review]
> patch
> 
> Review of attachment 8821894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsgc.cpp
> @@ +3819,5 @@
> >       * main thread.
> > +     *
> > +     * Otherwise, we always schedule a GC in the atoms zone so that atoms which
> > +     * the other collected zones are using are marked, and we can update the
> > +     * set of atoms in use by the other collected zones at the end of the GC.
> 
> I just came across this comment again.  Did you intend the atoms zone to be
> collected in every GC?  The code still requires the atoms zone to be
> scheduled to be collected, although it can now be collected in non-full GCs.

Yes, from what I saw while measuring the effects of this patch collecting the atoms zone didn't take very long, and is done off thread anyhow.  If the atoms zone is not collected by a GC then the mark bits in the other collected zones are not updated.  It would be fine to have another heuristic for deciding whether to collect the atoms zone during a GC, or to schedule the atoms zone for collection (so that the mark bits are updated) but not actually sweep dead atoms at the end, but this seemed unnecessarily complicated.