Restore in-memory parser cache for V8 compile.

Source/bindings/core/v8/V8ScriptRunner.cpp

Index: Source/bindings/core/v8/V8ScriptRunner.cpp
diff --git a/Source/bindings/core/v8/V8ScriptRunner.cpp b/Source/bindings/core/v8/V8ScriptRunner.cpp
index b187a3b7b995030842c064ffc17465ba0c7186df..7c54004e458b4b0fe038a7129df98c3313e7cff1 100644
--- a/Source/bindings/core/v8/V8ScriptRunner.cpp
+++ b/Source/bindings/core/v8/V8ScriptRunner.cpp
@@ -62,41 +62,44 @@ 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, bool persistCache)

[haraken, 2014/09/03 04:11:43]

CompileAndProduceCache => compileAndProduceCache

[vogelheim, 2014/09/03 11:21:34]

Done.

 {
-    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,
+            persistCache);
     }
-    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);
 }
 
+// Locally used enum to enumerate the actions for V8ScriptRunner::compileScript
+enum CompileScriptAction {
+    ConsumeParserCache,
+    ConsumeCodeCache,
+    ProduceParserCache,
+    ProduceCodeCache,
+    ProduceInMemoryParserCache,
+    Off
+};
+
 } // namespace
 
 v8::Local<v8::Script> V8ScriptRunner::compileScript(const ScriptSourceCode& source, v8::Isolate* isolate, AccessControlStatus corsStatus, V8CacheOptions cacheOptions)
@@ -117,41 +120,59 @@ 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);
-        }
+    // Device on the compile options (caching).

[marja, 2014/09/03 07:17:47]

"Device on"? (Obvsly I'm not a native speaker but what does "device on" mean?)

[vogelheim, 2014/09/03 11:21:34]

I meant: "Decide on ...". Not sure what happened there. If had written this on
my phone, I'd blame autocorrect.

+    CompileScriptAction compileScriptAction;
+    unsigned cacheTag = 0;
+    if (!resource || !resource->url().protocolIsInHTTPFamily() || code->Length() < 1024) {
+        compileScriptAction = Off;
     } else {
-        // No caching:
+        switch (cacheOptions) {
+        case V8CacheOptionsParse:
+            cacheTag = StringHash::hash(v8::V8::GetVersion()) * 2;
+            compileScriptAction = resource->cachedMetadata(cacheTag)
+                ? ConsumeParserCache : ProduceParserCache;
+            break;
+        case V8CacheOptionsCode:
+            cacheTag = StringHash::hash(v8::V8::GetVersion()) * 2 + 1;
+            compileScriptAction = resource->cachedMetadata(cacheTag)
+                ? ConsumeCodeCache : ProduceCodeCache;
+            break;
+        case V8CacheOptionsOff:
+        default:
+            // 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;
+            compileScriptAction = resource->cachedMetadata(cacheTag)
+                ? ConsumeParserCache : ProduceInMemoryParserCache;
+            break;
+        };
+    }
+
+    v8::Local<v8::Script> script;
+    switch (compileScriptAction) {

[marja, 2014/09/03 07:17:47]

Sounds a bit duplicate to first gather all the information in
compileScriptAction first, and right after that, split it to "cache option" &
"persist" & which function to call.

Can you do this in one go? Can't the switch above call the right function with
the right parameters? Does it get messy if you do that?

[vogelheim, 2014/09/03 11:21:34]

Hmm. Done. That was meant to separate the decision from the action, but I admit
it didn't really buy much. Please have a look at whether you like the new
version better.

+    case ProduceParserCache:
+        script = CompileAndProduceCache(code, origin, resource, isolate, v8::ScriptCompiler::kProduceParserCache, cacheTag, true);
+        break;
+    case ProduceCodeCache:
+        script = CompileAndProduceCache(code, origin, resource, isolate, v8::ScriptCompiler::kProduceCodeCache, cacheTag, true);
+        break;
+    case ConsumeParserCache:
+        script = CompileAndConsumeCache(code, origin, resource, isolate, v8::ScriptCompiler::kConsumeParserCache, cacheTag);
+        break;
+    case ConsumeCodeCache:
+        script = CompileAndConsumeCache(code, origin, resource, isolate, v8::ScriptCompiler::kConsumeCodeCache, cacheTag);
+        break;
+    case ProduceInMemoryParserCache:
+        script = CompileAndProduceCache(code, origin, resource, isolate, v8::ScriptCompiler::kProduceParserCache, cacheTag, false);
+        break;
+    case Off:
         v8::ScriptCompiler::Source source(code, origin);
-        script = v8::ScriptCompiler::Compile(
-            isolate, &source, v8::ScriptCompiler::kNoCompileOptions);
+        script = v8::ScriptCompiler::Compile(isolate, &source, v8::ScriptCompiler::kNoCompileOptions);
+        break;
     }
+
     return script;
 }