Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid relocation of strings by moving the content to heap memory #811

Open
wants to merge 1 commit into
base: master
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Expand Up @@ -21,5 +21,6 @@ Gemfile.lock
types_log
*.gem
embed-test.rb.log
gc_compact.rb.log
spec/ffi/embed-test/ext/Makefile
spec/ffi/embed-test/ext/embed_test.bundle
22 changes: 21 additions & 1 deletion ext/ffi_c/Call.c
Expand Up @@ -91,6 +91,23 @@ static inline void* getPointer(VALUE value, int type);

static ID id_to_ptr, id_map_symbol, id_to_native;

static VALUE rbffi_pin_string_content(VALUE str)
{
#ifdef RSTRING_EMBED_LEN_MAX
if (likely(RB_TYPE_P(str, T_STRING)) && !unlikely(RB_OBJ_FROZEN(str))){
/* Expand the string capacity so we aren't using embedded string memory
* but heap memory for the string content.
* This is to avoid relocation of the content by GC.compact
*/
long expand_len = RSTRING_EMBED_LEN_MAX + 1 - RSTRING_LEN(str);
if (likely(expand_len > 0)) {
rb_str_modify_expand( str, expand_len );
}
}
#endif
return str;
}

void
rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
FFIStorage* paramStorage, void** ffiValues,
Expand Down Expand Up @@ -298,7 +315,9 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
param->ptr = NULL;

} else {
param->ptr = StringValueCStr(argv[argidx]);
VALUE str = argv[argidx];
rbffi_pin_string_content(str);
param->ptr = StringValueCStr(str);
}

ADJ(param, ADDRESS);
Expand Down Expand Up @@ -435,6 +454,7 @@ getPointer(VALUE value, int type)

} else if (type == T_STRING) {

rbffi_pin_string_content(value);
return StringValuePtr(value);

} else if (type == T_NIL) {
Expand Down
3 changes: 3 additions & 0 deletions spec/ffi/fixtures/StringTest.c
Expand Up @@ -32,3 +32,6 @@ string_null(void)
return NULL;
}

void store_string(char* str, char **ptr) {
*ptr = str;
}
44 changes: 44 additions & 0 deletions spec/ffi/gc_compact.rb
@@ -0,0 +1,44 @@
# -*- encoding: utf-8 -*-
#
# This file is part of ruby-ffi.
# For licensing, see LICENSE.SPECS
#

require File.expand_path(File.join(File.dirname(__FILE__), "spec_helper"))

# These specs are not run as regular rspec file, but as a separate process from string_spec.rb.
# That's due to GC.verify_compaction_references is very memory consuming since it doubles the needed memory with each call.
# So it's intentionally that the file name miss the "_spec" prefix.

if GC.respond_to?(:compact)
describe "GC.compact" do
module GcCompactTestLib
extend FFI::Library
ffi_lib TestLibrary::PATH
attach_function :store_string, [ :string, :pointer ], :void
attach_function :store_pointer, :store_string, [ :pointer, :pointer ], :void
end

A = []
F = []
[0, 1, 23, 24].each_with_index do |len, idx|
A[idx] = "a" * len
F[idx] = ("a" * len).freeze

it "preserves a #{len} character string" do
mem = FFI::MemoryPointer.new :pointer, 3
GcCompactTestLib.store_string(A[idx], mem)
GcCompactTestLib.store_pointer(A[idx], mem + 1*FFI::TYPE_POINTER.size)
GcCompactTestLib.store_string(F[idx], mem + 2*FFI::TYPE_POINTER.size)

GC.verify_compaction_references(toward: :empty, double_heap: true)

expect( mem.get_pointer(0*FFI::TYPE_POINTER.size).read_string ).to eq("a" * len)
expect( mem.get_pointer(1*FFI::TYPE_POINTER.size).read_string ).to eq("a" * len)
expect( mem.get_pointer(2*FFI::TYPE_POINTER.size).read_string ).to eq("a" * len)
expect( A[idx] ).to eq("a" * len)
expect( F[idx] ).to eq("a" * len)
end
end
end
end
13 changes: 13 additions & 0 deletions spec/ffi/string_spec.rb
Expand Up @@ -23,6 +23,11 @@ module StrLibTest
expect(StrLibTest.pointer_buffer_equals(str + "a", str + "b", str.bytesize + 1)).to eq(0)
end

it "Frozen String can be passed to :pointer and :string argument" do
str = "string buffer".freeze
expect(StrLibTest.pointer_buffer_equals(str, str, str.bytesize)).to eq(1)
end

it "Poison null byte raises error" do
s = "123\0abc"
expect{ StrLibTest.pointer_buffer_equals("", s, 0) }.to raise_error(ArgumentError)
Expand Down Expand Up @@ -98,4 +103,12 @@ module StrLibTest
ptrary.write_array_of_pointer(ary)
expect { ptrary.get_array_of_string(-1) }.to raise_error(IndexError)
end

it "with GC.compact" do
skip "GC.compact is unavailable" unless GC.respond_to?(:compact)

out = external_run("rspec", "gc_compact.rb")
expect(out).to match(/ 0 failures/)
end

end