Skip to content

Commit

Permalink
Avoid relocation of strings by moving the content to heap memory
Browse files Browse the repository at this point in the history
This is due to the fact that ruby-2.7+ relocates emdedded strings by GC.compact .
See https://bugs.ruby-lang.org/issues/17023 for more description of the issue.

Unfortunately there's no good way to pin string values to a fixed memory location in the ruby C API.
The only way seems to be rb_gc_register_address(), but this function would keep a reference to the string object forever, so that it will never be freed.

To work around this issue the string capacity is expanded, so that the string content is moved from embedded string memory to ordinary heap memory.
The underlying string buffer of such strings is never relocated at GC.compact .

There are some issues:

1. String expanding fails with frozen strings, but frozen strings are relocated as well.

2. All strings smaller than 24 characters are copied when passed to the C function.
   This increases call time by up to 70% although this copying isn't required in almost all cases.
   It's only needed when GC.compact is used and the string in question is accessed after GC.compact.

3. The string object is modified in a way that changes it's RSTRING_PTR().
   That isn't visible in ruby, but could be noticed in conjunction with other C extensions.
  • Loading branch information
larskanis committed Jul 21, 2020
1 parent faae322 commit d301e55
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 1 deletion.
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

0 comments on commit d301e55

Please sign in to comment.