Bug 1052579 - Refactor and cleanup for JS StringBuffer class r=sfink
authorChris Martin <cmartin@mozilla.com>
Tue, 09 Apr 2019 12:56:55 +0000
changeset 468564 18a310712e500571d2080ad0c175427122d7423e
parent 468563 660471b4a2ada83f46bcb2f072f1c69a1d317724
child 468565 a16ab9d38fe748d125dad628afd99d442185f9e5
push id82665
push userrvandermeulen@mozilla.com
push dateTue, 09 Apr 2019 13:29:57 +0000
treeherderautoland@18a310712e50 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1052579
milestone68.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 1052579 - Refactor and cleanup for JS StringBuffer class r=sfink Refactored a couple of minor issues with JS StringBuffer, partly to prepare it to handle the new JSString buffer arena. 1. There were public functions that were returning mutable references to private members, making them not-really "private" at all. They are now hidden and new API calls were added to provide needed functionality. 2. ExtractWellSized() now uses the buffer's policy to resize/free it instead of directly calling JS alloc API 3. Changed FinishStringFlat to no longer take a reference to a StringBuffer and a reference to a StringBuffer member Differential Revision: https://phabricator.services.mozilla.com/D25706
js/src/builtin/JSON.cpp
js/src/util/StringBuffer.cpp
js/src/util/StringBuffer.h
--- a/js/src/builtin/JSON.cpp
+++ b/js/src/builtin/JSON.cpp
@@ -133,66 +133,71 @@ static MOZ_ALWAYS_INLINE RangedPtr<DstCh
     *dstPtr++ = ToLowerHex(as32 & 0xF);
   }
 
   /* Steps 3-4. */
   *dstPtr++ = '"';
   return dstPtr;
 }
 
-template <typename SrcCharT, typename CharVectorT>
-static bool Quote(JSContext* cx, CharVectorT& sb, JSLinearString* str) {
-  // We resize the backing buffer to the maximum size we could possibly need,
-  // write the escaped string into it, and shrink it back to the size we ended
-  // up needing.
-
-  size_t len = str->length();
-  CheckedInt<size_t> reservedLen = CheckedInt<size_t>(len) * 6 + 2;
-  if (MOZ_UNLIKELY(!reservedLen.isValid())) {
-    ReportAllocationOverflow(cx);
-    return false;
-  }
-
-  size_t sbInitialLen = sb.length();
-  if (!sb.growByUninitialized(reservedLen.value())) {
-    return false;
-  }
-
-  typedef typename CharVectorT::ElementType DstCharT;
+template <typename SrcCharT, typename DstCharT>
+static size_t QuoteHelper(const JSLinearString& linear, StringBuffer& sb,
+                          size_t sbOffset) {
+  size_t len = linear.length();
 
   JS::AutoCheckCannotGC nogc;
-  RangedPtr<const SrcCharT> srcBegin{str->chars<SrcCharT>(nogc), len};
-  RangedPtr<DstCharT> dstBegin{sb.begin(), sb.begin(), sb.end()};
+  RangedPtr<const SrcCharT> srcBegin{linear.chars<SrcCharT>(nogc), len};
+  RangedPtr<DstCharT> dstBegin{sb.begin<DstCharT>(), sb.begin<DstCharT>(),
+                               sb.end<DstCharT>()};
   RangedPtr<DstCharT> dstEnd =
-      InfallibleQuote(srcBegin, srcBegin + len, dstBegin + sbInitialLen);
-  size_t newSize = dstEnd - dstBegin;
-  sb.shrinkTo(newSize);
-  return true;
+      InfallibleQuote(srcBegin, srcBegin + len, dstBegin + sbOffset);
+
+  return dstEnd - dstBegin;
 }
 
 static bool Quote(JSContext* cx, StringBuffer& sb, JSString* str) {
   JSLinearString* linear = str->ensureLinear(cx);
   if (!linear) {
     return false;
   }
 
-  // Check if either has non-latin1 before calling ensure, so that the buffer's
-  // hasEnsured flag is set if the converstion to twoByte was automatic.
-  if (!sb.isUnderlyingBufferLatin1() || linear->hasTwoByteChars()) {
-    if (!sb.ensureTwoByteChars()) {
-      return false;
-    }
+  if (linear->hasTwoByteChars() && !sb.ensureTwoByteChars()) {
+    return false;
   }
-  if (linear->hasTwoByteChars()) {
-    return Quote<char16_t>(cx, sb.rawTwoByteBuffer(), linear);
+
+  // We resize the backing buffer to the maximum size we could possibly need,
+  // write the escaped string into it, and shrink it back to the size we ended
+  // up needing.
+
+  size_t len = linear->length();
+  size_t sbInitialLen = sb.length();
+
+  CheckedInt<size_t> reservedLen = CheckedInt<size_t>(len) * 6 + 2;
+  if (MOZ_UNLIKELY(!reservedLen.isValid())) {
+    ReportAllocationOverflow(cx);
+    return false;
   }
 
-  return sb.isUnderlyingBufferLatin1()
-             ? Quote<Latin1Char>(cx, sb.latin1Chars(), linear)
-             : Quote<Latin1Char>(cx, sb.rawTwoByteBuffer(), linear);
+  if (!sb.growByUninitialized(reservedLen.value())) {
+    return false;
+  }
+
+  size_t newSize;
+
+  if (linear->hasTwoByteChars()) {
+    newSize = QuoteHelper<char16_t, char16_t>(*linear, sb, sbInitialLen);
+  } else if (sb.isUnderlyingBufferLatin1()) {
+    newSize = QuoteHelper<Latin1Char, Latin1Char>(*linear, sb, sbInitialLen);
+  } else {
+    newSize = QuoteHelper<Latin1Char, char16_t>(*linear, sb, sbInitialLen);
+  }
+
+  sb.shrinkTo(newSize);
+
+  return true;
 }
 
 namespace {
 
 using ObjectVector = GCVector<JSObject*, 8>;
 
 class StringifyContext {
  public:
--- a/js/src/util/StringBuffer.cpp
+++ b/js/src/util/StringBuffer.cpp
@@ -10,45 +10,46 @@
 #include "mozilla/Unused.h"
 
 #include "vm/JSObject-inl.h"
 #include "vm/StringType-inl.h"
 
 using namespace js;
 
 template <typename CharT, class Buffer>
-static CharT* ExtractWellSized(JSContext* cx, Buffer& cb) {
+static CharT* ExtractWellSized(Buffer& cb) {
   size_t capacity = cb.capacity();
   size_t length = cb.length();
+  TempAllocPolicy allocPolicy = cb.allocPolicy();
 
   CharT* buf = cb.extractOrCopyRawBuffer();
   if (!buf) {
     return nullptr;
   }
 
   /* For medium/big buffers, avoid wasting more than 1/4 of the memory. */
   MOZ_ASSERT(capacity >= length);
   if (length > Buffer::sMaxInlineStorage && capacity - length > length / 4) {
-    CharT* tmp = cx->pod_realloc<CharT>(buf, capacity, length + 1);
+    CharT* tmp = allocPolicy.pod_realloc<CharT>(buf, capacity, length + 1);
     if (!tmp) {
-      js_free(buf);
+      allocPolicy.free_(buf);
       return nullptr;
     }
     buf = tmp;
   }
 
   return buf;
 }
 
 char16_t* StringBuffer::stealChars() {
   if (isLatin1() && !inflateChars()) {
     return nullptr;
   }
 
-  return ExtractWellSized<char16_t>(cx, twoByteChars());
+  return ExtractWellSized<char16_t>(twoByteChars());
 }
 
 bool StringBuffer::inflateChars() {
   MOZ_ASSERT(isLatin1());
 
   TwoByteCharBuffer twoByte(cx);
 
   /*
@@ -63,25 +64,32 @@ bool StringBuffer::inflateChars() {
 
   twoByte.infallibleAppend(latin1Chars().begin(), latin1Chars().length());
 
   cb.destroy();
   cb.construct<TwoByteCharBuffer>(std::move(twoByte));
   return true;
 }
 
-template <typename CharT, class Buffer>
-static JSFlatString* FinishStringFlat(JSContext* cx, StringBuffer& sb,
-                                      Buffer& cb) {
-  size_t len = sb.length();
-  if (!sb.append('\0')) {
+template <typename CharT>
+JSFlatString* StringBuffer::finishStringInternal(JSContext* cx) {
+  size_t len = length();
+
+  if (JSInlineString::lengthFits<CharT>(len)) {
+    mozilla::Range<const CharT> range(begin<CharT>(), len);
+    return NewInlineString<CanGC>(cx, range);
+  }
+
+  if (!append('\0')) {
     return nullptr;
   }
 
-  UniquePtr<CharT[], JS::FreePolicy> buf(ExtractWellSized<CharT>(cx, cb));
+  UniquePtr<CharT[], JS::FreePolicy> buf(
+      ExtractWellSized<CharT>(chars<CharT>()));
+
   if (!buf) {
     return nullptr;
   }
 
   JSFlatString* str = NewStringDontDeflate<CanGC>(cx, std::move(buf), len);
   if (!str) {
     return nullptr;
   }
@@ -105,30 +113,18 @@ JSFlatString* StringBuffer::finishString
     return nullptr;
   }
 
   JS_STATIC_ASSERT(JSFatInlineString::MAX_LENGTH_TWO_BYTE <
                    TwoByteCharBuffer::InlineLength);
   JS_STATIC_ASSERT(JSFatInlineString::MAX_LENGTH_LATIN1 <
                    Latin1CharBuffer::InlineLength);
 
-  if (isLatin1()) {
-    if (JSInlineString::lengthFits<Latin1Char>(len)) {
-      mozilla::Range<const Latin1Char> range(latin1Chars().begin(), len);
-      return NewInlineString<CanGC>(cx, range);
-    }
-  } else {
-    if (JSInlineString::lengthFits<char16_t>(len)) {
-      mozilla::Range<const char16_t> range(twoByteChars().begin(), len);
-      return NewInlineString<CanGC>(cx, range);
-    }
-  }
-
-  return isLatin1() ? FinishStringFlat<Latin1Char>(cx, *this, latin1Chars())
-                    : FinishStringFlat<char16_t>(cx, *this, twoByteChars());
+  return isLatin1() ? finishStringInternal<Latin1Char>(cx)
+                    : finishStringInternal<char16_t>(cx);
 }
 
 JSAtom* StringBuffer::finishAtom() {
   size_t len = length();
   if (len == 0) {
     return cx->names().empty;
   }
 
--- a/js/src/util/StringBuffer.h
+++ b/js/src/util/StringBuffer.h
@@ -23,81 +23,88 @@ namespace js {
  * Any operation which would exceed the maximum string length causes an
  * exception report on the context and results in a failed return value.
  *
  * Well-sized extractions (which waste no more than 1/4 of their char
  * buffer space) are guaranteed for strings built by this interface.
  * See |extractWellSized|.
  */
 class StringBuffer {
+  template <typename CharT>
+  using BufferType = Vector<CharT, 64 / sizeof(CharT)>;
+
   /*
    * The Vector's buffer may be either stolen or copied, so we need to use
    * TempAllocPolicy and account for the memory manually when stealing.
    */
-  typedef Vector<Latin1Char, 64> Latin1CharBuffer;
-  typedef Vector<char16_t, 32> TwoByteCharBuffer;
+  using Latin1CharBuffer = BufferType<Latin1Char>;
+  using TwoByteCharBuffer = BufferType<char16_t>;
 
   JSContext* cx;
 
   /*
    * If Latin1 strings are enabled, cb starts out as a Latin1CharBuffer. When
    * a TwoByte char is appended, inflateChars() constructs a TwoByteCharBuffer
    * and copies the Latin1 chars.
    */
   mozilla::MaybeOneOf<Latin1CharBuffer, TwoByteCharBuffer> cb;
 
-#ifdef DEBUG
-  /*
-   * Make sure ensureTwoByteChars() is called before calling
-   * infallibleAppend(char16_t).
-   */
-  bool hasEnsuredTwoByteChars_;
-#endif
-
   /* Number of reserve()'d chars, see inflateChars. */
   size_t reserved_;
 
   StringBuffer(const StringBuffer& other) = delete;
   void operator=(const StringBuffer& other) = delete;
 
-  MOZ_ALWAYS_INLINE bool isLatin1() const {
-    return cb.constructed<Latin1CharBuffer>();
+  template <typename CharT>
+  MOZ_ALWAYS_INLINE bool isCharType() const {
+    return cb.constructed<BufferType<CharT>>();
   }
-  MOZ_ALWAYS_INLINE bool isTwoByte() const { return !isLatin1(); }
+
+  MOZ_ALWAYS_INLINE bool isLatin1() const { return isCharType<Latin1Char>(); }
+
+  MOZ_ALWAYS_INLINE bool isTwoByte() const { return isCharType<char16_t>(); }
+
+  template <typename CharT>
+  MOZ_ALWAYS_INLINE BufferType<CharT>& chars() {
+    MOZ_ASSERT(isCharType<CharT>());
+    return cb.ref<BufferType<CharT>>();
+  }
+
+  template <typename CharT>
+  MOZ_ALWAYS_INLINE const BufferType<CharT>& chars() const {
+    MOZ_ASSERT(isCharType<CharT>());
+    return cb.ref<BufferType<CharT>>();
+  }
 
   MOZ_ALWAYS_INLINE TwoByteCharBuffer& twoByteChars() {
-    return cb.ref<TwoByteCharBuffer>();
+    return chars<char16_t>();
   }
 
   MOZ_ALWAYS_INLINE const TwoByteCharBuffer& twoByteChars() const {
-    return cb.ref<TwoByteCharBuffer>();
+    return chars<char16_t>();
+  }
+
+  MOZ_ALWAYS_INLINE Latin1CharBuffer& latin1Chars() {
+    return chars<Latin1Char>();
+  }
+
+  MOZ_ALWAYS_INLINE const Latin1CharBuffer& latin1Chars() const {
+    return chars<Latin1Char>();
   }
 
   MOZ_MUST_USE bool inflateChars();
 
+  template <typename CharT>
+  JSFlatString* finishStringInternal(JSContext* cx);
+
  public:
-  explicit StringBuffer(JSContext* cx)
-      : cx(cx)
-#ifdef DEBUG
-        ,
-        hasEnsuredTwoByteChars_(false)
-#endif
-        ,
-        reserved_(0) {
+  explicit StringBuffer(JSContext* cx) : cx(cx), reserved_(0) {
     cb.construct<Latin1CharBuffer>(cx);
   }
 
-  MOZ_ALWAYS_INLINE Latin1CharBuffer& latin1Chars() {
-    return cb.ref<Latin1CharBuffer>();
-  }
-
-  MOZ_ALWAYS_INLINE const Latin1CharBuffer& latin1Chars() const {
-    return cb.ref<Latin1CharBuffer>();
-  }
-
   void clear() {
     if (isLatin1()) {
       latin1Chars().clear();
     } else {
       twoByteChars().clear();
     }
   }
   MOZ_MUST_USE bool reserve(size_t len) {
@@ -105,35 +112,36 @@ class StringBuffer {
       reserved_ = len;
     }
     return isLatin1() ? latin1Chars().reserve(len)
                       : twoByteChars().reserve(len);
   }
   MOZ_MUST_USE bool resize(size_t len) {
     return isLatin1() ? latin1Chars().resize(len) : twoByteChars().resize(len);
   }
+  MOZ_MUST_USE bool growByUninitialized(size_t incr) {
+    return isLatin1() ? latin1Chars().growByUninitialized(incr)
+                      : twoByteChars().growByUninitialized(incr);
+  }
+  void shrinkTo(size_t newLength) {
+    return isLatin1() ? latin1Chars().shrinkTo(newLength)
+                      : twoByteChars().shrinkTo(newLength);
+  }
   bool empty() const {
     return isLatin1() ? latin1Chars().empty() : twoByteChars().empty();
   }
   size_t length() const {
     return isLatin1() ? latin1Chars().length() : twoByteChars().length();
   }
   char16_t getChar(size_t idx) const {
     return isLatin1() ? latin1Chars()[idx] : twoByteChars()[idx];
   }
 
   MOZ_MUST_USE bool ensureTwoByteChars() {
-    if (isLatin1() && !inflateChars()) {
-      return false;
-    }
-
-#ifdef DEBUG
-    hasEnsuredTwoByteChars_ = true;
-#endif
-    return true;
+    return isTwoByte() || inflateChars();
   }
 
   MOZ_MUST_USE bool append(const char16_t c) {
     if (isLatin1()) {
       if (c <= JSString::MAX_LATIN1_CHAR) {
         return latin1Chars().append(Latin1Char(c));
       }
       if (!inflateChars()) {
@@ -142,21 +150,16 @@ class StringBuffer {
     }
     return twoByteChars().append(c);
   }
   MOZ_MUST_USE bool append(Latin1Char c) {
     return isLatin1() ? latin1Chars().append(c) : twoByteChars().append(c);
   }
   MOZ_MUST_USE bool append(char c) { return append(Latin1Char(c)); }
 
-  TwoByteCharBuffer& rawTwoByteBuffer() {
-    MOZ_ASSERT(hasEnsuredTwoByteChars_);
-    return twoByteChars();
-  }
-
   inline MOZ_MUST_USE bool append(const char16_t* begin, const char16_t* end);
 
   MOZ_MUST_USE bool append(const char16_t* chars, size_t len) {
     return append(chars, chars + len);
   }
 
   MOZ_MUST_USE bool append(const Latin1Char* begin, const Latin1Char* end) {
     return isLatin1() ? latin1Chars().append(begin, end)
@@ -220,35 +223,51 @@ class StringBuffer {
 
   void infallibleAppendSubstring(JSLinearString* base, size_t off, size_t len);
 
   /*
    * Because inflation is fallible, these methods should only be used after
    * calling ensureTwoByteChars().
    */
   void infallibleAppend(const char16_t* chars, size_t len) {
-    MOZ_ASSERT(hasEnsuredTwoByteChars_);
     twoByteChars().infallibleAppend(chars, len);
   }
-  void infallibleAppend(char16_t c) {
-    MOZ_ASSERT(hasEnsuredTwoByteChars_);
-    twoByteChars().infallibleAppend(c);
-  }
+  void infallibleAppend(char16_t c) { twoByteChars().infallibleAppend(c); }
 
   bool isUnderlyingBufferLatin1() const { return isLatin1(); }
 
-  char16_t* rawTwoByteBegin() { return twoByteChars().begin(); }
-  char16_t* rawTwoByteEnd() { return twoByteChars().end(); }
-  const char16_t* rawTwoByteBegin() const { return twoByteChars().begin(); }
-  const char16_t* rawTwoByteEnd() const { return twoByteChars().end(); }
+  template <typename CharT>
+  CharT* begin() {
+    return chars<CharT>().begin();
+  }
+
+  template <typename CharT>
+  CharT* end() {
+    return chars<CharT>().end();
+  }
+
+  template <typename CharT>
+  const CharT* begin() const {
+    return chars<CharT>().begin();
+  }
 
-  Latin1Char* rawLatin1Begin() { return latin1Chars().begin(); }
-  Latin1Char* rawLatin1End() { return latin1Chars().end(); }
-  const Latin1Char* rawLatin1Begin() const { return latin1Chars().begin(); }
-  const Latin1Char* rawLatin1End() const { return latin1Chars().end(); }
+  template <typename CharT>
+  const CharT* end() const {
+    return chars<CharT>().end();
+  }
+
+  char16_t* rawTwoByteBegin() { return begin<char16_t>(); }
+  char16_t* rawTwoByteEnd() { return end<char16_t>(); }
+  const char16_t* rawTwoByteBegin() const { return begin<char16_t>(); }
+  const char16_t* rawTwoByteEnd() const { return end<char16_t>(); }
+
+  Latin1Char* rawLatin1Begin() { return begin<Latin1Char>(); }
+  Latin1Char* rawLatin1End() { return end<Latin1Char>(); }
+  const Latin1Char* rawLatin1Begin() const { return begin<Latin1Char>(); }
+  const Latin1Char* rawLatin1End() const { return end<Latin1Char>(); }
 
   /*
    * Creates a string from the characters in this buffer, then (regardless
    * whether string creation succeeded or failed) empties the buffer.
    */
   JSFlatString* finishString();
 
   /* Identical to finishString() except that an atom is created. */