Restore in-memory parser cache for V8 compile.

Also do a minor refactoring of the decision logic, which hopefully
makes it a bit easier to understand.

This is a fix for the performance regression introduced by
crrev.com/432273004, which inadvertently disabled the parser cache
when compiling without any compile options.

R=marja@chromium.org
BUG=404622
BUG=399580

Review URL: https://codereview.chromium.org/528013002

git-svn-id: svn://svn.chromium.org/blink/trunk@181472 bbb929c8-8fbe-4397-9dbb-9b2b20218538

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

@@ -62,39 +62,32 @@ v8::Local<v8::Value> throwStackOverflowExceptionIfNeeded(v8::Isolate* isolate)
     return result;
 }
 
-// Make a decision on whether we want to use V8 caching and how.
-// dataType, produceOption, consumeOption are out parameters.
-bool CacheDecider(
-    const v8::Handle<v8::String> code,
-    const ScriptResource* resource,
-    V8CacheOptions cacheOptions,
-    unsigned* dataType,
-    v8::ScriptCompiler::CompileOptions* compileOption,
-    bool* produce)
+v8::Local<v8::Script> compileAndProduceCache(v8::Handle<v8::String> code, v8::ScriptOrigin origin, ScriptResource* resource, v8::Isolate* isolate, v8::ScriptCompiler::CompileOptions options, unsigned cacheTag, Resource::MetadataCacheType cacheType)
 {
-    if (!resource || !resource->url().protocolIsInHTTPFamily() || code->Length() < 1024)
-        cacheOptions = V8CacheOptionsOff;
-
-    bool useCache = false;
-    switch (cacheOptions) {
-    case V8CacheOptionsOff:
-        *compileOption = v8::ScriptCompiler::kNoCompileOptions;
-        useCache = false;
-        break;
-    case V8CacheOptionsParse:
-        *dataType = StringHash::hash(v8::V8::GetVersion()) * 2;
-        *produce = !resource->cachedMetadata(*dataType);
-        *compileOption = *produce ? v8::ScriptCompiler::kProduceParserCache : v8::ScriptCompiler::kConsumeParserCache;
-        useCache = true;
-        break;
-    case V8CacheOptionsCode:
-        *dataType = StringHash::hash(v8::V8::GetVersion()) * 2 + 1;
-        *produce = !resource->cachedMetadata(*dataType);
-        *compileOption = *produce ? v8::ScriptCompiler::kProduceCodeCache : v8::ScriptCompiler::kConsumeCodeCache;
-        useCache = true;
-        break;
+    v8::ScriptCompiler::Source source(code, origin);
+    v8::Local<v8::Script> script = v8::ScriptCompiler::Compile(isolate, &source, options);
+    const v8::ScriptCompiler::CachedData* cachedData = source.GetCachedData();
+    if (resource && cachedData) {
+        resource->clearCachedMetadata();
+        resource->setCachedMetadata(
+            cacheTag,
+            reinterpret_cast<const char*>(cachedData->data),
+            cachedData->length,
+            cacheType);
     }
-    return useCache;
+    return script;
+}
+
+v8::Local<v8::Script> compileAndConsumeCache(v8::Handle<v8::String> code, v8::ScriptOrigin origin, ScriptResource* resource, v8::Isolate* isolate, v8::ScriptCompiler::CompileOptions options, unsigned cacheTag)
+{
+    // Consume existing cache data:
+    CachedMetadata* cachedMetadata = resource->cachedMetadata(cacheTag);
+    v8::ScriptCompiler::CachedData* cachedData = new v8::ScriptCompiler::CachedData(
+        reinterpret_cast<const uint8_t*>(cachedMetadata->data()),
+        cachedMetadata->size(),
+        v8::ScriptCompiler::CachedData::BufferNotOwned);
+    v8::ScriptCompiler::Source source(code, origin, cachedData);
+    return v8::ScriptCompiler::Compile(isolate, &source, options);
 }
 
 } // namespace
@@ -117,40 +110,36 @@ v8::Local<v8::Script> V8ScriptRunner::compileScript(v8::Handle<v8::String> code,
     v8::Handle<v8::Boolean> isSharedCrossOrigin = corsStatus == SharableCrossOrigin ? v8::True(isolate) : v8::False(isolate);
     v8::ScriptOrigin origin(name, line, column, isSharedCrossOrigin);
 
-    // V8 supports several forms of caching. Decide on the cache mode and call
-    // ScriptCompiler::Compile with suitable options.
-    unsigned dataTypeID = 0;
-    v8::ScriptCompiler::CompileOptions compileOption = v8::ScriptCompiler::kNoCompileOptions;
-    bool produce;
     v8::Local<v8::Script> script;
-    if (CacheDecider(code, resource, cacheOptions, &dataTypeID, &compileOption, &produce)) {
-        if (produce) {
-            // Produce new cache data:
-            v8::ScriptCompiler::Source source(code, origin);
-            script = v8::ScriptCompiler::Compile(isolate, &source, compileOption);
-            const v8::ScriptCompiler::CachedData* cachedData = source.GetCachedData();
-            if (cachedData) {
-                resource->clearCachedMetadata();
-                resource->setCachedMetadata(
-                    dataTypeID,
-                    reinterpret_cast<const char*>(cachedData->data),
-                    cachedData->length);
-            }
-        } else {
-            // Consume existing cache data:
-            CachedMetadata* cachedMetadata = resource->cachedMetadata(dataTypeID);
-            v8::ScriptCompiler::CachedData* cachedData = new v8::ScriptCompiler::CachedData(
-                reinterpret_cast<const uint8_t*>(cachedMetadata->data()),
-                cachedMetadata->size(),
-                v8::ScriptCompiler::CachedData::BufferNotOwned);
-            v8::ScriptCompiler::Source source(code, origin, cachedData);
-            script = v8::ScriptCompiler::Compile(isolate, &source, compileOption);
-        }
-    } else {
-        // No caching:
+    unsigned cacheTag = 0;
+    if (!resource || !resource->url().protocolIsInHTTPFamily() || code->Length() < 1024) {
         v8::ScriptCompiler::Source source(code, origin);
-        script = v8::ScriptCompiler::Compile(
-            isolate, &source, v8::ScriptCompiler::kNoCompileOptions);
+        script = v8::ScriptCompiler::Compile(isolate, &source, v8::ScriptCompiler::kNoCompileOptions);
+    } else {
+        switch (cacheOptions) {
+        case V8CacheOptionsParse:
+            cacheTag = StringHash::hash(v8::V8::GetVersion()) * 2;
+            script = resource->cachedMetadata(cacheTag)
+                ? compileAndConsumeCache(code, origin, resource, isolate, v8::ScriptCompiler::kConsumeParserCache, cacheTag)
+                : compileAndProduceCache(code, origin, resource, isolate, v8::ScriptCompiler::kProduceParserCache, cacheTag, Resource::SendToPlatform);
+            break;
+        case V8CacheOptionsCode:
+            cacheTag = StringHash::hash(v8::V8::GetVersion()) * 2 + 1;
+            script = resource->cachedMetadata(cacheTag)
+                ? compileAndConsumeCache(code, origin, resource, isolate, v8::ScriptCompiler::kConsumeCodeCache, cacheTag)
+                : compileAndProduceCache(code, origin, resource, isolate, v8::ScriptCompiler::kProduceCodeCache, cacheTag, Resource::SendToPlatform);
+            break;
+        case V8CacheOptionsOff:
+            // Previous behaviour was to always generate an in-memory parser
+            // cache. We emulate this here.
+            // TODO(vogelheim): Determine whether this should get its own
+            //                  setting, so we can also have a true 'off'.
+            cacheTag = StringHash::hash(v8::V8::GetVersion()) * 2;
+            script = resource->cachedMetadata(cacheTag)
+                ? compileAndConsumeCache(code, origin, resource, isolate, v8::ScriptCompiler::kConsumeParserCache, cacheTag)
+                : compileAndProduceCache(code, origin, resource, isolate, v8::ScriptCompiler::kProduceParserCache, cacheTag, Resource::CacheLocally);
+            break;
+        }
     }
     return script;
 }

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

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+#include "bindings/core/v8/V8ScriptRunner.h"
+
+#include "core/fetch/ScriptResource.h"
+#include <gtest/gtest.h>
+#include <v8.h>
+
+namespace blink {
+
+namespace {
+
+class V8ScriptRunnerTest : public ::testing::Test {
+public:
+    V8ScriptRunnerTest() { }
+    virtual ~V8ScriptRunnerTest() { }
+
+    static void SetUpTestCase()
+    {
+        cacheTagParser = StringHash::hash(v8::V8::GetVersion()) * 2;
+        cacheTagCode = cacheTagParser + 1;
+    }
+
+    virtual void SetUp() OVERRIDE
+    {
+        // To trick various layers of caching, increment a counter for each
+        // test and use it in code(), fielname() and url().
+        counter++;
+    }
+
+    virtual void TearDown() OVERRIDE
+    {
+        m_resourceRequest.clear();
+        m_resource.clear();
+    }
+
+    v8::Isolate* isolate()
+    {
+        return v8::Isolate::GetCurrent();
+    }
+    v8::Local<v8::String> v8String(const char* value)
+    {
+        return v8::String::NewFromOneByte(
+            isolate(), reinterpret_cast<const uint8_t*>(value));
+    }
+    v8::Local<v8::String> v8String(const WTF::String& value)
+    {
+        return v8String(value.ascii().data());
+    }
+    WTF::String code()
+    {
+        // Simple function for testing. Note:
+        // - Add counter to trick V8 code cache.
+        // - Pad counter to 1000 digits, to trick minimal cacheability threshold.
+        return WTF::String::format("a = function() { 1 + 1; } // %01000d\n", counter);
+    }
+    WTF::String filename()
+    {
+        return WTF::String::format("whatever%d.js", counter);
+    }
+    WTF::String url()
+    {
+        return WTF::String::format("http://bla.com/bla%d", counter);
+    }
+
+    bool compileScript(V8CacheOptions cacheOptions)
+    {
+        v8::HandleScope handleScope(isolate());
+        v8::Local<v8::Context> context = v8::Context::New(isolate());
+        v8::Context::Scope contextScope(context);
+        return !V8ScriptRunner::compileScript(
+            v8String(code()), filename(), WTF::TextPosition(), m_resource.get(),
+            isolate(), NotSharableCrossOrigin, cacheOptions)
+            .IsEmpty();
+    }
+
+    void setEmptyResource()
+    {
+        m_resourceRequest = WTF::adoptPtr(new ResourceRequest);
+        m_resource = WTF::adoptPtr(new ScriptResource(*m_resourceRequest.get(), "text/utf-8"));
+    }
+
+    void setResource()
+    {
+        m_resourceRequest = WTF::adoptPtr(new ResourceRequest(url()));
+        m_resource = WTF::adoptPtr(new ScriptResource(*m_resourceRequest.get(), "text/utf-8"));
+    }
+
+protected:
+    WTF::OwnPtr<ResourceRequest> m_resourceRequest;
+    WTF::OwnPtr<ScriptResource> m_resource;
+
+    static unsigned cacheTagParser;
+    static unsigned cacheTagCode;
+    static int counter;
+};
+
+unsigned V8ScriptRunnerTest::cacheTagParser = 0;
+unsigned V8ScriptRunnerTest::cacheTagCode = 0;
+int V8ScriptRunnerTest::counter = 0;
+
+TEST_F(V8ScriptRunnerTest, resourcelessShouldPass)
+{
+    EXPECT_TRUE(compileScript(V8CacheOptionsOff));
+    EXPECT_TRUE(compileScript(V8CacheOptionsParse));
+    EXPECT_TRUE(compileScript(V8CacheOptionsCode));
+}
+
+TEST_F(V8ScriptRunnerTest, emptyResourceDoesNothing)
+{
+    setEmptyResource();
+    EXPECT_TRUE(compileScript(V8CacheOptionsOff));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+
+    EXPECT_TRUE(compileScript(V8CacheOptionsParse));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+
+    EXPECT_TRUE(compileScript(V8CacheOptionsCode));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+}
+
+TEST_F(V8ScriptRunnerTest, defaultOptions)
+{
+    setResource();
+    EXPECT_TRUE(compileScript(V8CacheOptionsOff));
+    EXPECT_TRUE(m_resource->cachedMetadata(cacheTagParser));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+}
+
+TEST_F(V8ScriptRunnerTest, parseOptions)
+{
+    setResource();
+    EXPECT_TRUE(compileScript(V8CacheOptionsParse));
+    EXPECT_TRUE(m_resource->cachedMetadata(cacheTagParser));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+}
+
+TEST_F(V8ScriptRunnerTest, codeOptions)
+{
+    setResource();
+    EXPECT_TRUE(compileScript(V8CacheOptionsCode));
+    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
+
+    // TODO(vogelheim): Code caching is presently still disabled.
+    //                  Enable EXPECT when code caching lands.
+    // EXPECT_TRUE(m_resource->cachedMetadata(cacheTagCode));
+}
+
+} // namespace
+
+} // namespace blink

third_party/WebKit/Source/bindings/core/v8/v8.gypi

@@ -167,6 +167,7 @@
             'ScriptPromisePropertyTest.cpp',
             'SerializedScriptValueTest.cpp',
             'V8BindingTest.cpp',
+            'V8ScriptRunnerTest.cpp',
         ],
     },
 }

third_party/WebKit/Source/core/fetch/Resource.cpp

@@ -413,7 +413,7 @@ void Resource::setSerializedCachedMetadata(const char* data, size_t size)
     m_cachedMetadata = CachedMetadata::deserialize(data, size);
 }
 
-void Resource::setCachedMetadata(unsigned dataTypeID, const char* data, size_t size)
+void Resource::setCachedMetadata(unsigned dataTypeID, const char* data, size_t size, MetadataCacheType cacheType)
 {
     // Currently, only one type of cached metadata per resource is supported.
     // If the need arises for multiple types of metadata per resource this could
@@ -421,8 +421,11 @@ void Resource::setCachedMetadata(unsigned dataTypeID, const char* data, size_t s
     ASSERT(!m_cachedMetadata);
 
     m_cachedMetadata = CachedMetadata::create(dataTypeID, data, size);
-    const Vector<char>& serializedData = m_cachedMetadata->serialize();
-    blink::Platform::current()->cacheMetadata(m_response.url(), m_response.responseTime(), serializedData.data(), serializedData.size());
+
+    if (cacheType == SendToPlatform) {
+        const Vector<char>& serializedData = m_cachedMetadata->serialize();
+        blink::Platform::current()->cacheMetadata(m_response.url(), m_response.responseTime(), serializedData.data(), serializedData.size());
+    }
 }
 
 void Resource::clearCachedMetadata()

third_party/WebKit/Source/core/fetch/Resource.h

@@ -82,6 +82,11 @@ public:
         DecodeError
     };
 
+    enum MetadataCacheType {
+        SendToPlatform, // send cache data to blink::Platform::cacheMetadata
+        CacheLocally // cache only in Resource's member variables
+    };
+
     Resource(const ResourceRequest&, Type);
 #if ENABLE(OILPAN)
     virtual ~Resource();
@@ -195,7 +200,7 @@ public:
     // Caches the given metadata in association with this resource and suggests
     // that the platform persist it. The dataTypeID is a pseudo-randomly chosen
     // identifier that is used to distinguish data generated by the caller.
-    void setCachedMetadata(unsigned dataTypeID, const char*, size_t);
+    void setCachedMetadata(unsigned dataTypeID, const char*, size_t, MetadataCacheType = SendToPlatform);
 
     // Reset existing metadata, to allow setting new data.
     void clearCachedMetadata();