Experimental CompressibleString UMA

third_party/WebKit/Source/bindings/core/v8/V8ValueCache.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 36 matching lines @@
 {
     V8PerIsolateData::from(data.GetIsolate())->stringCache()->InvalidateLastString();
     data.GetParameter()->deref();
 }
 
 void StringCacheMapTraits::OnWeakCallback(const v8::WeakCallbackInfo<WeakCallbackDataType>& data)
 {
     V8PerIsolateData::from(data.GetIsolate())->stringCache()->InvalidateLastString();
 }
 
+
+CompressibleStringCacheMapTraits::MapType* CompressibleStringCacheMapTraits::MapFromWeakCallbackInfo(
+    const v8::WeakCallbackInfo<WeakCallbackDataType>& data)
+{
+    return &(V8PerIsolateData::from(data.GetIsolate())->stringCache()->m_compressibleStringCache);
+}
+
+void CompressibleStringCacheMapTraits::Dispose(
+    v8::Isolate* isolate, v8::Global<v8::String> value, CompressibleStringImpl* key)
+{
+    key->deref();
+}
+
+void CompressibleStringCacheMapTraits::DisposeWeak(const v8::WeakCallbackInfo<WeakCallbackDataType>& data)
+{
+    data.GetParameter()->deref();
+}
+
+void CompressibleStringCacheMapTraits::OnWeakCallback(const v8::WeakCallbackInfo<WeakCallbackDataType>& data)
+{
+}
+
+
 void StringCache::dispose()
 {
     // The MapType::Dispose callback calls StringCache::InvalidateLastString,
     // which will only work while the destructor has not yet finished. Thus,
     // we need to clear the map before the destructor has completed.
     m_stringCache.Clear();
 }
 
 static v8::Local<v8::String> makeExternalString(v8::Isolate* isolate, const String& string)
 {
     if (string.is8Bit()) {
         WebCoreStringResource8* stringResource = new WebCoreStringResource8(string);
         v8::Local<v8::String> newString;
         if (!v8::String::NewExternalOneByte(isolate, stringResource).ToLocal(&newString)) {
             delete stringResource;
             return v8::String::Empty(isolate);
         }
         return newString;
     }
 
     WebCoreStringResource16* stringResource = new WebCoreStringResource16(string);
     v8::Local<v8::String> newString;
     if (!v8::String::NewExternalTwoByte(isolate, stringResource).ToLocal(&newString)) {
         delete stringResource;
         return v8::String::Empty(isolate);
     }
     return newString;
 }
 
+static v8::Local<v8::String> makeExternalString(v8::Isolate* isolate, const CompressibleString& string)
+{
+    if (string.is8Bit()) {
+        WebCoreCompressibleStringResource8* stringResource = new WebCoreCompressibleStringResource8(string);
+        v8::Local<v8::String> newString;
+        if (!v8::String::NewExternalOneByte(isolate, stringResource).ToLocal(&newString)) {
+            delete stringResource;
+            return v8::String::Empty(isolate);
+        }
+        return newString;
+    }
+
+    WebCoreCompressibleStringResource16* stringResource = new WebCoreCompressibleStringResource16(string);
+    v8::Local<v8::String> newString;
+    if (!v8::String::NewExternalTwoByte(isolate, stringResource).ToLocal(&newString)) {
+        delete stringResource;
+        return v8::String::Empty(isolate);
+    }
+    return newString;
+}
+
 v8::Local<v8::String> StringCache::v8ExternalStringSlow(v8::Isolate* isolate, StringImpl* stringImpl)
 {
     if (!stringImpl->length())
         return v8::String::Empty(isolate);
 
     StringCacheMapTraits::MapType::PersistentValueReference cachedV8String = m_stringCache.GetReference(stringImpl);
     if (!cachedV8String.IsEmpty()) {
         m_lastStringImpl = stringImpl;
         m_lastV8String = cachedV8String;
         return m_lastV8String.NewLocal(isolate);
     }
 
     return createStringAndInsertIntoCache(isolate, stringImpl);
 }
 
+v8::Local<v8::String> StringCache::v8ExternalStringSlow(v8::Isolate* isolate, const CompressibleString& string)
+{
+    if (!string.length())
+        return v8::String::Empty(isolate);
+
+    CompressibleStringCacheMapTraits::MapType::PersistentValueReference cachedV8String = m_compressibleStringCache.GetReference(string.impl().get());
+    if (!cachedV8String.IsEmpty())
+        return cachedV8String.NewLocal(isolate);
+
+    return createStringAndInsertIntoCache(isolate, string);
+}
+
 void StringCache::setReturnValueFromStringSlow(v8::ReturnValue<v8::Value> returnValue, StringImpl* stringImpl)
 {
     if (!stringImpl->length()) {
         returnValue.SetEmptyString();
         return;
     }
 
     StringCacheMapTraits::MapType::PersistentValueReference cachedV8String = m_stringCache.GetReference(stringImpl);
     if (!cachedV8String.IsEmpty()) {
         m_lastStringImpl = stringImpl;
@@ skipping 12 matching lines @@
 
     v8::Local<v8::String> newString = makeExternalString(isolate, String(stringImpl));
     ASSERT(!newString.IsEmpty());
     ASSERT(newString->Length());
 
     v8::UniquePersistent<v8::String> wrapper(isolate, newString);
 
     stringImpl->ref();
     wrapper.MarkIndependent();
     m_stringCache.Set(stringImpl, wrapper.Pass(), &m_lastV8String);
     m_lastStringImpl = stringImpl;

[haraken, 2016/01/18 17:23:37]

Previously I mentioned that it would be unsafe to store a CompressibleString
onto m_lastStringImpl because the backing StringImpl can become stale if the
CompressibleString compresses the StringImpl. So we have introduced
m_compressibleStringCache to maintain CompressibleStrings separately from normal
Strings.

However, I begin to think that it is just safe to store a CompressibleString
onto m_lastStringImpl. Even if the CompressibleString compresses the backing
StringImpl while the m_lastStringImpl is pointing to the StringImpl, the worst
thing that can happen is just that the destruction of the StringImpl will be
delayed until the m_lastStringImpl points to another StringImpl.

So, my question is:

If we're allowed to store the CompressibleString onto m_lastStringImpl (this is
safe), can we remove a bunch of code duplication from this CL? For example, can
we remove m_compressibleStringCache and its Traits?

(I'm sorry for asking this kind of design question now -- I should have noticed
this earlier...)

[hajimehoshi, 2016/01/19 06:03:38]

In the worst case, the memory usage would be increased because both the
compressed string and the uncompressed string would exist at the same time. If
compressing can reduce 90% size of a JavaScript source, the memory usage will be
110% (100% + 10%), right? I think m_lastStringImpl is update so often that we
can allow this temporal memory usage.
Ok, let me try the refactoring you suggested.
No problem!

[hajimehoshi, 2016/01/19 06:21:39]

Oops, I found that I did NOT add m_lastCompressibleStringImpl and the last
CompressibleString object is not cached. I have no idea how often m_stringCache
is cleared, in other words, how long the temporal memory usage increase in 110%
as I said, 110% for big JavaScirpt source strings) continues.

 
     return newString;
 }
 
+v8::Local<v8::String> StringCache::createStringAndInsertIntoCache(v8::Isolate* isolate, const CompressibleString& string)
+{
+    CompressibleStringImpl* stringImpl = string.impl().get();
+
+    ASSERT(!m_compressibleStringCache.Contains(stringImpl));
+    ASSERT(stringImpl->originalLength());
+
+    v8::Local<v8::String> newString = makeExternalString(isolate, string);
+    ASSERT(!newString.IsEmpty());
+    ASSERT(newString->Length());
+
+    v8::UniquePersistent<v8::String> wrapper(isolate, newString);
+
+    stringImpl->ref();
+    wrapper.MarkIndependent();
+    CompressibleStringCacheMapTraits::MapType::PersistentValueReference unused;
+    m_compressibleStringCache.Set(stringImpl, wrapper.Pass(), &unused);

[haraken, 2016/01/19 07:43:35]

Add a comment and mention why you don't want to add CompressibleStrings to
m_lastStringImpl.

[hajimehoshi, 2016/01/19 10:19:20]

You mean m_stringCache? Done.

+
+    return newString;
+}
+
 void StringCache::InvalidateLastString()
 {
     m_lastStringImpl = nullptr;
     m_lastV8String.Reset();
 }
 
 } // namespace blink