1369444 - Sweep the atoms table incrementally

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1369444-sweep-atoms-incrementally

This patch splits atom sweeping into two parts - updating the atoms bitmaps and sweeping the main atoms table incrementally.

Incremental sweeping works by creating a secondary table for atoms allocated while we're sweeping the main table.  When atomising we check whether we are currently sweeping and if so check both tables, adding any new atoms to the secondary table.  We add in any new atoms at the end of sweeping.

I investigated using an ordered hash table for this instead and it worked well but increased memory usage by > 50%.  The atoms table can already take a couple of MiB so I figured we should probably not use this.  I think that approach would work well for some of the smaller tables however.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1167937e2b4948b906db97c3570d8704e3b3b33b

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8873517 [details] [diff] [review]
bug1369444-sweep-atoms-incrementally

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

This is great! It came together nicely. I sort of want a table that encapsulates the two-table switchover stuff, but it would be a lot of boilerplate for just one usage. Maybe if we want a bunch more of these.

::: js/src/jsgc.cpp
@@ +5515,5 @@
> +        JSAtom* atom = atomsToSweep.front().asPtrUnbarriered();
> +        if (IsAboutToBeFinalizedUnbarriered(&atom))
> +            atomsToSweep.removeFront();
> +        atomsToSweep.popFront();
> +    }

The state is all local, so this could be a little simpler:

  while (!atomsToSweep.empty()) {
    if (budget.isOverBudget())
      return NotFinished;
    ...sweep the entry...
  }

@@ +5530,5 @@
> +            oomUnsafe.crash("Adding atom from secondary table after sweep");
> +    }
> +    rt->destroyAtomsAddedWhileSweepingTable();
> +
> +    maybeAtoms.reset();

won't the reset() happen in the destructor anyway?

::: js/src/vm/Runtime.h
@@ +848,5 @@
>      }
> +
> +    bool createAtomsAddedWhileSweepingTable();
> +    void destroyAtomsAddedWhileSweepingTable();
> +    js::AtomSet*& atomsAddedWhileSweeping() {

Why AtomSet*& ? I didn't seen anywhere you modified this lvalue. Why not just AtomSet*?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> The state is all local, so this could be a little simpler:

Good idea, tidied as suggested.

> > +    maybeAtoms.reset();
> 
> won't the reset() happen in the destructor anyway?

maybeAtoms is a reference to the sweeping state in the GCRuntime to this needs to happen here.

> Why AtomSet*& ? I didn't seen anywhere you modified this lvalue. Why not
> just AtomSet*?

Oh yeah, fixed.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b894115e89
Sweep the atoms table incrementally r=sfink

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98b894115e89