Skip to content

Implement Write Barrier and dsize for FFI::FunctionType#1001

Merged
larskanis merged 1 commit into
ffi:masterfrom
casperisfine:function-type-write-barrier
Mar 6, 2023
Merged

Implement Write Barrier and dsize for FFI::FunctionType#1001
larskanis merged 1 commit into
ffi:masterfrom
casperisfine:function-type-write-barrier

Conversation

@casperisfine

@casperisfine casperisfine commented Mar 6, 2023

Copy link
Copy Markdown

Ref: #991

Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC.

The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected.

This commit also implement a dsize function so that these instance report a more relevant size in various memory profilers.

Comment thread ext/ffi_c/FunctionInfo.c
Comment on lines +66 to +67
// IMPORTANT: WB_PROTECTED objects must only use the RB_OBJ_WRITE()
// macro to update VALUE references, as to trigger write barriers.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was asked to add this comment when I added write barriers in the protobuf gem, and I think it's a good idea as it might remind future contributors.

Comment thread ext/ffi_c/FunctionInfo.c
}

static size_t
fntype_memsize(const void *data)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think I accounted for all held data, but worst case these function don't have to be 100% accurate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A FunctionInfo can have a closurePool associated for all closures that are created in relation to this FunctionInfo. I'll add at least the management data of the ClosurePool to the size.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sounds good, I'll have a look at it.

We canalso report the size of the pool divided by the number of refs to it, I've done that in msgpack-ruby.


function_type = FFI::FunctionType.new(:int, [])
size = ObjectSpace.memsize_of(function_type)
expect(size).to be > base_size

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The size can vary on platforms etc, so I usually use this pattern to ensure the function report something, but not assert a precise size.

@casperisfine
casperisfine force-pushed the function-type-write-barrier branch 2 times, most recently from ddee7c8 to 617472b Compare March 6, 2023 09:32
Comment thread spec/ffi/function_type_spec.rb Outdated

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

describe FFI::FunctionType, skip: RUBY_PLATFORM == "java" do

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The java ext doesn't define this class, I suppose that's why it wasn't tested. But I think these specs are worth it for the MRI implementation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, FunctionInfo is just an internal class not exposed through the API.

Write barrier protected objects are allowed to be promoted to the old generation,
which means they only get marked on major GC.

The downside is that the RB_BJ_WRITE macro MUST be used to set references,
otherwise the referenced object may be garbaged collected.

This commit also implement a `dsize` function so that these instance
report a more relevant size in various memory profilers.
@casperisfine
casperisfine force-pushed the function-type-write-barrier branch from 617472b to fae39d8 Compare March 6, 2023 09:50
@casperisfine

Copy link
Copy Markdown
Author

Ok, it's now 🟢 , moving on to the next one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants