lib,src: remove post-gc event infrastructure
authorBen Noordhuis <info@bnoordhuis.nl>
Tue, 16 Dec 2014 12:21:41 +0000 (13:21 +0100)
committerBen Noordhuis <info@bnoordhuis.nl>
Thu, 18 Dec 2014 18:39:30 +0000 (19:39 +0100)
Remove the 'gc' event from the v8 module and remove the supporting
infrastructure from src/.  It gets the axe because:

1. There are currently no users.  It was originally conceived as
   an upstreamed subset of StrongLoop's strong-agent GC metrics,
   but the strong-agent code base has evolved considerably since
   that time and has no use anymore for what is in core.

2. The implementation is not quite sound.  It calls into JS land
   from inside the GC epilog and that is unsafe.  We could fix
   that by delaying the callback until a safe time but because
   there are no users anyway, removing it is all around easier.

PR-URL: https://github.com/iojs/io.js/pull/174
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
lib/v8.js
src/env-inl.h
src/env.h
src/node_v8.cc
test/parallel/test-v8-gc.js [deleted file]

index 53b3b00..d4e4c34 100644 (file)
--- a/lib/v8.js
+++ b/lib/v8.js
 
 'use strict';
 
-var EventEmitter = require('events');
 var v8binding = process.binding('v8');
-
-var v8 = module.exports = new EventEmitter();
-v8.getHeapStatistics = v8binding.getHeapStatistics;
-v8.setFlagsFromString = v8binding.setFlagsFromString;
-
-
-function emitGC(before, after) {
-  v8.emit('gc', before, after);
-}
-
-
-v8.on('newListener', function(name) {
-  if (name === 'gc' && EventEmitter.listenerCount(this, name) === 0) {
-    v8binding.startGarbageCollectionTracking(emitGC);
-  }
-});
-
-
-v8.on('removeListener', function(name) {
-  if (name === 'gc' && EventEmitter.listenerCount(this, name) === 0) {
-    v8binding.stopGarbageCollectionTracking();
-  }
-});
+exports.getHeapStatistics = v8binding.getHeapStatistics;
+exports.setFlagsFromString = v8binding.setFlagsFromString;
index 4ccf899..e38f5a8 100644 (file)
 
 namespace node {
 
-inline Environment::GCInfo::GCInfo()
-    : type_(static_cast<v8::GCType>(0)),
-      flags_(static_cast<v8::GCCallbackFlags>(0)),
-      timestamp_(0) {
-}
-
-inline Environment::GCInfo::GCInfo(v8::Isolate* isolate,
-                                   v8::GCType type,
-                                   v8::GCCallbackFlags flags,
-                                   uint64_t timestamp)
-    : type_(type),
-      flags_(flags),
-      timestamp_(timestamp) {
-  isolate->GetHeapStatistics(&stats_);
-}
-
-inline v8::GCType Environment::GCInfo::type() const {
-  return type_;
-}
-
-inline v8::GCCallbackFlags Environment::GCInfo::flags() const {
-  return flags_;
-}
-
-inline v8::HeapStatistics* Environment::GCInfo::stats() const {
-  // TODO(bnoordhuis) Const-ify once https://codereview.chromium.org/63693005
-  // lands and makes it way into a stable release.
-  return const_cast<v8::HeapStatistics*>(&stats_);
-}
-
-inline uint64_t Environment::GCInfo::timestamp() const {
-  return timestamp_;
-}
-
 inline Environment::IsolateData* Environment::IsolateData::Get(
     v8::Isolate* isolate) {
   return static_cast<IsolateData*>(isolate->GetData(kIsolateSlot));
@@ -99,9 +65,7 @@ inline Environment::IsolateData::IsolateData(v8::Isolate* isolate,
     PropertyName ## _(isolate, FIXED_ONE_BYTE_STRING(isolate, StringValue)),
     PER_ISOLATE_STRING_PROPERTIES(V)
 #undef V
-    ref_count_(0) {
-  QUEUE_INIT(&gc_tracker_queue_);
-}
+    ref_count_(0) {}
 
 inline uv_loop_t* Environment::IsolateData::event_loop() const {
   return event_loop_;
@@ -231,7 +195,6 @@ inline Environment::Environment(v8::Local<v8::Context> context,
   set_binding_cache_object(v8::Object::New(isolate()));
   set_module_load_list_array(v8::Array::New(isolate()));
   RB_INIT(&cares_task_list_);
-  QUEUE_INIT(&gc_tracker_queue_);
   QUEUE_INIT(&req_wrap_queue_);
   QUEUE_INIT(&handle_wrap_queue_);
   QUEUE_INIT(&handle_cleanup_queue_);
index c167041..76f7284 100644 (file)
--- a/src/env.h
+++ b/src/env.h
@@ -258,7 +258,6 @@ namespace node {
   V(context, v8::Context)                                                     \
   V(domain_array, v8::Array)                                                  \
   V(fs_stats_constructor_function, v8::Function)                              \
-  V(gc_info_callback_function, v8::Function)                                  \
   V(module_load_list_array, v8::Array)                                        \
   V(pipe_constructor_template, v8::FunctionTemplate)                          \
   V(process_object, v8::Object)                                               \
@@ -390,10 +389,6 @@ class Environment {
   inline void CleanupHandles();
   inline void Dispose();
 
-  // Defined in src/node_profiler.cc.
-  void StartGarbageCollectionTracking(v8::Local<v8::Function> callback);
-  void StopGarbageCollectionTracking();
-
   void AssignToContext(v8::Local<v8::Context> context);
 
   inline v8::Isolate* isolate() const;
@@ -494,13 +489,10 @@ class Environment {
  private:
   static const int kIsolateSlot = NODE_ISOLATE_SLOT;
 
-  class GCInfo;
   class IsolateData;
   inline Environment(v8::Local<v8::Context> context, uv_loop_t* loop);
   inline ~Environment();
   inline IsolateData* isolate_data() const;
-  void AfterGarbageCollectionCallback(const GCInfo* before,
-                                      const GCInfo* after);
 
   enum ContextEmbedderDataIndex {
     kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX
@@ -520,7 +512,6 @@ class Environment {
   ares_task_list cares_task_list_;
   bool using_smalloc_alloc_cb_;
   bool using_domains_;
-  QUEUE gc_tracker_queue_;
   bool printed_error_;
   debugger::Agent debugger_agent_;
 
@@ -536,26 +527,6 @@ class Environment {
   ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
 #undef V
 
-  class GCInfo {
-   public:
-    inline GCInfo();
-    inline GCInfo(v8::Isolate* isolate,
-                  v8::GCType type,
-                  v8::GCCallbackFlags flags,
-                  uint64_t timestamp);
-    inline v8::GCType type() const;
-    inline v8::GCCallbackFlags flags() const;
-    // TODO(bnoordhuis) Const-ify once https://codereview.chromium.org/63693005
-    // lands and makes it way into a stable release.
-    inline v8::HeapStatistics* stats() const;
-    inline uint64_t timestamp() const;
-   private:
-    v8::GCType type_;
-    v8::GCCallbackFlags flags_;
-    v8::HeapStatistics stats_;
-    uint64_t timestamp_;
-  };
-
   // Per-thread, reference-counted singleton.
   class IsolateData {
    public:
@@ -564,10 +535,6 @@ class Environment {
     inline void Put();
     inline uv_loop_t* event_loop() const;
 
-    // Defined in src/node_profiler.cc.
-    void StartGarbageCollectionTracking(Environment* env);
-    void StopGarbageCollectionTracking(Environment* env);
-
 #define V(PropertyName, StringValue)                                          \
     inline v8::Local<v8::String> PropertyName() const;
     PER_ISOLATE_STRING_PROPERTIES(V)
@@ -578,16 +545,6 @@ class Environment {
     inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop);
     inline v8::Isolate* isolate() const;
 
-    // Defined in src/node_profiler.cc.
-    static void BeforeGarbageCollection(v8::Isolate* isolate,
-                                        v8::GCType type,
-                                        v8::GCCallbackFlags flags);
-    static void AfterGarbageCollection(v8::Isolate* isolate,
-                                       v8::GCType type,
-                                       v8::GCCallbackFlags flags);
-    void BeforeGarbageCollection(v8::GCType type, v8::GCCallbackFlags flags);
-    void AfterGarbageCollection(v8::GCType type, v8::GCCallbackFlags flags);
-
     uv_loop_t* const event_loop_;
     v8::Isolate* const isolate_;
 
@@ -597,9 +554,6 @@ class Environment {
 #undef V
 
     unsigned int ref_count_;
-    QUEUE gc_tracker_queue_;
-    GCInfo gc_info_before_;
-    GCInfo gc_info_after_;
 
     DISALLOW_COPY_AND_ASSIGN(IsolateData);
   };
index fa60bbe..2a080f9 100644 (file)
@@ -31,153 +31,15 @@ namespace node {
 using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
-using v8::GCCallbackFlags;
-using v8::GCType;
 using v8::Handle;
-using v8::HandleScope;
 using v8::HeapStatistics;
 using v8::Isolate;
 using v8::Local;
-using v8::Null;
-using v8::Number;
 using v8::Object;
 using v8::String;
 using v8::Uint32;
 using v8::V8;
 using v8::Value;
-using v8::kGCTypeAll;
-using v8::kGCTypeMarkSweepCompact;
-using v8::kGCTypeScavenge;
-
-
-void Environment::IsolateData::BeforeGarbageCollection(Isolate* isolate,
-                                                       GCType type,
-                                                       GCCallbackFlags flags) {
-  Get(isolate)->BeforeGarbageCollection(type, flags);
-}
-
-
-void Environment::IsolateData::AfterGarbageCollection(Isolate* isolate,
-                                                      GCType type,
-                                                      GCCallbackFlags flags) {
-  Get(isolate)->AfterGarbageCollection(type, flags);
-}
-
-
-void Environment::IsolateData::BeforeGarbageCollection(GCType type,
-                                                       GCCallbackFlags flags) {
-  gc_info_before_ = GCInfo(isolate(), type, flags, uv_hrtime());
-}
-
-
-void Environment::IsolateData::AfterGarbageCollection(GCType type,
-                                                      GCCallbackFlags flags) {
-  gc_info_after_ = GCInfo(isolate(), type, flags, uv_hrtime());
-
-  // The copy upfront and the remove-then-insert is to avoid corrupting the
-  // list when the callback removes itself from it.  QUEUE_FOREACH() is unsafe
-  // when the list is mutated while being walked.
-  ASSERT(QUEUE_EMPTY(&gc_tracker_queue_) == false);
-  QUEUE queue;
-  QUEUE* q = QUEUE_HEAD(&gc_tracker_queue_);
-  QUEUE_SPLIT(&gc_tracker_queue_, q, &queue);
-  while (QUEUE_EMPTY(&queue) == false) {
-    q = QUEUE_HEAD(&queue);
-    QUEUE_REMOVE(q);
-    QUEUE_INSERT_TAIL(&gc_tracker_queue_, q);
-    Environment* env = ContainerOf(&Environment::gc_tracker_queue_, q);
-    env->AfterGarbageCollectionCallback(&gc_info_before_, &gc_info_after_);
-  }
-}
-
-
-void Environment::IsolateData::StartGarbageCollectionTracking(
-    Environment* env) {
-  if (QUEUE_EMPTY(&gc_tracker_queue_)) {
-    isolate()->AddGCPrologueCallback(BeforeGarbageCollection, v8::kGCTypeAll);
-    isolate()->AddGCEpilogueCallback(AfterGarbageCollection, v8::kGCTypeAll);
-  }
-  ASSERT(QUEUE_EMPTY(&env->gc_tracker_queue_) == true);
-  QUEUE_INSERT_TAIL(&gc_tracker_queue_, &env->gc_tracker_queue_);
-}
-
-
-void Environment::IsolateData::StopGarbageCollectionTracking(Environment* env) {
-  ASSERT(QUEUE_EMPTY(&env->gc_tracker_queue_) == false);
-  QUEUE_REMOVE(&env->gc_tracker_queue_);
-  QUEUE_INIT(&env->gc_tracker_queue_);
-  if (QUEUE_EMPTY(&gc_tracker_queue_)) {
-    isolate()->RemoveGCPrologueCallback(BeforeGarbageCollection);
-    isolate()->RemoveGCEpilogueCallback(AfterGarbageCollection);
-  }
-}
-
-
-// Considering a memory constrained environment, creating more objects is less
-// than ideal
-void Environment::AfterGarbageCollectionCallback(const GCInfo* before,
-                                                 const GCInfo* after) {
-  HandleScope handle_scope(isolate());
-  Context::Scope context_scope(context());
-  Local<Value> argv[] = { Object::New(isolate()), Object::New(isolate()) };
-  const GCInfo* infov[] = { before, after };
-  for (unsigned i = 0; i < ARRAY_SIZE(argv); i += 1) {
-    Local<Object> obj = argv[i].As<Object>();
-    const GCInfo* info = infov[i];
-    switch (info->type()) {
-      case kGCTypeScavenge:
-        obj->Set(type_string(), scavenge_string());
-        break;
-      case kGCTypeMarkSweepCompact:
-        obj->Set(type_string(), mark_sweep_compact_string());
-        break;
-      default:
-        UNREACHABLE();
-    }
-    obj->Set(flags_string(), Uint32::NewFromUnsigned(isolate(), info->flags()));
-    obj->Set(timestamp_string(), Number::New(isolate(), info->timestamp()));
-    // TODO(trevnorris): Setting many object properties in C++ is a significant
-    // performance hit. Redo this to pass the results to JS and create/set the
-    // properties there.
-#define V(name)                                                               \
-    do {                                                                      \
-      obj->Set(name ## _string(),                                             \
-               Uint32::NewFromUnsigned(isolate(), info->stats()->name()));    \
-    } while (0)
-    V(total_heap_size);
-    V(total_heap_size_executable);
-    V(total_physical_size);
-    V(used_heap_size);
-    V(heap_size_limit);
-#undef V
-  }
-  MakeCallback(this,
-               Null(isolate()),
-               gc_info_callback_function(),
-               ARRAY_SIZE(argv),
-               argv);
-}
-
-
-void Environment::StartGarbageCollectionTracking(Local<Function> callback) {
-  ASSERT(gc_info_callback_function().IsEmpty() == true);
-  set_gc_info_callback_function(callback);
-  isolate_data()->StartGarbageCollectionTracking(this);
-}
-
-
-void Environment::StopGarbageCollectionTracking() {
-  ASSERT(gc_info_callback_function().IsEmpty() == false);
-  isolate_data()->StopGarbageCollectionTracking(this);
-  set_gc_info_callback_function(Local<Function>());
-}
-
-
-void StartGarbageCollectionTracking(const FunctionCallbackInfo<Value>& args) {
-  CHECK(args[0]->IsFunction() == true);
-  Environment* env = Environment::GetCurrent(args);
-  env->StartGarbageCollectionTracking(args[0].As<Function>());
-}
 
 
 void GetHeapStatistics(const FunctionCallbackInfo<Value>& args) {
@@ -201,11 +63,6 @@ void GetHeapStatistics(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-void StopGarbageCollectionTracking(const FunctionCallbackInfo<Value>& args) {
-  Environment::GetCurrent(args)->StopGarbageCollectionTracking();
-}
-
-
 void SetFlagsFromString(const FunctionCallbackInfo<Value>& args) {
   String::Utf8Value flags(args[0]);
   V8::SetFlagsFromString(*flags, flags.length());
@@ -216,12 +73,6 @@ void InitializeV8Bindings(Handle<Object> target,
                           Handle<Value> unused,
                           Handle<Context> context) {
   Environment* env = Environment::GetCurrent(context);
-  env->SetMethod(target,
-                 "startGarbageCollectionTracking",
-                 StartGarbageCollectionTracking);
-  env->SetMethod(target,
-                 "stopGarbageCollectionTracking",
-                 StopGarbageCollectionTracking);
   env->SetMethod(target, "getHeapStatistics", GetHeapStatistics);
   env->SetMethod(target, "setFlagsFromString", SetFlagsFromString);
 }
diff --git a/test/parallel/test-v8-gc.js b/test/parallel/test-v8-gc.js
deleted file mode 100644 (file)
index 4bce809..0000000
+++ /dev/null
@@ -1,46 +0,0 @@
-// Copyright (c) 2014, StrongLoop Inc.
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
-// WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
-// MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
-// ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
-// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
-// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
-// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-
-// Flags: --expose_gc
-
-var common = require('../common');
-var assert = require('assert');
-var v8 = require('v8');
-
-assert(typeof gc === 'function', 'Run this test with --expose_gc.');
-
-var ncalls = 0;
-var before;
-var after;
-
-function ongc(before_, after_) {
-  // Try very hard to not create garbage because that could kick off another
-  // garbage collection cycle.
-  before = before_;
-  after = after_;
-  ncalls += 1;
-}
-
-gc();
-v8.on('gc', ongc);
-gc();
-v8.removeListener('gc', ongc);
-gc();
-
-assert.equal(ncalls, 1);
-assert.equal(typeof before, 'object');
-assert.equal(typeof after, 'object');
-assert.equal(typeof before.timestamp, 'number');
-assert.equal(typeof after.timestamp, 'number');
-assert.equal(before.timestamp <= after.timestamp, true);