From dab6f681cd8c43351aa56f4deb2e327c8e4c5cfe Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 16 Dec 2014 13:21:41 +0100 Subject: [PATCH] lib,src: remove post-gc event infrastructure 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 --- lib/v8.js | 26 +------ src/env-inl.h | 39 +--------- src/env.h | 46 ----------- src/node_v8.cc | 149 ------------------------------------ test/parallel/test-v8-gc.js | 46 ----------- 5 files changed, 3 insertions(+), 303 deletions(-) delete mode 100644 test/parallel/test-v8-gc.js diff --git a/lib/v8.js b/lib/v8.js index 53b3b0094c89d7..d4e4c3450b3389 100644 --- a/lib/v8.js +++ b/lib/v8.js @@ -14,28 +14,6 @@ '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; diff --git a/src/env-inl.h b/src/env-inl.h index 4ccf899a46e141..e38f5a8d166031 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -34,40 +34,6 @@ namespace node { -inline Environment::GCInfo::GCInfo() - : type_(static_cast(0)), - flags_(static_cast(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(&stats_); -} - -inline uint64_t Environment::GCInfo::timestamp() const { - return timestamp_; -} - inline Environment::IsolateData* Environment::IsolateData::Get( v8::Isolate* isolate) { return static_cast(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 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_); diff --git a/src/env.h b/src/env.h index c167041e25cec6..76f7284f1edfd5 100644 --- 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 callback); - void StopGarbageCollectionTracking(); - void AssignToContext(v8::Local 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 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 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); }; diff --git a/src/node_v8.cc b/src/node_v8.cc index fa60bbee4b3dd1..2a080f98c3e141 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -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 argv[] = { Object::New(isolate()), Object::New(isolate()) }; - const GCInfo* infov[] = { before, after }; - for (unsigned i = 0; i < ARRAY_SIZE(argv); i += 1) { - Local obj = argv[i].As(); - 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 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()); -} - - -void StartGarbageCollectionTracking(const FunctionCallbackInfo& args) { - CHECK(args[0]->IsFunction() == true); - Environment* env = Environment::GetCurrent(args); - env->StartGarbageCollectionTracking(args[0].As()); -} void GetHeapStatistics(const FunctionCallbackInfo& args) { @@ -201,11 +63,6 @@ void GetHeapStatistics(const FunctionCallbackInfo& args) { } -void StopGarbageCollectionTracking(const FunctionCallbackInfo& args) { - Environment::GetCurrent(args)->StopGarbageCollectionTracking(); -} - - void SetFlagsFromString(const FunctionCallbackInfo& args) { String::Utf8Value flags(args[0]); V8::SetFlagsFromString(*flags, flags.length()); @@ -216,12 +73,6 @@ void InitializeV8Bindings(Handle target, Handle unused, Handle 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 index 4bce8099d8b6c6..00000000000000 --- a/test/parallel/test-v8-gc.js +++ /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);