Sweep the atoms table incrementally

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf])

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8873517 [details] [diff] [review]
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
Attachment #8873517 - Flags: review?(sphink)
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*?
Attachment #8873517 - Flags: review?(sphink) → review+
(Assignee)

Comment 3

a year ago
(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

a year ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b894115e89
Sweep the atoms table incrementally r=sfink

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98b894115e89
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1370069
(Assignee)

Updated

a year ago
No longer blocks: 1367727
(Assignee)

Updated

a year ago
Blocks: 1368761

Updated

a year ago
Depends on: 1375445
You need to log in before you can comment on or make changes to this bug.