Let the second pass phantom callbacks run in a separate task on the foreground thread.

R=jochen@chromium.org
LOG=y
BUG=

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

Cr-Commit-Position: refs/heads/master@{#29680}

diff --git a/include/v8-util.h b/include/v8-util.h
index 6454a19..c996c99 100644 (file)
--- a/include/v8-util.h
+++ b/include/v8-util.h
@@ -133,6 +133,8 @@ class DefaultGlobalMapTraits : public StdMapTraits<K, V> {
     return K();
   }
   static void DisposeCallbackData(WeakCallbackDataType* data) {}
+  static void OnWeakCallback(
+      const WeakCallbackInfo<WeakCallbackDataType>& data) {}
   static void Dispose(Isolate* isolate, Global<V> value, K key) {}
   // This is a second pass callback, so SetSecondPassCallback cannot be called.
   static void DisposeWeak(const WeakCallbackInfo<WeakCallbackDataType>& data) {}
@@ -452,7 +454,7 @@ class GlobalValueMap : public PersistentValueMapBase<K, V, Traits> {
               : WeakCallbackType::kParameter;
       Local<V> value(Local<V>::New(this->isolate(), *persistent));
       persistent->template SetWeak<typename Traits::WeakCallbackDataType>(
-          Traits::WeakCallbackParameter(this, key, value), FirstWeakCallback,
+          Traits::WeakCallbackParameter(this, key, value), OnWeakCallback,
           callback_type);
     }
     PersistentContainerValue old_value =
@@ -471,12 +473,13 @@ class GlobalValueMap : public PersistentValueMapBase<K, V, Traits> {
   }
 
  private:
-  static void FirstWeakCallback(
+  static void OnWeakCallback(
       const WeakCallbackInfo<typename Traits::WeakCallbackDataType>& data) {
     if (Traits::kCallbackType != kNotWeak) {
       auto map = Traits::MapFromWeakCallbackInfo(data);
       K key = Traits::KeyFromWeakCallbackInfo(data);
       map->RemoveWeak(key);
+      Traits::OnWeakCallback(data);
       data.SetSecondPassCallback(SecondWeakCallback);
     }
   }

diff --git a/samples/process.cc b/samples/process.cc
index a62950a..c18fb41 100644 (file)
--- a/samples/process.cc
+++ b/samples/process.cc
@@ -666,11 +666,13 @@ StringHttpRequest kSampleRequests[kSampleSize] = {
 };
 
 
-bool ProcessEntries(HttpRequestProcessor* processor, int count,
-                    StringHttpRequest* reqs) {
+bool ProcessEntries(v8::Platform* platform, HttpRequestProcessor* processor,
+                    int count, StringHttpRequest* reqs) {
   for (int i = 0; i < count; i++) {
-    if (!processor->Process(&reqs[i]))
-      return false;
+    bool result = processor->Process(&reqs[i]);
+    while (v8::platform::PumpMessageLoop(platform, Isolate::GetCurrent()))
+      continue;
+    if (!result) return false;
   }
   return true;
 }
@@ -713,7 +715,7 @@ int main(int argc, char* argv[]) {
     fprintf(stderr, "Error initializing processor.\n");
     return 1;
   }
-  if (!ProcessEntries(&processor, kSampleSize, kSampleRequests))
+  if (!ProcessEntries(platform, &processor, kSampleSize, kSampleRequests))
     return 1;
   PrintMap(&output);
 }

diff --git a/samples/shell.cc b/samples/shell.cc
index 3a743d6..a6b01d9 100644 (file)
--- a/samples/shell.cc
+++ b/samples/shell.cc
@@ -45,8 +45,9 @@
 
 
 v8::Local<v8::Context> CreateShellContext(v8::Isolate* isolate);
-void RunShell(v8::Local<v8::Context> context);
-int RunMain(v8::Isolate* isolate, int argc, char* argv[]);
+void RunShell(v8::Local<v8::Context> context, v8::Platform* platform);
+int RunMain(v8::Isolate* isolate, v8::Platform* platform, int argc,
+            char* argv[]);
 bool ExecuteString(v8::Isolate* isolate, v8::Local<v8::String> source,
                    v8::Local<v8::Value> name, bool print_result,
                    bool report_exceptions);
@@ -94,8 +95,8 @@ int main(int argc, char* argv[]) {
       return 1;
     }
     v8::Context::Scope context_scope(context);
-    result = RunMain(isolate, argc, argv);
-    if (run_shell) RunShell(context);
+    result = RunMain(isolate, platform, argc, argv);
+    if (run_shell) RunShell(context, platform);
   }
   isolate->Dispose();
   v8::V8::Dispose();
@@ -269,7 +270,8 @@ v8::MaybeLocal<v8::String> ReadFile(v8::Isolate* isolate, const char* name) {
 
 
 // Process remaining command line arguments and execute files
-int RunMain(v8::Isolate* isolate, int argc, char* argv[]) {
+int RunMain(v8::Isolate* isolate, v8::Platform* platform, int argc,
+            char* argv[]) {
   for (int i = 1; i < argc; i++) {
     const char* str = argv[i];
     if (strcmp(str, "--shell") == 0) {
@@ -292,7 +294,9 @@ int RunMain(v8::Isolate* isolate, int argc, char* argv[]) {
                .ToLocal(&source)) {
         return 1;
       }
-      if (!ExecuteString(isolate, source, file_name, false, true)) return 1;
+      bool success = ExecuteString(isolate, source, file_name, false, true);
+      while (v8::platform::PumpMessageLoop(platform, isolate)) continue;
+      if (!success) return 1;
     } else {
       // Use all other arguments as names of files to load and run.
       v8::Local<v8::String> file_name =
@@ -303,7 +307,9 @@ int RunMain(v8::Isolate* isolate, int argc, char* argv[]) {
         fprintf(stderr, "Error reading '%s'\n", str);
         continue;
       }
-      if (!ExecuteString(isolate, source, file_name, false, true)) return 1;
+      bool success = ExecuteString(isolate, source, file_name, false, true);
+      while (v8::platform::PumpMessageLoop(platform, isolate)) continue;
+      if (!success) return 1;
     }
   }
   return 0;
@@ -311,7 +317,7 @@ int RunMain(v8::Isolate* isolate, int argc, char* argv[]) {
 
 
 // The read-eval-execute loop of the shell.
-void RunShell(v8::Local<v8::Context> context) {
+void RunShell(v8::Local<v8::Context> context, v8::Platform* platform) {
   fprintf(stderr, "V8 version %s [sample shell]\n", v8::V8::GetVersion());
   static const int kBufferSize = 256;
   // Enter the execution environment before evaluating any code.
@@ -330,6 +336,8 @@ void RunShell(v8::Local<v8::Context> context) {
         v8::String::NewFromUtf8(context->GetIsolate(), str,
                                 v8::NewStringType::kNormal).ToLocalChecked(),
         name, true, true);
+    while (v8::platform::PumpMessageLoop(platform, context->GetIsolate()))
+      continue;
   }
   fprintf(stderr, "\n");
 }

diff --git a/src/d8.cc b/src/d8.cc
index 19aa86b..b68e4ba 100644 (file)
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -349,6 +349,7 @@ bool Shell::ExecuteString(Isolate* isolate, Handle<String> source,
       return false;
     }
     result = script->Run();
+    EmptyMessageQueues(isolate);
     data->realm_current_ = data->realm_switch_;
   }
   if (result.IsEmpty()) {
@@ -2009,6 +2010,11 @@ void Shell::CollectGarbage(Isolate* isolate) {
 }
 
 
+void Shell::EmptyMessageQueues(Isolate* isolate) {
+  while (v8::platform::PumpMessageLoop(g_platform, isolate)) continue;
+}
+
+
 #ifndef V8_SHARED
 bool Shell::SerializeValue(Isolate* isolate, Handle<Value> value,
                            const ObjectList& to_transfer,

diff --git a/src/d8.h b/src/d8.h
index ff83544..bb5592b 100644 (file)
--- a/src/d8.h
+++ b/src/d8.h
@@ -366,6 +366,7 @@ class Shell : public i::AllStatic {
   static void Exit(int exit_code);
   static void OnExit(Isolate* isolate);
   static void CollectGarbage(Isolate* isolate);
+  static void EmptyMessageQueues(Isolate* isolate);
 
 #ifndef V8_SHARED
   // TODO(binji): stupid implementation for now. Is there an easy way to hash an

diff --git a/src/global-handles.cc b/src/global-handles.cc
index aa6542b..4cd0a18 100644 (file)
--- a/src/global-handles.cc
+++ b/src/global-handles.cc
@@ -495,6 +495,29 @@ class GlobalHandles::NodeIterator {
   DISALLOW_COPY_AND_ASSIGN(NodeIterator);
 };
 
+class GlobalHandles::PendingPhantomCallbacksSecondPassTask : public v8::Task {
+ public:
+  // Takes ownership of the contents of pending_phantom_callbacks, leaving it in
+  // the same state it would be after a call to Clear().
+  PendingPhantomCallbacksSecondPassTask(
+      List<PendingPhantomCallback>* pending_phantom_callbacks, Isolate* isolate)
+      : isolate_(isolate) {
+    pending_phantom_callbacks_.Swap(pending_phantom_callbacks);
+  }
+
+  ~PendingPhantomCallbacksSecondPassTask() override {}
+
+  void Run() override {
+    InvokeSecondPassPhantomCallbacks(&pending_phantom_callbacks_, isolate_);
+  }
+
+ private:
+  List<PendingPhantomCallback> pending_phantom_callbacks_;
+  Isolate* isolate_;
+
+  DISALLOW_COPY_AND_ASSIGN(PendingPhantomCallbacksSecondPassTask);
+};
+
 
 GlobalHandles::GlobalHandles(Isolate* isolate)
     : isolate_(isolate),
@@ -709,6 +732,19 @@ bool GlobalHandles::IterateObjectGroups(ObjectVisitor* v,
 }
 
 
+void GlobalHandles::InvokeSecondPassPhantomCallbacks(
+    List<PendingPhantomCallback>* callbacks, Isolate* isolate) {
+  while (callbacks->length() != 0) {
+    auto callback = callbacks->RemoveLast();
+    DCHECK(callback.node() == nullptr);
+    // No second pass callback required.
+    if (callback.callback() == nullptr) continue;
+    // Fire second pass callback
+    callback.Invoke(isolate);
+  }
+}
+
+
 int GlobalHandles::PostScavengeProcessing(
     const int initial_post_gc_processing_count) {
   int freed_nodes = 0;
@@ -791,7 +827,8 @@ void GlobalHandles::UpdateListOfNewSpaceNodes() {
 }
 
 
-int GlobalHandles::DispatchPendingPhantomCallbacks() {
+int GlobalHandles::DispatchPendingPhantomCallbacks(
+    bool synchronous_second_pass) {
   int freed_nodes = 0;
   {
     // The initial pass callbacks must simply clear the nodes.
@@ -804,14 +841,15 @@ int GlobalHandles::DispatchPendingPhantomCallbacks() {
       freed_nodes++;
     }
   }
-  // The second pass empties the list.
-  while (pending_phantom_callbacks_.length() != 0) {
-    auto callback = pending_phantom_callbacks_.RemoveLast();
-    DCHECK(callback.node() == nullptr);
-    // No second pass callback required.
-    if (callback.callback() == nullptr) continue;
-    // Fire second pass callback.
-    callback.Invoke(isolate());
+  if (pending_phantom_callbacks_.length() > 0) {
+    if (synchronous_second_pass) {
+      InvokeSecondPassPhantomCallbacks(&pending_phantom_callbacks_, isolate());
+    } else {
+      auto* task = new PendingPhantomCallbacksSecondPassTask(
+          &pending_phantom_callbacks_, isolate());
+      V8::GetCurrentPlatform()->CallOnForegroundThread(
+          reinterpret_cast<v8::Isolate*>(isolate()), task);
+    }
   }
   pending_phantom_callbacks_.Clear();
   return freed_nodes;
@@ -838,14 +876,17 @@ void GlobalHandles::PendingPhantomCallback::Invoke(Isolate* isolate) {
 }
 
 
-int GlobalHandles::PostGarbageCollectionProcessing(GarbageCollector collector) {
+int GlobalHandles::PostGarbageCollectionProcessing(
+    GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) {
   // Process weak global handle callbacks. This must be done after the
   // GC is completely done, because the callbacks may invoke arbitrary
   // API functions.
   DCHECK(isolate_->heap()->gc_state() == Heap::NOT_IN_GC);
   const int initial_post_gc_processing_count = ++post_gc_processing_count_;
   int freed_nodes = 0;
-  freed_nodes += DispatchPendingPhantomCallbacks();
+  bool synchronous_second_pass =
+      (gc_callback_flags & kGCCallbackFlagForced) != 0;
+  freed_nodes += DispatchPendingPhantomCallbacks(synchronous_second_pass);
   if (initial_post_gc_processing_count != post_gc_processing_count_) {
     // If the callbacks caused a nested GC, then return.  See comment in
     // PostScavengeProcessing.

diff --git a/src/global-handles.h b/src/global-handles.h
index 6724847..0ee8c20 100644 (file)
--- a/src/global-handles.h
+++ b/src/global-handles.h
@@ -181,7 +181,8 @@ class GlobalHandles {
 
   // Process pending weak handles.
   // Returns the number of freed nodes.
-  int PostGarbageCollectionProcessing(GarbageCollector collector);
+  int PostGarbageCollectionProcessing(
+      GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags);
 
   // Iterates over all strong handles.
   void IterateStrongRoots(ObjectVisitor* v);
@@ -287,17 +288,21 @@ class GlobalHandles {
   // don't assign any initial capacity.
   static const int kObjectGroupConnectionsCapacity = 20;
 
+  class PendingPhantomCallback;
+
   // Helpers for PostGarbageCollectionProcessing.
+  static void InvokeSecondPassPhantomCallbacks(
+      List<PendingPhantomCallback>* callbacks, Isolate* isolate);
   int PostScavengeProcessing(int initial_post_gc_processing_count);
   int PostMarkSweepProcessing(int initial_post_gc_processing_count);
-  int DispatchPendingPhantomCallbacks();
+  int DispatchPendingPhantomCallbacks(bool synchronous_second_pass);
   void UpdateListOfNewSpaceNodes();
 
   // Internal node structures.
   class Node;
   class NodeBlock;
   class NodeIterator;
-  class PendingPhantomCallback;
+  class PendingPhantomCallbacksSecondPassTask;
 
   Isolate* isolate_;
 

diff --git a/src/heap/heap.cc b/src/heap/heap.cc
index eef3742..3340a0f 100644 (file)
--- a/src/heap/heap.cc
+++ b/src/heap/heap.cc
@@ -1306,7 +1306,8 @@ bool Heap::PerformGarbageCollection(
     AllowHeapAllocation allow_allocation;
     GCTracer::Scope scope(tracer(), GCTracer::Scope::EXTERNAL);
     freed_global_handles =
-        isolate_->global_handles()->PostGarbageCollectionProcessing(collector);
+        isolate_->global_handles()->PostGarbageCollectionProcessing(
+            collector, gc_callback_flags);
   }
   gc_post_processing_depth_--;
 

diff --git a/src/list-inl.h b/src/list-inl.h
index 98f0343..47653ef 100644 (file)
--- a/src/list-inl.h
+++ b/src/list-inl.h
@@ -125,6 +125,12 @@ bool List<T, P>::RemoveElement(const T& elm) {
   return false;
 }
 
+template <typename T, class P>
+void List<T, P>::Swap(List<T, P>* list) {
+  std::swap(data_, list->data_);
+  std::swap(length_, list->length_);
+  std::swap(capacity_, list->capacity_);
+}
 
 template<typename T, class P>
 void List<T, P>::Allocate(int length, P allocator) {

diff --git a/src/list.h b/src/list.h
index b636449..8021a9f 100644 (file)
--- a/src/list.h
+++ b/src/list.h
@@ -5,6 +5,8 @@
 #ifndef V8_LIST_H_
 #define V8_LIST_H_
 
+#include <algorithm>
+
 #include "src/checks.h"
 #include "src/utils.h"
 
@@ -137,6 +139,9 @@ class List {
   // Drop the last 'count' elements from the list.
   INLINE(void RewindBy(int count)) { Rewind(length_ - count); }
 
+  // Swaps the contents of the two lists.
+  INLINE(void Swap(List<T, AllocationPolicy>* list));
+
   // Halve the capacity if fill level is less than a quarter.
   INLINE(void Trim(AllocationPolicy allocator = AllocationPolicy()));
 

diff --git a/test/cctest/cctest.cc b/test/cctest/cctest.cc
index b5771ff..35e7665 100644 (file)
--- a/test/cctest/cctest.cc
+++ b/test/cctest/cctest.cc
@@ -97,6 +97,7 @@ void CcTest::Run() {
   }
   callback_();
   if (initialize_) {
+    EmptyMessageQueues(isolate_);
     isolate_->Exit();
   }
 }

diff --git a/test/cctest/cctest.h b/test/cctest/cctest.h
index cc9edc8..ceb9f58 100644 (file)
--- a/test/cctest/cctest.h
+++ b/test/cctest/cctest.h
@@ -28,6 +28,7 @@
 #ifndef CCTEST_H_
 #define CCTEST_H_
 
+#include "include/libplatform/libplatform.h"
 #include "src/v8.h"
 
 #ifndef TEST
@@ -594,6 +595,13 @@ static inline void EnableDebugger() {
 static inline void DisableDebugger() { v8::Debug::SetDebugEventListener(NULL); }
 
 
+static inline void EmptyMessageQueues(v8::Isolate* isolate) {
+  while (v8::platform::PumpMessageLoop(v8::internal::V8::GetCurrentPlatform(),
+                                       isolate))
+    ;
+}
+
+
 // Helper class for new allocations tracking and checking.
 // To use checking of JS allocations tracking in a test,
 // just create an instance of this class.

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 6f5ac70..556b8ca 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -3275,6 +3275,7 @@ TEST(TwoPassPhantomCallbacks) {
   }
   CHECK_EQ(static_cast<int>(kLength), instance_counter);
   CcTest::heap()->CollectAllGarbage();
+  EmptyMessageQueues(isolate);
   CHECK_EQ(0, instance_counter);
 }
 
@@ -3293,6 +3294,7 @@ TEST(TwoPassPhantomCallbacksNestedGc) {
   array[15]->MarkTriggerGc();
   CHECK_EQ(static_cast<int>(kLength), instance_counter);
   CcTest::heap()->CollectAllGarbage();
+  EmptyMessageQueues(isolate);
   CHECK_EQ(0, instance_counter);
 }
 
@@ -3342,6 +3344,8 @@ class PhantomStdMapTraits : public v8::StdMapTraits<K, V> {
     CHECK_EQ(IntKeyToVoidPointer(key),
              v8::Object::GetAlignedPointerFromInternalField(value, 0));
   }
+  static void OnWeakCallback(
+      const v8::WeakCallbackInfo<WeakCallbackDataType>&) {}
   static void DisposeWeak(
       const v8::WeakCallbackInfo<WeakCallbackDataType>& info) {
     K key = KeyFromWeakCallbackInfo(info);
@@ -6806,6 +6810,7 @@ THREADED_TEST(GCFromWeakCallbacks) {
                             v8::WeakCallbackType::kParameter);
       object.handle.MarkIndependent();
       invoke_gc[outer_gc]();
+      EmptyMessageQueues(isolate);
       CHECK(object.flag);
     }
   }
@@ -11885,6 +11890,7 @@ THREADED_TEST(NoGlobalHandlesOrphaningDueToWeakCallback) {
   handle3.SetWeak(&handle3, HandleCreatingCallback1,
                   v8::WeakCallbackType::kParameter);
   CcTest::heap()->CollectAllGarbage();
+  EmptyMessageQueues(isolate);
 }
 
 

diff --git a/test/unittests/test-utils.cc b/test/unittests/test-utils.cc
index 522ee12..93a46ed 100644 (file)
--- a/test/unittests/test-utils.cc
+++ b/test/unittests/test-utils.cc
@@ -4,10 +4,12 @@
 
 #include "test/unittests/test-utils.h"
 
+#include "include/libplatform/libplatform.h"
 #include "src/base/platform/time.h"
 #include "src/debug.h"
 #include "src/flags.h"
 #include "src/isolate.h"
+#include "src/v8.h"
 
 namespace v8 {
 
@@ -51,6 +53,9 @@ void TestWithIsolate::SetUpTestCase() {
 // static
 void TestWithIsolate::TearDownTestCase() {
   ASSERT_TRUE(isolate_ != NULL);
+  v8::Platform* platform = internal::V8::GetCurrentPlatform();
+  ASSERT_TRUE(platform != NULL);
+  while (platform::PumpMessageLoop(platform, isolate_)) continue;
   isolate_->Dispose();
   isolate_ = NULL;
   delete array_buffer_allocator_;