Changeset 24633 in webkit for trunk/JavaScriptCore/kjs/ustring.cpp

Timestamp:

Jul 25, 2007, 2:50:00 PM (18 years ago)

Author:

mjs

Message:

JavaScriptCore:

Reviewed by Darin.

• JavaScriptCore part of fix for <rdar://problem/5300291> Optimize GC to reclaim big, temporary objects (like XMLHttpRequest.responseXML) quickly

Also, as a side effect of optimizations included in this patch:

• 7% speedup on JavaScript iBench
• 4% speedup on "Celtic Kane" JS benchmark

The basic idea is explained in a big comment in collector.cpp. When unusually
large objecs are allocated, we push the next GC closer on the assumption that
most objects are short-lived.

I also did the following two optimizations in the course of tuning
this not to be a performance regression:

1) Change UString::Rep to hold a self-pointer as the baseString in
the unshared case, instead of a null pointer; this removes a
number of null checks in hot code because many places already
wanted to use the rep itself or the baseString as appropriate.

2) Avoid creating duplicate StringImpls when creating a
StringInstance (the object wrapper for a JS string) or calling
their methods. Since a temporary wrapper object is made every time
a string method is called, this resulted in two useless extra
StringImpls being allocated for no reason whenever a String method
was invoked on a string value. Now we bypass those.

• kjs/collector.cpp: (KJS::): (KJS::Collector::recordExtraCost): Basics of the extra cost mechanism. (KJS::Collector::allocate): ditto (KJS::Collector::collect): ditto
• kjs/collector.h: (KJS::Collector::reportExtraMemoryCost): ditto
• kjs/array_object.cpp: (ArrayInstance::ArrayInstance): record extra cost
• kjs/internal.cpp: (KJS::StringImp::toObject): don't create a whole new StringImpl just to be the internal value of a StringInstance! StringImpls are immutable so there's no point tot his.
• kjs/internal.h: (KJS::StringImp::StringImp): report extra cost
• kjs/string_object.cpp: (KJS::StringInstance::StringInstance): new version that takes a StringImp (KJS::StringProtoFunc::callAsFunction): don't create a whole new StringImpl just to convert self to string! we already have one in the internal value
• kjs/string_object.h: report extra cost
• kjs/ustring.cpp: All changes to handle baseString being self instead of null in the unshared case. (KJS::): (KJS::UString::Rep::create): (KJS::UString::Rep::destroy): (KJS::UString::usedCapacity): (KJS::UString::usedPreCapacity): (KJS::UString::expandCapacity): (KJS::UString::expandPreCapacity): (KJS::UString::UString): (KJS::UString::append): (KJS::UString::operator=): (KJS::UString::copyForWriting):
• kjs/ustring.h: (KJS::UString::Rep::baseIsSelf): new method, now that baseString is self instead of null in the unshared case we can't just null check. (KJS::UString::Rep::data): adjusted as mentioned above (KJS::UString::cost): new method to compute the cost for a UString, for use by StringImpl.

• kjs/value.cpp: (KJS::jsString): style fixups. (KJS::jsOwnedString): new method, use this for strings allocated from UStrings held by the parse tree. Tracking their cost as part of string cost is pointless, because garbage collecting them will not actually free the relevant string buffer.
• kjs/value.h: prototyped jsOwnedString.
• kjs/nodes.cpp: (StringNode::evaluate): use jsOwnedString as appropriate (RegExpNode::evaluate): ditto (PropertyNameNode::evaluate): ditto (ForInNode::execute): ditto

• JavaScriptCore.exp: Exported some new symbols.

WebCore:

Reviewed by Darin.

• fixed <rdar://problem/5300291> Optimize GC to reclaim big, temporary objects (like XMLHttpRequest.responseXML) quickly

With this plus related JavaScriptCore changes, a number of XMLHttpRequest situations that
result in huge data sets are addressed, including a single huge responseXML on an XMR done
repeatedly, or accessing responseText repeatedly during loading of a single large XHR.

In addition to the GC changes in JavaScriptCore, I changed responseText to be stored as a
KJS::UString instead of a WebCore::String so that the JavaScript responseText value can
share the buffer (indeed multiple intermediate responseTexts can share its buffer).

First of all, here's some manual test cases that will each blow out the process VM without this fix,
but will settle into decent steady state with.

• manual-tests/memory: Added.
• manual-tests/memory/MessageUidsAlreadyDownloaded2: Added.
• manual-tests/memory/string-growth.html: Added.
• manual-tests/memory/xhr-multiple-requests-responseText.html: Added.
• manual-tests/memory/xhr-multiple-requests-responseXML.html: Added.
• manual-tests/memory/xhr-multiple-requests.html: Added.
• manual-tests/memory/xhr-repeated-string-access.xml: Added.

And here's the actual code changes:

• WebCore.xcodeproj/project.pbxproj:
• bindings/js/JSDocumentCustom.cpp: (WebCore::toJS): Record extra cost if the document is frameless (counting the nodes doesn't make a measurable performance difference here in any case I could find)
• bindings/js/JSXMLHttpRequest.cpp: (KJS::JSXMLHttpRequest::getValueProperty): Adjust for the fact that ressponseText is now stored as a UString.
• bindings/js/kjs_binding.cpp: (KJS::jsOwnedStringOrNull): New helper.
• bindings/js/kjs_binding.h:
• xml/XMLHttpRequest.cpp: (WebCore::XMLHttpRequest::getResponseText): It's a UString! (WebCore::XMLHttpRequest::getResponseXML): handle the fact that m_responseText is a UString. (WebCore::XMLHttpRequest::XMLHttpRequest): ditto. (WebCore::XMLHttpRequest::abort): call dropProtection (WebCore::XMLHttpRequest::didFinishLoading): call dropProtection (WebCore::XMLHttpRequest::dropProtection): after removing our GC protection, report extra cost of this XHR's responseText buffer.
• xml/XMLHttpRequest.h:

File:

• trunk/JavaScriptCore/kjs/ustring.cpp (modified) (19 diffs)

trunk/JavaScriptCore/kjs/ustring.cpp

@@ -27 +27 @@
 
 #include "JSLock.h"
+#include "collector.h"
 #include "dtoa.h"
 #include "function.h"
@@ -54 +55 @@
 extern const double Inf;
 
+// we'd rather not do shared substring append for small strings, since
+// this runs too much risk of a tiny initial string holding down a
+// huge buffer. This is also tuned to match the extra cost size, so we
+// don't ever share a buffer that wouldn't be over the extra cost
+// threshold already.
+// FIXME: this should be size_t but that would cause warnings until we
+// fix UString sizes to be size_t instad of int
+static const int minShareSize = Collector::minExtraCostSize / sizeof(UChar);
+
 COMPILE_ASSERT(sizeof(UChar) == 2, uchar_is_2_bytes)
 
@@ -141 +151 @@
 // Hack here to avoid a global with a constructor; point to an unsigned short instead of a UChar.
 static unsigned short almostUChar;
-UString::Rep UString::Rep::null = { 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0 };
-UString::Rep UString::Rep::empty = { 0, 0, 1, 0, 0, 0, reinterpret_cast<UChar*>(&almostUChar), 0, 0, 0, 0 };
+UString::Rep UString::Rep::null = { 0, 0, 1, 0, 0, &UString::Rep::null, 0, 0, 0, 0, 0 };
+UString::Rep UString::Rep::empty = { 0, 0, 1, 0, 0, &UString::Rep::empty, reinterpret_cast<UChar*>(&almostUChar), 0, 0, 0, 0 };
 const int normalStatBufferSize = 4096;
 static char *statBuffer = 0;
@@ -183 +193 @@
   ASSERT(JSLock::lockCount() > 0);
 
-  Rep *r = new Rep;
+  Rep* r = new Rep;
   r->offset = 0;
   r->len = l;
@@ -189 +199 @@
   r->_hash = 0;
   r->isIdentifier = 0;
-  r->baseString = 0;
+  r->baseString = r;
   r->buf = d;
   r->usedCapacity = l;
@@ -207 +217 @@
   int baseOffset = base->offset;
 
-  if (base->baseString) {
-    base = base->baseString;
-  }
+  base = base->baseString;
 
   assert(-(offset + baseOffset) <= base->usedPreCapacity);
@@ -237 +245 @@
   if (isIdentifier)
     Identifier::remove(this);
-  if (baseString) {
+  if (baseString != this) {
     baseString->deref();
   } else {
@@ -348 +356 @@
 inline int UString::usedCapacity() const
 {
-  return m_rep->baseString ? m_rep->baseString->usedCapacity : m_rep->usedCapacity;
+  return m_rep->baseString->usedCapacity;
 }
 
 inline int UString::usedPreCapacity() const
 {
-  return m_rep->baseString ? m_rep->baseString->usedPreCapacity : m_rep->usedPreCapacity;
+  return m_rep->baseString->usedPreCapacity;
 }
 
 void UString::expandCapacity(int requiredLength)
 {
-  Rep *r = m_rep->baseString ? m_rep->baseString : rep();
+  Rep* r = m_rep->baseString;
 
   if (requiredLength > r->capacity) {
@@ -372 +380 @@
 void UString::expandPreCapacity(int requiredPreCap)
 {
-  Rep *r = m_rep->baseString ? m_rep->baseString : rep();
+  Rep* r = m_rep->baseString;
 
   if (requiredPreCap > r->preCapacity) {
@@ -441 +449 @@
     // b is empty
     m_rep = a.m_rep;
-  } else if (aOffset + aSize == a.usedCapacity() && 4 * aSize >= bSize &&
+  } else if (aOffset + aSize == a.usedCapacity() && aSize >= minShareSize && 4 * aSize >= bSize &&
              (-bOffset != b.usedPreCapacity() || aSize >= bSize)) {
     // - a reaches the end of its buffer so it qualifies for shared append
@@ -454 +462 @@
     } else
         m_rep = &Rep::null;
-  } else if (-bOffset == b.usedPreCapacity() && 4 * bSize >= aSize) {
+  } else if (-bOffset == b.usedPreCapacity() && bSize >= minShareSize && 4 * bSize >= aSize) {
     // - b reaches the beginning of its buffer so it qualifies for shared prepend
     // - also, it's at least a quarter the length of a - prepending to a much shorter
@@ -680 +688 @@
   } else if (tSize == 0) {
     // t is empty
-  } else if (!m_rep->baseString && m_rep->rc == 1) {
+  } else if (m_rep->baseIsSelf() && m_rep->rc == 1) {
     // this is direct and has refcount of 1 (so we can just alter it directly)
     expandCapacity(thisOffset + length);
@@ -686 +694 @@
     m_rep->len = length;
     m_rep->_hash = 0;
-  } else if (thisOffset + thisSize == usedCapacity()) {
-    // this reaches the end of the buffer - extend it
+  } else if (thisOffset + thisSize == usedCapacity() && thisSize >= minShareSize) {
+    // this reaches the end of the buffer - extend it if it's long enough to append to
     expandCapacity(thisOffset + length);
     memcpy(const_cast<UChar *>(data() + thisSize), t.data(), tSize * sizeof(UChar));
@@ -717 +725 @@
   } else if (tSize == 0) {
     // t is empty, we'll just return *this below.
-  } else if (!m_rep->baseString && m_rep->rc == 1) {
+  } else if (m_rep->baseIsSelf() && m_rep->rc == 1) {
     // this is direct and has refcount of 1 (so we can just alter it directly)
     expandCapacity(thisOffset + length);
@@ -725 +733 @@
     m_rep->len = length;
     m_rep->_hash = 0;
-  } else if (thisOffset + thisSize == usedCapacity()) {
+  } else if (thisOffset + thisSize == usedCapacity() && thisSize >= minShareSize) {
     // this string reaches the end of the buffer - extend it
     expandCapacity(thisOffset + length);
@@ -759 +767 @@
     m_rep = Rep::create(d, 1);
     m_rep->capacity = newCapacity;
-  } else if (!m_rep->baseString && m_rep->rc == 1) {
+  } else if (m_rep->baseIsSelf() && m_rep->rc == 1) {
     // this is direct and has refcount of 1 (so we can just alter it directly)
     expandCapacity(thisOffset + length + 1);
@@ -766 +774 @@
     m_rep->len = length + 1;
     m_rep->_hash = 0;
-  } else if (thisOffset + length == usedCapacity()) {
+  } else if (thisOffset + length == usedCapacity() && length >= minShareSize) {
     // this reaches the end of the string - extend it and share
     expandCapacity(thisOffset + length + 1);
@@ -831 +839 @@
   int l = c ? static_cast<int>(strlen(c)) : 0;
   UChar *d;
-  if (m_rep->rc == 1 && l <= m_rep->capacity && !m_rep->baseString && m_rep->offset == 0 && m_rep->preCapacity == 0) {
+  if (m_rep->rc == 1 && l <= m_rep->capacity && m_rep->baseIsSelf() && m_rep->offset == 0 && m_rep->preCapacity == 0) {
     d = m_rep->buf;
     m_rep->_hash = 0;
@@ -1129 +1137 @@
 void UString::copyForWriting()
 {
-  if (m_rep->rc > 1 || m_rep->baseString) {
+  if (m_rep->rc > 1 || !m_rep->baseIsSelf()) {
     int l = size();
     UChar *n = static_cast<UChar *>(fastMalloc(sizeof(UChar) * l));