Bug 1706694 - Don't overwrite leftmost rope's internals at the start when reusing the leftmost child buffer in string flattening r=jandem
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 28 Apr 2021 08:55:53 +0000
changeset 577793 1d6f87631df729ae076d9d8ce5a4ae7f54491340
parent 577792 a47a2921119cea73dcc8fd97b85501deefab2f2e
child 577794 abfa26f32f96833e7c7f925c23e7851de4b4d3e3
push id38416
push usermalexandru@mozilla.com
push dateWed, 28 Apr 2021 21:55:23 +0000
treeherdermozilla-central@1005c275ed0f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1706694
milestone90.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 1706694 - Don't overwrite leftmost rope's internals at the start when reusing the leftmost child buffer in string flattening r=jandem This is unnecessary and was causing problems. We can leave the leftmost rope to be handled in the same way as all the other ropes, and move the special case to just avoid copying the chars when reusing the leftmost buffer. The problem was that now we always barrier ropes, before overwriting the left child pointer with the parent pointer. If this happens during incremental GC we must ensure we haven't already overwritten this pointer we the buffer pointer. This patch also moves changing the leftmost string into a dependent string until the end. Depends on D113316 Differential Revision: https://phabricator.services.mozilla.com/D113503
js/src/vm/StringType.cpp
--- a/js/src/vm/StringType.cpp
+++ b/js/src/vm/StringType.cpp
@@ -617,25 +617,25 @@ static bool UpdateNurseryBuffersOnTransf
     // Leftmost child is giving its nursery-held chars buffer to a
     // tenured string.
     nursery.removeMallocedBuffer(buffer, size);
   }
 
   return true;
 }
 
-static bool CanReuseLeftmostBuffer(JSRope* leftmostRope, size_t wholeLength,
+static bool CanReuseLeftmostBuffer(JSString* leftmostChild, size_t wholeLength,
                                    bool hasTwoByteChars) {
-  if (!leftmostRope->leftChild()->isExtensible()) {
+  if (!leftmostChild->isExtensible()) {
     return false;
   }
 
-  JSExtensibleString& leftmostChild = leftmostRope->leftChild()->asExtensible();
-  return leftmostChild.capacity() >= wholeLength &&
-         leftmostChild.hasTwoByteChars() == hasTwoByteChars;
+  JSExtensibleString& str = leftmostChild->asExtensible();
+  return str.capacity() >= wholeLength &&
+         str.hasTwoByteChars() == hasTwoByteChars;
 }
 
 JSLinearString* JSRope::flatten(JSContext* maybecx) {
   mozilla::Maybe<AutoGeckoProfilerEntry> entry;
   if (maybecx && !maybecx->isHelperThreadContext()) {
     entry.emplace(maybecx, "JSRope::flatten");
   }
 
@@ -728,59 +728,34 @@ JSLinearString* JSRope::flattenInternal(
 
   Nursery& nursery = root->runtimeFromMainThread()->gc.nursery();
 
   /* Find the left most string, containing the first string. */
   JSRope* leftmostRope = root;
   while (leftmostRope->leftChild()->isRope()) {
     leftmostRope = &leftmostRope->leftChild()->asRope();
   }
+  JSString* leftmostChild = leftmostRope->leftChild();
 
   bool reuseLeftmostBuffer = CanReuseLeftmostBuffer(
-      leftmostRope, wholeLength, std::is_same_v<CharT, char16_t>);
-
-  uint32_t leftmostChildLength = 0;  // Only set if we reuse leftmost buffer.
+      leftmostChild, wholeLength, std::is_same_v<CharT, char16_t>);
 
   if (reuseLeftmostBuffer) {
-    JSExtensibleString& left = leftmostRope->leftChild()->asExtensible();
-    size_t capacity = left.capacity();
+    JSExtensibleString& left = leftmostChild->asExtensible();
+    wholeCapacity = left.capacity();
     wholeChars = const_cast<CharT*>(left.nonInlineChars<CharT>(nogc));
-    wholeCapacity = capacity;
 
     // Nursery::registerMallocedBuffer is fallible, so attempt it first before
     // doing anything irreversible.
     if (!UpdateNurseryBuffersOnTransfer(nursery, &left, root, wholeChars,
                                         wholeCapacity * sizeof(CharT))) {
       return nullptr;
     }
-
-    ropeBarrierDuringFlattening<usingBarrier>(leftmostRope);
-    leftmostRope->setNonInlineChars(wholeChars);
-    leftmostChildLength = left.length();
-
-    // Remove memory association for left node we're about to make into a
-    // dependent string.
-    if (left.isTenured()) {
-      RemoveCellMemory(&left, left.allocSize(), MemoryUse::StringContents);
-    }
-
-    uint32_t flags = INIT_DEPENDENT_FLAGS;
-    if (left.inStringToAtomCache()) {
-      flags |= IN_STRING_TO_ATOM_CACHE;
-    }
-    left.setLengthAndFlags(leftmostChildLength,
-                           StringFlagsForCharType<CharT>(flags));
-    left.d.s.u3.base = (JSLinearString*)root; /* will be true on exit */
-    if (left.isTenured() && !root->isTenured()) {
-      // leftmost child -> root is a tenured -> nursery edge.
-      root->storeBuffer()->putWholeCell(&left);
-    }
   } else {
     // If we can't reuse the leftmost child's buffer, allocate a new one.
-
     if (!AllocChars(root, wholeLength, &wholeChars, &wholeCapacity)) {
       return nullptr;
     }
 
     if (!root->isTenured()) {
       if (!nursery.registerMallocedBuffer(wholeChars,
                                           wholeCapacity * sizeof(CharT))) {
         js_free(wholeChars);
@@ -802,29 +777,26 @@ first_visit_node : {
   ropeBarrierDuringFlattening<usingBarrier>(str);
 
   JSString& left = *str->d.s.u2.left;
   str->d.s.u2.parent = parent;
   str->setFlagBit(parentFlag);
   parent = nullptr;
   parentFlag = 0;
 
-  if (reuseLeftmostBuffer && str == leftmostRope) {
-    // Left child has already been overwritten.
-    pos += leftmostChildLength;
-    goto visit_right_child;
-  }
   if (left.isRope()) {
     /* Return to this node when 'left' done, then goto visit_right_child. */
     parent = str;
     parentFlag = FLATTEN_VISIT_RIGHT;
     str = &left;
     goto first_visit_node;
   }
-  CopyChars(pos, left.asLinear());
+  if (!(reuseLeftmostBuffer && &left == leftmostChild)) {
+    CopyChars(pos, left.asLinear());
+  }
   pos += left.length();
 }
 
 visit_right_child : {
   JSString& right = *str->d.s.u3.right;
   if (right.isRope()) {
     /* Return to this node when 'right' done, then goto finish_node. */
     parent = str;
@@ -876,22 +848,43 @@ finish_node : {
   goto visit_right_child;
 }
 
 finish_root:
   // We traversed all the way back up to the root so we're finished.
   MOZ_ASSERT(str == root);
   MOZ_ASSERT(pos == wholeChars + wholeLength);
 
-  str->setLengthAndFlags(wholeLength,
-                         StringFlagsForCharType<CharT>(EXTENSIBLE_FLAGS));
-  str->setNonInlineChars(wholeChars);
-  str->d.s.u3.capacity = wholeCapacity;
-  if (str->isTenured()) {
-    AddCellMemory(str, str->asLinear().allocSize(), MemoryUse::StringContents);
+  root->setLengthAndFlags(wholeLength,
+                          StringFlagsForCharType<CharT>(EXTENSIBLE_FLAGS));
+  root->setNonInlineChars(wholeChars);
+  root->d.s.u3.capacity = wholeCapacity;
+  if (root->isTenured()) {
+    AddCellMemory(root, root->asLinear().allocSize(),
+                  MemoryUse::StringContents);
+  }
+
+  if (reuseLeftmostBuffer) {
+    // Remove memory association for left node we're about to make into a
+    // dependent string.
+    JSString& left = *leftmostChild;
+    if (left.isTenured()) {
+      RemoveCellMemory(&left, left.allocSize(), MemoryUse::StringContents);
+    }
+
+    uint32_t flags = INIT_DEPENDENT_FLAGS;
+    if (left.inStringToAtomCache()) {
+      flags |= IN_STRING_TO_ATOM_CACHE;
+    }
+    left.setLengthAndFlags(left.length(), StringFlagsForCharType<CharT>(flags));
+    left.d.s.u3.base = &root->asLinear();
+    if (left.isTenured() && !root->isTenured()) {
+      // leftmost child -> root is a tenured -> nursery edge.
+      root->storeBuffer()->putWholeCell(&left);
+    }
   }
 
   return &root->asLinear();
 }
 
 template <JSRope::UsingBarrier usingBarrier>
 /* static */
 inline void JSRope::ropeBarrierDuringFlattening(JSString* str) {