Bug 1624842 - Update base shapes on the main thread after compacting GC r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 25 Mar 2020 22:36:29 +0000
changeset 520483 57e989c8a37cbe84810e37dce41331a77847bd66
parent 520482 f72280a54c6e1889d33de7a4e79d4fb0b3927c11
child 520484 ee0d41c18c40e8364705ff6609442c7895a4ec17
push id37252
push usermalexandru@mozilla.com
push dateThu, 26 Mar 2020 15:34:27 +0000
treeherdermozilla-central@31360ced8ff8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1624842
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1624842 - Update base shapes on the main thread after compacting GC r=sfink Previously we updated base shapes in parallel on helper threads, but this can cause a race with updating shapes. The race occurs between getting a base shape's unowned base shape in fixupShapeTreeAfterMovingGC and updating that field of the base shape. I don't think this race is currently causing problems. The fix is to update base shapes on the main thread along with shapes (which are already updated on the main thread). It's unlikely that this will make much difference to performance due to the relatively small number of base shapes. I took the opportunity to refactor Shape::fixupShapeTreeAfterMovingGC slightly. Differential Revision: https://phabricator.services.mozilla.com/D68206
js/src/gc/GC.cpp
js/src/vm/Shape.cpp
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -2183,17 +2183,18 @@ static void UpdateArenaPointersTyped(Mov
 }
 
 static bool CanUpdateKindInBackground(AllocKind kind) {
   // We try to update as many GC things in parallel as we can, but there are
   // kinds for which this might not be safe:
   //  - we assume JSObjects that are foreground finalized are not safe to
   //    update in parallel
   //  - updating a shape touches child shapes in fixupShapeTreeAfterMovingGC()
-  return js::gc::IsBackgroundFinalized(kind) && !IsShapeAllocKind(kind);
+  return js::gc::IsBackgroundFinalized(kind) && !IsShapeAllocKind(kind) &&
+         kind != AllocKind::BASE_SHAPE;
 }
 
 /*
  * Update the internal pointers for all cells in an arena.
  */
 static void UpdateArenaPointers(MovingTracer* trc, Arena* arena) {
   AllocKind kind = arena->getAllocKind();
 
--- a/js/src/vm/Shape.cpp
+++ b/js/src/vm/Shape.cpp
@@ -1950,29 +1950,19 @@ void Shape::fixupShapeTreeAfterMovingGC(
       children.setSingleShape(gc::Forwarded(children.toSingleShape()));
     }
     return;
   }
 
   MOZ_ASSERT(children.isShapeSet());
   ShapeSet* set = children.toShapeSet();
   for (ShapeSet::Enum e(*set); !e.empty(); e.popFront()) {
-    Shape* key = e.front();
-    if (IsForwarded(key)) {
-      key = Forwarded(key);
-    }
-
-    BaseShape* base = key->base();
-    if (IsForwarded(base)) {
-      base = Forwarded(base);
-    }
-    UnownedBaseShape* unowned = base->unowned();
-    if (IsForwarded(unowned)) {
-      unowned = Forwarded(unowned);
-    }
+    Shape* key = MaybeForwarded(e.front());
+    BaseShape* base = MaybeForwarded(key->base());
+    UnownedBaseShape* unowned = MaybeForwarded(base->unowned());
 
     GetterOp getter = key->getter();
     if (key->hasGetterObject()) {
       getter = GetterOp(MaybeForwarded(key->getterObject()));
     }
 
     SetterOp setter = key->setter();
     if (key->hasSetterObject()) {