Implement rb_get_alloc_func and spec related functions #1874
Conversation
| BASIC_ALLOC = ::BasicObject.singleton_class.instance_method(:__allocate__) | ||
|
|
||
| def rb_newobj_of(ruby_class) | ||
| # we need to bypass __allocate__ on ruby_class |
There was a problem hiding this comment.
Why do we need to bypass? Could you add a spec failing without that?
There was a problem hiding this comment.
Is it that rb_newobj_of should ignore the alloc_func, or it should ignore the __allocate__ for core classes, or something else?
There was a problem hiding this comment.
Why do we need to bypass? Could you add a spec failing without that?
Ah right, because speced_allocator is a alloc_func and it calls rb_newobj_of.
If we just ruby_class.send :__allocate__ then it's a stack overflow.
And I guess rb_newobj_of is supposed to work inside an alloc_func?
Writing some thoughts here, not sure what's best yet.
What we need would be to ignore the __allocate__ defined by rb_define_alloc_func and call the original one. This is needed if we want it to work for e.g., rb_newobj_of(String).
Maybe a way to achieve that is to check for the alloc_func in BasicObject#allocate, which I guess is what MRI does.
Not sure how to handle inheritance though, could be by propagating an alloc_func field in ClassLayout, much like instanceFactory.
Alternatively, we could introduce a separate method for the alloc_func to have something like:
BasicObject#allocate calls __alloc_func__ which by default calls __allocate__.
But the extra indirection seems potentially costly.
Maybe the simplest and clearest is simply defining core __allocate__ also as __dynamic_object_factory__ (because it exposes the DynamicObjectFactory, which is defined per Layout, to Ruby essentially) or another name (__layout_allocate__, __internal_allocate__, __dynamic_object_allocator__?), and then make rb_newobj_of calls __dynamic_object_factory__. Then alloc_func can still override __allocate__ but not touch __dynamic_object_factory__.
In MRI, rb_newobj_of seems to basically just return a zeroed RBasic, but in our case we need to use the correct DynamicObjectFactory based for the Layouts.
There was a problem hiding this comment.
I updated the comment, I think the __dynamic_object_factory__ approach seems the way to go.
| @@ -345,6 +345,31 @@ static VALUE object_spec_rb_class_inherited_p(VALUE self, VALUE mod, VALUE arg) | |||
| return rb_class_inherited_p(mod, arg); | |||
| } | |||
|
|
|||
| static VALUE speced_allocator(VALUE klass) { | |||
| VALUE instance = rb_newobj_of(klass, 0); | |||
| rb_funcall(instance, rb_intern("instance_variable_set"), 2, ID2SYM(rb_intern("@from_custom_allocator")), Qtrue); | |||
There was a problem hiding this comment.
Small thing, there is rb_iv_set() to do this more directly.
| @@ -1278,17 +1279,40 @@ def rb_eval_string(str) | |||
| eval(str) | |||
| end | |||
|
|
|||
| BASIC_ALLOC = ::BasicObject.singleton_class.instance_method(:__allocate__) | |||
There was a problem hiding this comment.
There is already BASIC_OBJECT_ALLOCATE a bit below
|
I pushed my work on top of this PR to master...eregon:rb-get-alloc-func if you want to have a look. |
|
Can you add a change log entry if this is already in CI? |
|
Yes I added one (see the diff above) |
These's some lose ends in this PR that need tidying up but can I get a general opinion on it?
Shopify#1