Bug 1473149. Add an external string variant that keeps a DynamicAtom alive. r=njn,rwood
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 10 Jul 2018 11:21:42 -0700
changeset 425702 ed26ab52032ea0a2f1420236e50b6f37824e9c8b
parent 425689 9631474563f37d34ba1dffbf98d566804f3fbfc8
child 425703 09380f0f8aea557beb724192b355985bd1e28e2b
push id34262
push usercsabou@mozilla.com
push dateTue, 10 Jul 2018 21:51:50 +0000
treeherdermozilla-central@70f901964f97 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn, rwood
bugs1473149
milestone63.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 1473149. Add an external string variant that keeps a DynamicAtom alive. r=njn,rwood The change to call AsStatic() in SetKnownLiveAtom is drive-by performance cleanup.
dom/bindings/DOMString.h
js/xpconnect/src/XPCConvert.cpp
js/xpconnect/src/XPCString.cpp
js/xpconnect/src/xpcpublic.h
testing/talos/talos/tests/perf-reftest-singletons/README
testing/talos/talos/tests/perf-reftest-singletons/id-getter-1.html
testing/talos/talos/tests/perf-reftest-singletons/id-getter-2.html
testing/talos/talos/tests/perf-reftest-singletons/id-getter-3.html
testing/talos/talos/tests/perf-reftest-singletons/id-getter-4.html
testing/talos/talos/tests/perf-reftest-singletons/id-getter-5.html
testing/talos/talos/tests/perf-reftest-singletons/id-getter-6.html
testing/talos/talos/tests/perf-reftest-singletons/id-getter-7.html
testing/talos/talos/tests/perf-reftest-singletons/perf_reftest_singletons.manifest
xpcom/ds/nsAtom.h
--- a/dom/bindings/DOMString.h
+++ b/dom/bindings/DOMString.h
@@ -154,16 +154,36 @@ public:
 
   // Get the length of the literal.  Can only be called if HasLiteral().
   uint32_t LiteralLength() const
   {
     MOZ_ASSERT(HasLiteral(), "Don't call this if there is no literal");
     return mLength;
   }
 
+  bool HasAtom() const
+  {
+    MOZ_ASSERT(!mString || !mStringBuffer,
+               "Shouldn't have both present!");
+    MOZ_ASSERT(mState > State::Null,
+               "Caller should have checked IsNull() and IsEmpty() first");
+    return mState == State::UnownedAtom;
+  }
+
+  // Get the atom.  This can only be called if HasAtom() returned true.  If
+  // that's true, it will never return null.
+  nsDynamicAtom* Atom() const
+  {
+    MOZ_ASSERT(HasAtom(),
+               "Don't ask for the atom if we don't have it");
+    MOZ_ASSERT(mAtom,
+               "We better have an atom if we claim to");
+    return mAtom;
+  }
+
   // Initialize the DOMString to a (nsStringBuffer, length) pair.  The length
   // does NOT have to be the full length of the (null-terminated) string in the
   // nsStringBuffer.
   void SetKnownLiveStringBuffer(nsStringBuffer* aStringBuffer, uint32_t aLength)
   {
     MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
     if (aLength != 0) {
       SetStringBufferInternal(aStringBuffer, aLength);
@@ -209,27 +229,27 @@ public:
     eTreatNullAsEmpty,
     eNullNotExpected
   };
 
   void SetKnownLiveAtom(nsAtom* aAtom, NullHandling aNullHandling)
   {
     MOZ_ASSERT(mString.isNothing(), "We already have a string?");
     MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
-    MOZ_ASSERT(!mStringBuffer, "Setting stringbuffer twice?");
+    MOZ_ASSERT(!mAtom, "Setting atom twice?");
     MOZ_ASSERT(aAtom || aNullHandling != eNullNotExpected);
     if (aNullHandling == eNullNotExpected || aAtom) {
       if (aAtom->IsStatic()) {
-        // Static atoms are backed by literals.
-        SetLiteralInternal(aAtom->GetUTF16String(), aAtom->GetLength());
+        // Static atoms are backed by literals.  Explicitly call AsStatic() here
+        // to avoid the extra IsStatic() checks in nsAtom::GetUTF16String().
+        SetLiteralInternal(aAtom->AsStatic()->GetUTF16String(),
+                           aAtom->GetLength());
       } else {
-        // Dynamic atoms own their own chars, and never have 0 length because
-        // nsGkAtoms::_empty is a static atom.
-        MOZ_ASSERT(aAtom->GetLength() > 0);
-        AsAString().Assign(aAtom->AsDynamic()->String(), aAtom->GetLength());
+        mAtom = aAtom->AsDynamic();
+        mState = State::UnownedAtom;
       }
     } else if (aNullHandling == eTreatNullAsNull) {
       SetNull();
     }
   }
 
   void SetNull()
   {
@@ -272,16 +292,18 @@ public:
         // Safe to share the buffer.
         buf->ToString(len, aString);
       } else {
         // We need to copy, unfortunately.
         aString.Assign(chars, len);
       }
     } else if (HasLiteral()) {
       aString.AssignLiteral(Literal(), LiteralLength());
+    } else if (HasAtom()) {
+      mAtom->ToString(aString);
     } else {
       aString = AsAString();
     }
   }
 
 private:
   void SetStringBufferInternal(nsStringBuffer* aStringBuffer, uint32_t aLength)
   {
@@ -307,16 +329,19 @@ private:
     Empty, // An empty string.  Default state.
     Null,  // Null (not a string at all)
 
     // All states that involve actual string data should come after
     // Empty and Null.
 
     String, // An XPCOM string stored in mString.
     Literal, // A string literal (static lifetime).
+    UnownedAtom, // mAtom is valid and we are not holding a ref.
+    // If we ever add an OwnedAtom state, XPCStringConvert::DynamicAtomToJSVal
+    // will need to grow an out param for whether the atom was shared.
     OwnedStringBuffer, // mStringBuffer is valid and we have a ref to it.
     UnownedStringBuffer, // mStringBuffer is valid; we are not holding a ref.
     // The two string buffer values must come last.  This lets us avoid doing
     // two tests to figure out whether we have a stringbuffer.
   };
 
   // We need to be able to act like a string as needed
   Maybe<nsAutoString> mString;
@@ -324,16 +349,20 @@ private:
   union
   {
     // The nsStringBuffer in the OwnedStringBuffer/UnownedStringBuffer cases.
     nsStringBuffer* MOZ_UNSAFE_REF("The ways in which this can be safe are "
                                  "documented above and enforced through "
                                  "assertions") mStringBuffer;
     // The literal in the Literal case.
     const char16_t* mLiteral;
+    // The atom in the UnownedAtom case.
+    nsDynamicAtom* MOZ_UNSAFE_REF("The ways in which this can be safe are "
+                                  "documented above and enforced through "
+                                  "assertions") mAtom;
   };
 
   // Length in the stringbuffer and literal cases.
   uint32_t mLength;
 
   State mState;
 };
 
--- a/js/xpconnect/src/XPCConvert.cpp
+++ b/js/xpconnect/src/XPCConvert.cpp
@@ -579,16 +579,18 @@ XPCConvert::JSData2Native(void* d, Handl
                 ws->Assign(chars, length);
             }
         } else if (XPCStringConvert::IsLiteral(str)) {
             // The characters represent a literal char16_t string constant
             // compiled into libxul, such as the string "undefined" above.
             const char16_t* chars = JS_GetTwoByteExternalStringChars(str);
             ws->AssignLiteral(chars, length);
         } else {
+            // We don't bother checking for a dynamic-atom external string,
+            // because we'd just need to copy out of it anyway.
             if (!AssignJSString(cx, *ws, str))
                 return false;
         }
         return true;
     }
 
     case nsXPTType::T_CHAR_STR:
     case nsXPTType::T_PSTRING_SIZE_IS:
--- a/js/xpconnect/src/XPCString.cpp
+++ b/js/xpconnect/src/XPCString.cpp
@@ -42,16 +42,31 @@ XPCStringConvert::FinalizeDOMString(cons
 {
     nsStringBuffer* buf = nsStringBuffer::FromData(chars);
     buf->Release();
 }
 
 const JSStringFinalizer XPCStringConvert::sDOMStringFinalizer =
     { XPCStringConvert::FinalizeDOMString };
 
+// static
+void
+XPCStringConvert::FinalizeDynamicAtom(const JSStringFinalizer* fin,
+                                      char16_t* chars)
+{
+    nsDynamicAtom* atom = nsDynamicAtom::FromChars(chars);
+    // nsDynamicAtom::Release is always-inline and defined in a translation unit
+    // we can't get to here.  So we need to go through nsAtom::Release to call
+    // it.
+    static_cast<nsAtom*>(atom)->Release();
+}
+
+const JSStringFinalizer XPCStringConvert::sDynamicAtomFinalizer =
+    { XPCStringConvert::FinalizeDynamicAtom };
+
 // convert a readable to a JSString, copying string data
 // static
 bool
 XPCStringConvert::ReadableToJSVal(JSContext* cx,
                                   const nsAString& readable,
                                   nsStringBuffer** sharedBuffer,
                                   MutableHandleValue vp)
 {
--- a/js/xpconnect/src/xpcpublic.h
+++ b/js/xpconnect/src/xpcpublic.h
@@ -7,16 +7,17 @@
 #ifndef xpcpublic_h
 #define xpcpublic_h
 
 #include "jsapi.h"
 #include "js/HeapAPI.h"
 #include "js/GCAPI.h"
 #include "js/Proxy.h"
 
+#include "nsAtom.h"
 #include "nsISupports.h"
 #include "nsIURI.h"
 #include "nsIPrincipal.h"
 #include "nsIGlobalObject.h"
 #include "nsPIDOMWindow.h"
 #include "nsWrapperCache.h"
 #include "nsString.h"
 #include "nsTArray.h"
@@ -261,35 +262,61 @@ public:
         JSString* str = JS_NewMaybeExternalString(cx, literal, length,
                                                   &sLiteralFinalizer, &ignored);
         if (!str)
             return false;
         rval.setString(str);
         return true;
     }
 
+    static inline bool
+    DynamicAtomToJSVal(JSContext* cx, nsDynamicAtom* atom,
+                       JS::MutableHandleValue rval)
+    {
+        bool sharedAtom;
+        JSString* str = JS_NewMaybeExternalString(cx, atom->GetUTF16String(),
+                                                  atom->GetLength(),
+                                                  &sDynamicAtomFinalizer,
+                                                  &sharedAtom);
+        if (!str)
+            return false;
+        if (sharedAtom) {
+            // We only have non-owning atoms in DOMString for now.
+            // nsDynamicAtom::AddRef is always-inline and defined in a
+            // translation unit we can't get to here.  So we need to go through
+            // nsAtom::AddRef to call it.
+            static_cast<nsAtom*>(atom)->AddRef();
+        }
+        rval.setString(str);
+        return true;
+    }
+
     static MOZ_ALWAYS_INLINE bool IsLiteral(JSString* str)
     {
         return JS_IsExternalString(str) &&
                JS_GetExternalStringFinalizer(str) == &sLiteralFinalizer;
     }
 
     static MOZ_ALWAYS_INLINE bool IsDOMString(JSString* str)
     {
         return JS_IsExternalString(str) &&
                JS_GetExternalStringFinalizer(str) == &sDOMStringFinalizer;
     }
 
 private:
-    static const JSStringFinalizer sLiteralFinalizer, sDOMStringFinalizer;
+    static const JSStringFinalizer
+      sLiteralFinalizer, sDOMStringFinalizer, sDynamicAtomFinalizer;
 
     static void FinalizeLiteral(const JSStringFinalizer* fin, char16_t* chars);
 
     static void FinalizeDOMString(const JSStringFinalizer* fin, char16_t* chars);
 
+    static void FinalizeDynamicAtom(const JSStringFinalizer* fin,
+                                    char16_t* chars);
+
     XPCStringConvert() = delete;
 };
 
 class nsIAddonInterposition;
 
 namespace xpc {
 
 // If these functions return false, then an exception will be set on cx.
@@ -361,16 +388,20 @@ bool NonVoidStringToJsval(JSContext* cx,
         return true;
     }
 
     if (str.HasLiteral()) {
         return XPCStringConvert::StringLiteralToJSVal(cx, str.Literal(),
                                                       str.LiteralLength(), rval);
     }
 
+    if (str.HasAtom()) {
+        return XPCStringConvert::DynamicAtomToJSVal(cx, str.Atom(), rval);
+    }
+
     // It's an actual XPCOM string
     return NonVoidStringToJsval(cx, str.AsAString(), rval);
 }
 
 MOZ_ALWAYS_INLINE
 bool StringToJsval(JSContext* cx, mozilla::dom::DOMString& str,
                    JS::MutableHandleValue rval)
 {
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/README
@@ -0,0 +1,12 @@
+This directory is for adding short performance tests that will be tracked in
+Talos and receive regression alerts.
+
+To add a test:
+
+1)  Create a test HTML file which includes <script src="util.js"></script>
+2)  In that file, have an onload handler which does the following:
+   i) Any pre-test setup needed.
+  ii) A call to perf_start().
+ iii) The test steps.
+  iv) A call to perf_finish().
+3)  Add your test to the perf_reftest_singletons.manifest file.
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-1.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // A very short string.
+  el.id = "a";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-2.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // The longest string we can fit in a single-byte inline string (15 chars).
+  el.id = "aaaaaaaaaaaaaaa";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-3.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // The shortest string we can't fit in a single-byte inline string (16 chars).
+  el.id = "aaaaaaaaaaaaaaaa";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-4.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // The longest string we can fit in an autostring buffer (63 chars).
+  el.id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-5.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // The shortest string we can't fit in an autostring buffer (64 chars).
+  el.id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-6.html
@@ -0,0 +1,17 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // The longest string we can share via the external string cache after
+  // checking the chars (100 chars).
+  el.id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/talos/talos/tests/perf-reftest-singletons/id-getter-7.html
@@ -0,0 +1,17 @@
+<!doctype html>
+<script src="util.js"></script>
+<script>
+onload = function() {
+  var count = 20000000;
+  var el = document.createElement("span");
+  // The shortest string we can't share via the external string cache after
+  // checking the chars (101 chars).
+  el.id = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+  var getter = Object.getOwnPropertyDescriptor(Element.prototype, "id").get;
+  perf_start();
+  for (var i = 0; i < count; ++i) {
+    getter.call(el);
+  }
+  perf_finish();
+};
+</script>
--- a/testing/talos/talos/tests/perf-reftest-singletons/perf_reftest_singletons.manifest
+++ b/testing/talos/talos/tests/perf-reftest-singletons/perf_reftest_singletons.manifest
@@ -10,8 +10,16 @@
 % http://localhost/tests/perf-reftest-singletons/coalesce-1.html
 % http://localhost/tests/perf-reftest-singletons/coalesce-2.html
 % http://localhost/tests/perf-reftest-singletons/parent-basic-singleton.html
 % http://localhost/tests/perf-reftest-singletons/tiny-traversal-singleton.html
 % http://localhost/tests/perf-reftest-singletons/nth-index-1.html
 % http://localhost/tests/perf-reftest-singletons/nth-index-2.html
 
 % http://localhost/tests/perf-reftest-singletons/bidi-resolution-1.html
+
+% http://localhost/tests/perf-reftest-singletons/id-getter-1.html
+% http://localhost/tests/perf-reftest-singletons/id-getter-2.html
+% http://localhost/tests/perf-reftest-singletons/id-getter-3.html
+% http://localhost/tests/perf-reftest-singletons/id-getter-4.html
+% http://localhost/tests/perf-reftest-singletons/id-getter-5.html
+% http://localhost/tests/perf-reftest-singletons/id-getter-6.html
+% http://localhost/tests/perf-reftest-singletons/id-getter-7.html
--- a/xpcom/ds/nsAtom.h
+++ b/xpcom/ds/nsAtom.h
@@ -167,16 +167,21 @@ public:
   MozExternalRefCountType AddRef();
   MozExternalRefCountType Release();
 
   const char16_t* String() const
   {
     return reinterpret_cast<const char16_t*>(this + 1);
   }
 
+  static nsDynamicAtom* FromChars(char16_t* chars)
+  {
+    return reinterpret_cast<nsDynamicAtom*>(chars) - 1;
+  }
+
 private:
   friend class nsAtomTable;
   friend class nsAtomSubTable;
   // XXX: we'd like to remove nsHtml5AtomEntry. See bug 1392185.
   friend class nsHtml5AtomEntry;
 
   // These shouldn't be used directly, even by friend classes. The
   // Create()/Destroy() methods use them.