[Bindings] Create and use V8 context snapshots

third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp

Index: third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
diff --git a/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp b/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
index cf34f7b9da16048600d8b887495f3dbeb03fcea6..c060de94bbd9bb326831be166835139dd12bc208 100644
--- a/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
+++ b/third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
@@ -27,6 +27,7 @@
 
 #include <memory>
 
+#include "platform/RuntimeEnabledFeatures.h"
 #include "platform/ScriptForbiddenScope.h"
 #include "platform/WebTaskRunner.h"
 #include "platform/bindings/DOMDataStore.h"
@@ -52,18 +53,31 @@ static void MicrotasksCompletedCallback(v8::Isolate* isolate) {
   V8PerIsolateData::From(isolate)->RunEndOfScopeTasks();
 }
 
-V8PerIsolateData::V8PerIsolateData(WebTaskRunner* task_runner)
-    : isolate_holder_(
+V8PerIsolateData::V8PerIsolateData(WebTaskRunner* task_runner,
+                                   intptr_t* table,
+                                   V8ContextMode v8_context_mode)
+    : v8_context_mode_(v8_context_mode),
+      isolate_holder_(
           task_runner ? task_runner->ToSingleThreadTaskRunner() : nullptr,
           gin::IsolateHolder::kSingleThread,
           IsMainThread() ? gin::IsolateHolder::kDisallowAtomicsWait
-                         : gin::IsolateHolder::kAllowAtomicsWait),
+                         : gin::IsolateHolder::kAllowAtomicsWait,
+          table,
+          v8_context_mode_ == V8ContextMode::kUseSnapshot ? &startup_data_
+                                                          : nullptr),

[haraken, 2017/07/04 15:04:20]

Nit: It looks a bit weird to do the heavy work during the constructor.

Would it be possible to create a helper function in isolate_holder.h (e.g.,
IsolateHolder::CreateIsolate()) and call it at line 75?

I don't fully understand why the v8::Islate needs to be initialized during the
constructor.

[peria, 2017/07/07 06:22:03]

Hmm, of course it is technically possible, but what does it change in practice?

Instantiation of v8::Isolate (and gin::IsolateHolder) are done here to guarantee
their lifetime same with V8PerIsolateData.

[haraken, 2017/07/07 06:59:11]

I just don't really like doing a complex thing during a constructor because the
object is half-backed.

Can we delay instantiating the isolate instantiation to line 75 as well? In
short, I'd prefer moving what you're currently doing in IsolateHolder's
constructor to line 75.

[peria, 2017/07/10 03:39:13]

Line 75 is also in V8PerIsolateData's constructor, and it is half-baked there.
What the difference here and there?
And in that sense, IsolateHolder is also half-baked instance at line #75,
so I have to move some other members *below* #75 together.

I think your change will make it more dangerous.

FYI, Jochen opposed to instantiate isolate separately.
(https://codereview.chromium.org/2897853002/#msg18)

[haraken, 2017/07/10 04:58:46]

In this specific case, it will be safe because V8PerIsolateData doesn't have a
vtable (by accident).

However, if V8PerIsolateData has a vtable, it is not correctly initialized until
line 75. That's a difference between before line 75 and after line 75. Because
of the difference, there are a couple of restrictions about what we can do
during the constructor (i.e., before 75) e.g., we're not allowed to call a V8
API that may cause an incremental marking. We've spent lots of time fixing
subtle bugs caused by that (we've removed V8 APIs from almost all object
constructors.).

So, in general I want to avoid doing complex things (e.g., calling V8 APIs)
during an object constructor.

[haraken, 2017/07/10 04:58:46]

In this particular case, it will be safe.

However, if V8PerIsolateData has a vtable, it won't be correctly initialized
until line 75. That's the difference between before line 75 and after line 75.
For example, we've removed V8 APIs from object constructors (i.e., before line
75) because the V8 API may trigger an incremental marking, access the object
whose vtable is not yet initialized and crash.

So in general I want to avoid doing complex things (e.g., calling V8 APIs)
during an object constructor.

Would it be hard to refactor the code like this?

V8PerIsolateData() : ...  // Initialize only members that don't need
isolate_holder_.
{
  isolate_holder.Initialize();  // Read the startup_data_ and initialize the
isolate.
  ...;  // Initialize members that need isolate_holder_.
}

[peria, 2017/07/10 05:37:56]

Agreed to avoid doing complex things in V8PerIsolateData, and tried it,
but it seems difficult because of the newly introduced V8GlobalValueMap
|interface_template_map_for_v8_snapshot_|.  It has DISALLOW_NEW().

+      interface_template_map_for_v8_snapshot_(GetIsolate()),
       string_cache_(WTF::WrapUnique(new StringCache(GetIsolate()))),
       private_property_(V8PrivateProperty::Create()),
       constructor_mode_(ConstructorMode::kCreateNewObject),
       use_counter_disabled_(false),
       is_handling_recursion_level_error_(false),
       is_reporting_exception_(false) {
+  // If it fails to load the snapshot file, falls back to kDontUseSnapshot mode.

[haraken, 2017/07/04 15:04:20]

When can this happen?

[peria, 2017/07/07 06:22:03]

I hope this happens just in some unit tests, which do not have
v8_context_snapshot.blob binded.

[haraken, 2017/07/07 06:59:11]

Let's add a comment.

+  // TODO(peria): Remove this fallback routine.
+  if (v8_context_mode_ == V8ContextMode::kUseSnapshot && !startup_data_.data) {
+    v8_context_mode_ = V8ContextMode::kDontUseSnapshot;
+  }
+
   // FIXME: Remove once all v8::Isolate::GetCurrent() calls are gone.
   GetIsolate()->Enter();
   GetIsolate()->AddBeforeCallEnteredCallback(&BeforeCallEnteredCallback);
@@ -72,6 +86,28 @@ V8PerIsolateData::V8PerIsolateData(WebTaskRunner* task_runner)
     g_main_thread_per_isolate_data = this;
 }
 
+V8PerIsolateData::V8PerIsolateData(intptr_t* reference_table)
+    : v8_context_mode_(V8ContextMode::kTakeSnapshot),
+      isolate_holder_(reference_table, nullptr),
+      interface_template_map_for_v8_snapshot_(GetIsolate()),
+      string_cache_(WTF::WrapUnique(new StringCache(GetIsolate()))),
+      private_property_(V8PrivateProperty::Create()),
+      constructor_mode_(ConstructorMode::kCreateNewObject),
+      use_counter_disabled_(false),
+      is_handling_recursion_level_error_(false),
+      is_reporting_exception_(false) {
+  // This constructor assumes to be used to take V8 snapshot of context of
+  // Blink, and it must be run on the main thread.

[haraken, 2017/07/04 15:04:20]

// This constructor is used for taking a V8 context snapshot. It must run on the
main thread.

Move this comment to line 89.

[peria, 2017/07/07 06:22:03]

Done.

+  CHECK(IsMainThread());
+
+  // SnapshotCreator enters the isolate, so we don't call Isolate::Enter() here.
+
+  // FIXME: Remove once all v8::Isolate::GetCurrent() calls are gone.

[haraken, 2017/07/04 15:04:20]

Remove this comment. This FIXME is for GetIsolate()->Enter().

[peria, 2017/07/07 06:22:03]

Done.

+  GetIsolate()->AddBeforeCallEnteredCallback(&BeforeCallEnteredCallback);
+  GetIsolate()->AddMicrotasksCompletedCallback(&MicrotasksCompletedCallback);

[haraken, 2017/07/04 15:04:20]

I don't think we need line 106 and 107. What happens if we remove the two lines?

[peria, 2017/07/10 03:39:13]

Seems no problems.
Done.

+  g_main_thread_per_isolate_data = this;
+}
+
 V8PerIsolateData::~V8PerIsolateData() {}
 
 v8::Isolate* V8PerIsolateData::MainThreadIsolate() {
@@ -79,8 +115,24 @@ v8::Isolate* V8PerIsolateData::MainThreadIsolate() {
   return g_main_thread_per_isolate_data->GetIsolate();
 }
 
-v8::Isolate* V8PerIsolateData::Initialize(WebTaskRunner* task_runner) {
-  V8PerIsolateData* data = new V8PerIsolateData(task_runner);
+v8::Isolate* V8PerIsolateData::Initialize(WebTaskRunner* task_runner,
+                                          intptr_t* reference_table,
+                                          V8ContextMode context_mode) {
+  DCHECK(context_mode == V8ContextMode::kDontUseSnapshot || reference_table);
+
+  V8PerIsolateData* data = nullptr;
+  if (context_mode == V8ContextMode::kTakeSnapshot) {
+    CHECK(reference_table);
+    data = new V8PerIsolateData(reference_table);
+  } else {
+    if (!RuntimeEnabledFeatures::V8ContextSnapshotEnabled()) {

[haraken, 2017/07/04 15:04:20]

It looks a bit strange to override the setting after calling
V8PerIsolateData::Initialize. Can we move line 128 - 130 to the caller site?

[peria, 2017/07/07 06:22:03]

Done.

+      context_mode = V8ContextMode::kDontUseSnapshot;
+      reference_table = nullptr;
+    }
+    data = new V8PerIsolateData(task_runner, reference_table, context_mode);
+  }
+  DCHECK(data);
+
   v8::Isolate* isolate = data->GetIsolate();
   isolate->SetData(gin::kEmbedderBlink, data);
   return isolate;
@@ -164,6 +216,11 @@ v8::Local<v8::FunctionTemplate> V8PerIsolateData::FindOrCreateOperationTemplate(
 v8::Local<v8::FunctionTemplate> V8PerIsolateData::FindInterfaceTemplate(
     const DOMWrapperWorld& world,
     const void* key) {
+  if (GetV8ContextMode() == V8ContextMode::kTakeSnapshot) {
+    const WrapperTypeInfo* type = reinterpret_cast<const WrapperTypeInfo*>(key);
+    return interface_template_map_for_v8_snapshot_.Get(type);
+  }
+
   auto& map = SelectInterfaceTemplateMap(world);
   auto result = map.find(key);
   if (result != map.end())
@@ -175,8 +232,19 @@ void V8PerIsolateData::SetInterfaceTemplate(
     const DOMWrapperWorld& world,
     const void* key,
     v8::Local<v8::FunctionTemplate> value) {
-  auto& map = SelectInterfaceTemplateMap(world);
-  map.insert(key, v8::Eternal<v8::FunctionTemplate>(GetIsolate(), value));
+  if (GetV8ContextMode() == V8ContextMode::kTakeSnapshot) {
+    auto& map = interface_template_map_for_v8_snapshot_;
+    const WrapperTypeInfo* type = reinterpret_cast<const WrapperTypeInfo*>(key);
+    map.Set(type, value);
+  } else {
+    auto& map = SelectInterfaceTemplateMap(world);
+    map.insert(key, v8::Eternal<v8::FunctionTemplate>(GetIsolate(), value));
+  }
+}
+
+void V8PerIsolateData::ClearPersistentsForV8Snapshot() {
+  interface_template_map_for_v8_snapshot_.Clear();
+  private_property_.reset();
 }
 
 const v8::Eternal<v8::Name>* V8PerIsolateData::FindOrCreateEternalNameCache(