From d969b1cdb9204cd89184a7a3b9b6b3be5a7d6ad6 Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Tue, 28 Jul 2015 17:57:01 +0300 Subject: [PATCH 1/2] add back the basics of memory testing. This adds back the memory tests which ensure The Ruby Racer's stability. It also includes the changes needed to make these initial specs pass. At this point to disallow calling Dispose() directly on the underlying Isolate. If you do, then you could have all of the Ruby objects happily sitting in memory, but trying to use them would result in a segfault for trying to use a dead isolate. In order to avoid that, we'd have to put in safeguards everywhere to make sure for any operation on any context or object, that it's isolate was still alive. In order to reduce the complexity of memory management, the safest thing is to let the garbage collection hook dispose of the isolate itself. That way there is no doubt that the isolate is no longer in use. --- .travis.yml | 1 + ext/v8/isolate.cc | 10 +++++----- ext/v8/isolate.h | 13 +++++++++++-- spec/mem/blunt_spec.rb | 23 +++++++++++++---------- 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/.travis.yml b/.travis.yml index e0a9086..1bbff0b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,6 +24,7 @@ script: - bundle exec rake compile - bundle exec rspec spec/c - bundle exec rspec spec/v8/context_spec.rb + - bundle exec rspec spec/mem sudo: false addons: apt: diff --git a/ext/v8/isolate.cc b/ext/v8/isolate.cc index e9e001b..a185128 100644 --- a/ext/v8/isolate.cc +++ b/ext/v8/isolate.cc @@ -11,7 +11,7 @@ namespace rr { ClassBuilder("Isolate"). defineSingletonMethod("New", &New). - defineMethod("Dispose", &Isolate::Dispose). + defineMethod("IdleNotificationDeadline", &IdleNotificationDeadline). store(&Class); } @@ -26,7 +26,6 @@ namespace rr { create_params.array_buffer_allocator = &data->array_buffer_allocator; v8::Isolate* isolate = v8::Isolate::New(create_params); - isolate->SetData(0, data); isolate->AddGCPrologueCallback(&clearReferences); @@ -34,9 +33,10 @@ namespace rr { return Isolate(isolate); } - VALUE Isolate::Dispose(VALUE self) { + + VALUE Isolate::IdleNotificationDeadline(VALUE self, VALUE deadline_in_seconds) { Isolate isolate(self); - isolate->Dispose(); - return Qnil; + Locker lock(isolate); + return Bool(isolate->IdleNotificationDeadline(NUM2DBL(deadline_in_seconds))); } } diff --git a/ext/v8/isolate.h b/ext/v8/isolate.h index 7fc1480..59d2782 100644 --- a/ext/v8/isolate.h +++ b/ext/v8/isolate.h @@ -50,6 +50,7 @@ namespace rr { */ static void destroy(IsolateData* data) { Isolate isolate(data); + isolate->Dispose(); isolate.decrementTotalReferences(); } @@ -159,7 +160,15 @@ namespace rr { while (data->rb_release_queue.try_dequeue(object)) { isolate.releaseObject(object); } - rb_gc_mark(data->retained_objects); + //TODO: This should not be necessary since sometimes the + //instance of V8::RetainedObjects appears to magically be of + //type T_NONE instead of T_OBJECT. Later, it will be T_OBJECT, + //but if called while T_NONE, it will cause rb_gc_mark to dump + //core. + //See https://bugs.ruby-lang.org/issues/10803 + if (TYPE(data->retained_objects) != T_NONE) { + rb_gc_mark(data->retained_objects); + } } /** @@ -177,7 +186,7 @@ namespace rr { } - static VALUE Dispose(VALUE self); + static VALUE IdleNotificationDeadline(VALUE self, VALUE deadline_in_seconds); /** * Recent versions of V8 will segfault unless you pass in an diff --git a/spec/mem/blunt_spec.rb b/spec/mem/blunt_spec.rb index 7c5a058..a3f346e 100644 --- a/spec/mem/blunt_spec.rb +++ b/spec/mem/blunt_spec.rb @@ -7,7 +7,7 @@ describe "A Very blunt test to make sure that we aren't doing stupid leaks", :me end #allocate a single context to make sure that v8 loads its snapshot and #we pay the overhead. - V8::Context.new + V8::Isolate.new @start_memory = process_memory GC.stress = true end @@ -15,23 +15,27 @@ describe "A Very blunt test to make sure that we aren't doing stupid leaks", :me after do GC.stress = false end + + it "can create 3 isolates in a row" do + 3.times { V8::Isolate.new } + end + it "won't increase process memory by more than 50% no matter how many contexts we create" do - 500.times do - V8::Context.new - run_v8_gc + 250.times do + isolate = V8::Context.new.isolate.native + isolate.IdleNotificationDeadline(0.1) end - process_memory.should <= @start_memory * 1.5 + expect(process_memory).to be <= @start_memory * 1.5 end it "can eval simple value passing statements repeatedly without significantly increasing memory" do - V8::C::Locker() do - cxt = V8::Context.new + V8::Context.new do |cxt| 500.times do cxt.eval('7 * 6') - run_v8_gc + cxt.isolate.native.IdleNotificationDeadline(0.1) end end - process_memory.should <= @start_memory * 1.1 + expect(process_memory).to be <= @start_memory * 1.1 end def process_memory @@ -39,4 +43,3 @@ describe "A Very blunt test to make sure that we aren't doing stupid leaks", :me end end - From 5e21f1deaeacf1d00afb304954f41173e12b671e Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Tue, 28 Jul 2015 18:29:38 +0300 Subject: [PATCH 2/2] remove calls to Dispose in specs --- spec/c/isolate_spec.rb | 4 ---- spec/c_spec_helper.rb | 2 -- spec/spec_helper.rb | 6 ------ 3 files changed, 12 deletions(-) diff --git a/spec/c/isolate_spec.rb b/spec/c/isolate_spec.rb index 593c1b7..61656dc 100644 --- a/spec/c/isolate_spec.rb +++ b/spec/c/isolate_spec.rb @@ -6,8 +6,4 @@ describe V8::C::Isolate do it 'can create a new isolate' do expect(isolate).to be end - - it "can be disposed of" do - isolate.Dispose() - end end diff --git a/spec/c_spec_helper.rb b/spec/c_spec_helper.rb index c57f12c..c0116ad 100644 --- a/spec/c_spec_helper.rb +++ b/spec/c_spec_helper.rb @@ -23,8 +23,6 @@ module V8ContextHelpers @ctx.Exit end end - ensure - @isolate.Dispose() end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 41785c8..b31ecb4 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,11 +1,5 @@ require 'v8' -def run_v8_gc - V8::C::V8::LowMemoryNotification() - while !V8::C::V8::IdleNotification() do - end -end - def rputs(msg) puts "
#{ERB::Util.h(msg)}
" $stdout.flush