Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

ensure idenity map entries are cleaned up: #384

Open
wants to merge 1 commit into
base: upgrade-to-v8-4.5
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions ext/v8/null.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,20 @@
namespace rr {
class Null : public Ref<v8::Value> {
public:
Null(v8::Isolate* isolate, v8::Local<v8::Value> undefined) :
Ref<v8::Value>(isolate, undefined) {}

static inline void Init() {
ClassBuilder("Null", Primitive::Class).
store(&Class);

//implement V8::C::Null(isolate)
rb_define_singleton_method(rb_eval_string("V8::C"), "Null", (VALUE (*)(...))&instance, 1);
}
Null(v8::Isolate* isolate, v8::Local<v8::Value> undefined) :
Ref<v8::Value>(isolate, undefined) {}

static VALUE instance(VALUE self, VALUE rb_isolate) {
Isolate isolate(rb_isolate);
Locker lock(isolate);
return Null(isolate, v8::Null(isolate));
}
};
}
Expand Down
6 changes: 6 additions & 0 deletions lib/v8/conversion.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ def to_v8(context)
end
end

class NilClass
def to_v8(context)
V8::C::Null(context.isolate.native)
end
end

##
# The following are the default conversions from Ruby objects into
# low-level C++ objects.
Expand Down
11 changes: 9 additions & 2 deletions lib/v8/weak.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,14 @@ def [](key)
end

def []=(key, value)
@values[key] = V8::Weak::Ref.new(value)
V8::Weak::Ref.new(value).tap do |ref|
@values[key] = ref
ObjectSpace.define_finalizer(value, self.class.cleanup_entry(@values, key, ref))
end
end

def self.cleanup_entry(values, key, ref)
proc { values.delete(key) if values[key] == ref }
end
end
end
Expand Down Expand Up @@ -70,4 +77,4 @@ def populate(block)
end
end
end
end
end
52 changes: 52 additions & 0 deletions spec/mem/identity_mapping_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
require 'spec_helper'

describe "Identity Mapping Memory Usages" do
it "does not leak memory by virtue of maintaining an identity map" do
##
# This verifies that the identity map which is holds on to the
# mappings between v8 objects and ruby objects does not leak its
# entries as originally reported by this ticket:
# https://github.com/cowboyd/therubyracer/pull/336
#
# Without the patch, while the v8 object was not being leaked, the
# entry in the identity map was. This test injects 5000 objects
# into the v8 context 20 times over, which will generate 5000
# unique proxies. All of these proxies should be immediately released.

require 'memory_profiler'

def rss
`ps -eo pid,rss | grep #{Process.pid} | awk '{print $2}'`.to_i
end

cxt = V8::Context.new
Object.new.tap do |o|
cxt["object-#{o.object_id}"] = o
cxt["object-#{o.object_id}"] = nil
end

# MemoryProfiler has a helper that runs the GC multiple times to make
# sure all objects that can be freed are freed.
MemoryProfiler::Helpers.full_gc

start = rss
#puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

20.times do

5000.times { |i|
Object.new.tap do |o|
cxt["object-#{o.object_id}"] = o
cxt["object-#{o.object_id}"] = nil
end
while cxt.isolate.native.IdleNotificationDeadline(0.1); end
}
MemoryProfiler::Helpers.full_gc
#puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

end
finish = rss

expect(finish).to be <= start * 2.5
end if RUBY_VERSION >= "2.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Wouldn't it be nicer if we just put the example in a pending state with a message that it should only be run only newer versions of ruby?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably so. I'll fix that.

end
4 changes: 4 additions & 0 deletions spec/v8/context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@
@cxt = V8::Context.new
# @cxt['o'] = @instance
end
it "can embed nil into a context" do
@cxt['nil'] = nil
expect(@cxt['nil']).to be_nil
end

it "can embed a closure into a context and call it" do
@cxt["say"] = lambda { |*args| args[-2] * args[-1] }
Expand Down
2 changes: 2 additions & 0 deletions therubyracer.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@ Gem::Specification.new do |gem|

gem.add_dependency 'ref'
gem.add_dependency 'libv8', '~> 4.5.95.0'

gem.add_development_dependency 'memory_profiler'
end