Unreviewed, rolling out r132913.
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Nov 2012 18:33:58 +0000 (18:33 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Nov 2012 18:33:58 +0000 (18:33 +0000)
http://trac.webkit.org/changeset/132913
https://bugs.webkit.org/show_bug.cgi?id=91850

Caused performance regressions.
See https://bugs.webkit.org/show_bug.cgi?id=100872 for details.

* bindings/v8/V8PerIsolateData.cpp:
(WebCore::V8PerIsolateData::visitExternalStrings):
* bindings/v8/V8StringResource.cpp:
(StringTraits):
(WebCore::v8StringToWebCoreString):
* bindings/v8/V8ValueCache.cpp:
(WebCore::makeExternalString):
(WebCore::WebCoreStringResource::visitStrings):
* bindings/v8/V8ValueCache.h:
(WebCore::WebCoreStringResource::WebCoreStringResource):
(WebCore::WebCoreStringResource::~WebCoreStringResource):
(WebCore::WebCoreStringResource::data):
(WebCoreStringResource):
(WebCore::WebCoreStringResource::length):
(WebCore::WebCoreStringResource::atomicString):
(WebCore::WebCoreStringResource::toStringResource):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@133319 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/V8PerIsolateData.cpp
Source/WebCore/bindings/v8/V8StringResource.cpp
Source/WebCore/bindings/v8/V8ValueCache.cpp
Source/WebCore/bindings/v8/V8ValueCache.h

index 19e34ce..2ee688a 100644 (file)
@@ -1,3 +1,29 @@
+2012-11-02  Ojan Vafai  <ojan@chromium.org>
+
+        Unreviewed, rolling out r132913.
+        http://trac.webkit.org/changeset/132913
+        https://bugs.webkit.org/show_bug.cgi?id=91850
+
+        Caused performance regressions.
+        See https://bugs.webkit.org/show_bug.cgi?id=100872 for details.
+
+        * bindings/v8/V8PerIsolateData.cpp:
+        (WebCore::V8PerIsolateData::visitExternalStrings):
+        * bindings/v8/V8StringResource.cpp:
+        (StringTraits):
+        (WebCore::v8StringToWebCoreString):
+        * bindings/v8/V8ValueCache.cpp:
+        (WebCore::makeExternalString):
+        (WebCore::WebCoreStringResource::visitStrings):
+        * bindings/v8/V8ValueCache.h:
+        (WebCore::WebCoreStringResource::WebCoreStringResource):
+        (WebCore::WebCoreStringResource::~WebCoreStringResource):
+        (WebCore::WebCoreStringResource::data):
+        (WebCoreStringResource):
+        (WebCore::WebCoreStringResource::length):
+        (WebCore::WebCoreStringResource::atomicString):
+        (WebCore::WebCoreStringResource::toStringResource):
+
 2012-11-02  Martin Robinson  <mrobinson@igalia.com>
 
         [GTK] Remove dependency on SoupPasswordManager
index 4d8e0d1..373066c 100644 (file)
@@ -121,7 +121,7 @@ void V8PerIsolateData::visitExternalStrings(ExternalStringVisitor* visitor)
         virtual ~VisitorImpl() { }
         virtual void VisitExternalString(v8::Handle<v8::String> string)
         {
-            WebCoreStringResourceBase* resource = WebCoreStringResourceBase::toWebCoreStringResourceBase(string);
+            WebCoreStringResource* resource = static_cast<WebCoreStringResource*>(string->GetExternalStringResource());
             if (resource)
                 resource->visitStrings(m_visitor);
         }
index 430825b..598553f 100644 (file)
 
 namespace WebCore {
 
-template<class StringClass> struct StringTraits {
-    static const StringClass& fromStringResource(WebCoreStringResourceBase*);
-    static bool is16BitAtomicString(StringClass&);
-    template<bool ascii>
+template <class StringClass> struct StringTraits {
+    static const StringClass& fromStringResource(WebCoreStringResource*);
     static StringClass fromV8String(v8::Handle<v8::String>, int);
 };
 
 template<>
 struct StringTraits<String> {
-    static const String& fromStringResource(WebCoreStringResourceBase* resource)
+    static const String& fromStringResource(WebCoreStringResource* resource)
     {
         return resource->webcoreString();
     }
-    static bool is16BitAtomicString(String& string)
+
+    static String fromV8String(v8::Handle<v8::String> v8String, int length)
     {
-        return false;
+        ASSERT(v8String->Length() == length);
+        // NOTE: as of now, String(const UChar*, int) performs String::createUninitialized
+        // anyway, so no need to optimize like we do for AtomicString below.
+        UChar* buffer;
+        String result = String::createUninitialized(length, buffer);
+        v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
+        return result;
     }
-    template<bool ascii>
-    static String fromV8String(v8::Handle<v8::String>, int);
 };
 
 template<>
 struct StringTraits<AtomicString> {
-    static const AtomicString& fromStringResource(WebCoreStringResourceBase* resource)
+    static const AtomicString& fromStringResource(WebCoreStringResource* resource)
     {
         return resource->atomicString();
     }
-    static bool is16BitAtomicString(AtomicString& string)
+
+    static AtomicString fromV8String(v8::Handle<v8::String> v8String, int length)
     {
-        return !string.string().is8Bit();
+        ASSERT(v8String->Length() == length);
+        static const int inlineBufferSize = 16;
+        if (length <= inlineBufferSize) {
+            UChar inlineBuffer[inlineBufferSize];
+            v8String->Write(reinterpret_cast<uint16_t*>(inlineBuffer), 0, length);
+            return AtomicString(inlineBuffer, length);
+        }
+        UChar* buffer;
+        String string = String::createUninitialized(length, buffer);
+        v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
+        return AtomicString(string);
     }
-    template<bool ascii>
-    static AtomicString fromV8String(v8::Handle<v8::String>, int);
 };
 
-template<>
-String StringTraits<String>::fromV8String<false>(v8::Handle<v8::String> v8String, int length)
-{
-    ASSERT(v8String->Length() == length);
-    UChar* buffer;
-    String result = String::createUninitialized(length, buffer);
-    v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
-    return result;
-}
-
-template<>
-AtomicString StringTraits<AtomicString>::fromV8String<false>(v8::Handle<v8::String> v8String, int length)
-{
-    ASSERT(v8String->Length() == length);
-    static const int inlineBufferSize = 16;
-    if (length <= inlineBufferSize) {
-        UChar inlineBuffer[inlineBufferSize];
-        v8String->Write(reinterpret_cast<uint16_t*>(inlineBuffer), 0, length);
-        return AtomicString(inlineBuffer, length);
-    }
-    UChar* buffer;
-    String string = String::createUninitialized(length, buffer);
-    v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
-    return AtomicString(string);
-}
-
-template<>
-String StringTraits<String>::fromV8String<true>(v8::Handle<v8::String> v8String, int length)
-{
-    ASSERT(v8String->Length() == length);
-    LChar* buffer;
-    String result = String::createUninitialized(length, buffer);
-    v8String->WriteAscii(reinterpret_cast<char*>(buffer), 0, length, v8::String::PRESERVE_ASCII_NULL);
-    return result;
-}
-
-template<>
-inline AtomicString StringTraits<AtomicString>::fromV8String<true>(v8::Handle<v8::String> v8String, int length)
-{
-    // FIXME: There is no inline fast path for 8 bit atomic strings.
-    String result = StringTraits<String>::fromV8String<true>(v8String, length);
-    return AtomicString(result);
-}
-
-template<typename StringType>
+template <typename StringType>
 StringType v8StringToWebCoreString(v8::Handle<v8::String> v8String, ExternalMode external)
 {
-    {
-        // A lot of WebCoreStringResourceBase::toWebCoreStringResourceBase is copied here by hand for performance reasons.
-        // This portion of this function is very hot in certain Dromeao benchmarks.
-        v8::String::Encoding encoding;
-        v8::String::ExternalStringResourceBase* resource = v8String->GetExternalStringResourceBase(&encoding);
-        if (LIKELY(!!resource)) {
-            WebCoreStringResourceBase* base;
-            if (encoding == v8::String::ASCII_ENCODING)
-                base = static_cast<WebCoreStringResource8*>(resource);
-            else
-                base = static_cast<WebCoreStringResource16*>(resource);
-            return StringTraits<StringType>::fromStringResource(base);
-        }
-    }
+    WebCoreStringResource* stringResource = WebCoreStringResource::toStringResource(v8String);
+    if (stringResource)
+        return StringTraits<StringType>::fromStringResource(stringResource);
 
     int length = v8String->Length();
-    if (UNLIKELY(!length))
+    if (!length)
         return String("");
 
-    bool nonAscii = v8String->MayContainNonAscii();
-    StringType result(nonAscii ? StringTraits<StringType>::template fromV8String<false>(v8String, length) : StringTraits<StringType>::template fromV8String<true>(v8String, length));
+    StringType result(StringTraits<StringType>::fromV8String(v8String, length));
 
-    if (external != Externalize || !v8String->CanMakeExternal())
-        return result;
-
-    if (!nonAscii && !StringTraits<StringType>::is16BitAtomicString(result)) {
-        WebCoreStringResource8* stringResource = new WebCoreStringResource8(result);
-        if (UNLIKELY(!v8String->MakeExternal(stringResource)))
-            delete stringResource;
-    } else {
-        WebCoreStringResource16* stringResource = new WebCoreStringResource16(result);
-        if (UNLIKELY(!v8String->MakeExternal(stringResource)))
+    if (external == Externalize && v8String->CanMakeExternal()) {
+        stringResource = new WebCoreStringResource(result);
+        if (!v8String->MakeExternal(stringResource)) {
+            // In case of a failure delete the external resource as it was not used.
             delete stringResource;
+        }
     }
     return result;
 }
index d18011d..39cffc5 100644 (file)
@@ -33,15 +33,7 @@ namespace WebCore {
 
 static v8::Local<v8::String> makeExternalString(const String& string)
 {
-    if (string.is8Bit() && string.containsOnlyASCII()) {
-        WebCoreStringResource8* stringResource = new WebCoreStringResource8(string);
-        v8::Local<v8::String> newString = v8::String::NewExternal(stringResource);
-        if (newString.IsEmpty())
-            delete stringResource;
-        return newString;
-    }
-
-    WebCoreStringResource16* stringResource = new WebCoreStringResource16(string);
+    WebCoreStringResource* stringResource = new WebCoreStringResource(string);
     v8::Local<v8::String> newString = v8::String::NewExternal(stringResource);
     if (newString.IsEmpty())
         delete stringResource;
@@ -100,18 +92,7 @@ v8::Local<v8::String> StringCache::v8ExternalStringSlow(StringImpl* stringImpl,
     return newString;
 }
 
-WebCoreStringResourceBase* WebCoreStringResourceBase::toWebCoreStringResourceBase(v8::Handle<v8::String> string)
-{
-    v8::String::Encoding encoding;
-    v8::String::ExternalStringResourceBase* resource = string->GetExternalStringResourceBase(&encoding);
-    if (!resource)
-        return 0;
-    if (encoding == v8::String::ASCII_ENCODING)
-        return static_cast<WebCoreStringResource8*>(resource);
-    return static_cast<WebCoreStringResource16*>(resource);
-}
-
-void WebCoreStringResourceBase::visitStrings(ExternalStringVisitor* visitor)
+void WebCoreStringResource::visitStrings(ExternalStringVisitor* visitor)
 {
     visitor->visitJSExternalString(m_plainString.impl());
     if (m_plainString.impl() != m_atomicString.impl() && !m_atomicString.isNull())
index c28d542..671f68d 100644 (file)
@@ -72,21 +72,19 @@ const int numberOfCachedSmallIntegers = 64;
 
 // WebCoreStringResource is a helper class for v8ExternalString. It is used
 // to manage the life-cycle of the underlying buffer of the external string.
-class WebCoreStringResourceBase {
+class WebCoreStringResource : public v8::String::ExternalStringResource {
 public:
-    static WebCoreStringResourceBase* toWebCoreStringResourceBase(v8::Handle<v8::String>);
-
-    explicit WebCoreStringResourceBase(const String& string)
+    explicit WebCoreStringResource(const String& string)
         : m_plainString(string)
     {
 #ifndef NDEBUG
         m_threadId = WTF::currentThread();
 #endif
         ASSERT(!string.isNull());
-        v8::V8::AdjustAmountOfExternalAllocatedMemory(memoryConsumption(string));
+        v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * string.length());
     }
 
-    explicit WebCoreStringResourceBase(const AtomicString& string)
+    explicit WebCoreStringResource(const AtomicString& string)
         : m_plainString(string.string())
         , m_atomicString(string)
     {
@@ -94,20 +92,27 @@ public:
         m_threadId = WTF::currentThread();
 #endif
         ASSERT(!string.isNull());
-        v8::V8::AdjustAmountOfExternalAllocatedMemory(memoryConsumption(string));
+        v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * string.length());
     }
 
-    virtual ~WebCoreStringResourceBase()
+    virtual ~WebCoreStringResource()
     {
 #ifndef NDEBUG
         ASSERT(m_threadId == WTF::currentThread());
 #endif
-        int reducedExternalMemory = -memoryConsumption(m_plainString);
+        int reducedExternalMemory = -2 * m_plainString.length();
         if (m_plainString.impl() != m_atomicString.impl() && !m_atomicString.isNull())
-            reducedExternalMemory -= memoryConsumption(m_atomicString.string());
+            reducedExternalMemory *= 2;
         v8::V8::AdjustAmountOfExternalAllocatedMemory(reducedExternalMemory);
     }
 
+    virtual const uint16_t* data() const
+    {
+        return reinterpret_cast<const uint16_t*>(m_plainString.impl()->characters());
+    }
+
+    virtual size_t length() const { return m_plainString.impl()->length(); }
+
     const String& webcoreString() { return m_plainString; }
 
     const AtomicString& atomicString()
@@ -119,14 +124,19 @@ public:
             m_atomicString = AtomicString(m_plainString);
             ASSERT(!m_atomicString.isNull());
             if (m_plainString.impl() != m_atomicString.impl())
-                v8::V8::AdjustAmountOfExternalAllocatedMemory(memoryConsumption(m_atomicString.string()));
+                v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * m_atomicString.length());
         }
         return m_atomicString;
     }
 
     void visitStrings(ExternalStringVisitor*);
 
-protected:
+    static WebCoreStringResource* toStringResource(v8::Handle<v8::String> v8String)
+    {
+        return static_cast<WebCoreStringResource*>(v8String->GetExternalStringResource());
+    }
+
+private:
     // A shallow copy of the string. Keeps the string buffer alive until the V8 engine garbage collects it.
     String m_plainString;
     // If this string is atomic or has been made atomic earlier the
@@ -136,40 +146,11 @@ protected:
     // into that string.
     AtomicString m_atomicString;
 
-private:
-    static int memoryConsumption(const String& string)
-    {
-        return string.length() * (string.is8Bit() ? sizeof(LChar) : sizeof(UChar));
-    }
 #ifndef NDEBUG
     WTF::ThreadIdentifier m_threadId;
 #endif
 };
 
-class WebCoreStringResource16 : public WebCoreStringResourceBase, public v8::String::ExternalStringResource {
-public:
-    explicit WebCoreStringResource16(const String& string) : WebCoreStringResourceBase(string) { }
-    explicit WebCoreStringResource16(const AtomicString& string) : WebCoreStringResourceBase(string) { }
-
-    virtual size_t length() const OVERRIDE { return m_plainString.impl()->length(); }
-    virtual const uint16_t* data() const OVERRIDE
-    {
-        return reinterpret_cast<const uint16_t*>(m_plainString.impl()->characters());
-    }
-};
-
-class WebCoreStringResource8 : public WebCoreStringResourceBase, public v8::String::ExternalAsciiStringResource {
-public:
-    explicit WebCoreStringResource8(const String& string) : WebCoreStringResourceBase(string) { }
-    explicit WebCoreStringResource8(const AtomicString& string) : WebCoreStringResourceBase(string) { }
-
-    virtual size_t length() const OVERRIDE { return m_plainString.impl()->length(); }
-    virtual const char* data() const OVERRIDE
-    {
-        return reinterpret_cast<const char*>(m_plainString.impl()->characters8());
-    }
-};
-
 class IntegerCache {
 public:
      IntegerCache() : m_initialized(false) { };