Move object_id in attributes
#13159
Move object_id in attributes
#13159
Conversation
✅ All Tests passed!✖️no tests failed ✔️62066 tests passed(1 flake) |
1aae814 to
d8c231b
Compare
gc/default/default.c
Outdated
| } | ||
|
|
||
| if (rb_shape_has_object_id(shape)) { | ||
| // We could avoid locking if the object isn't shareable |
There was a problem hiding this comment.
For T_MODULE and T_CLASS it might be worth finding 8B in rb_classext_t so we can do some atomic CAS and find_or_create the object_id with no actual locking.
But if we need to also set the FL_SEEN_OBJ_ID flag, it might be more tricky than that?
f1b501e to
63e2d8d
Compare
af67723 to
8ff0247
Compare
1cd4964 to
0efa5fb
Compare
b58815c to
6adbaaf
Compare
| // We could avoid locking if the object isn't shareable | ||
| // but we'll lock anyway to lookup the next shape, and | ||
| // we'd at least need to generate the object_id using atomics. | ||
| lock_lev = rb_gc_vm_lock(); |
There was a problem hiding this comment.
Might be best to do in a followup, to it's easier to review, but we're not yet reducing contention at all here.
As for looking up the next shape, we can now do it without locking in the single-child happy path: #13191.
So combined with an atomic increment of next_object_id we could in many case not lock even on the initial write.
| #define OBJ_ID_INCREMENT (RUBY_IMMEDIATE_MASK + 1) | ||
| #define OBJ_ID_INITIAL (OBJ_ID_INCREMENT) | ||
|
|
||
| static unsigned long long next_object_id = OBJ_ID_INITIAL; | ||
| static VALUE id_to_obj_value = 0; | ||
| static st_table *id_to_obj_tbl = NULL; |
There was a problem hiding this comment.
Perhaps we should move object_id related logic in its own file? Might be followup worthy.
gc.c
Outdated
| if (FL_TEST(obj, FL_EXIVAR)) { | ||
| rb_free_generic_ivar((VALUE)obj); | ||
| FL_UNSET(obj, FL_EXIVAR); | ||
| } |
There was a problem hiding this comment.
I think it would be very easy to get rid of FL_EXIVAR, and replace it with something like:
!RB_TYPE_ANY(obj, T_OBJECT, T_CLASS, T_MODULE) && get_shape(obj)->next_field_index > 0It adds one extra pointer chase, but it could be optimized out because generic objects with no ivars will always have either the ROOT shape or the ROOT_FROZEN shape. So it could be a very quick operation.
7f53212 to
2c256dc
Compare
| bool | ||
| rb_shape_too_complex_p(rb_shape_t *shape) | ||
| { | ||
| return shape->flags & SHAPE_FL_TOO_COMPLEX; |
There was a problem hiding this comment.
We might want to turn these small helpers into static inline in shape.h for performance reason. But I'd rather do it in a followup as keeping all this logic private helps with refactoring for now.
7f8d390 to
dc039da
Compare
dc039da to
1f95d2a
Compare
Ivars will longer be the only thing stored inline via shapes, so keeping the `iv_index` and `ivptr` names would be confusing. Instance variables won't be the only thing stored inline via shapes, so keeping the `ivptr` name would be confusing. `field` encompass anything that can be stored in a VALUE array. Similarly, `gen_ivtbl` becomes `gen_fields_tbl`.
Also refactor checks for `->type == SHAPE_OBJ_TOO_COMPLEX`.
This opens the door to store more informations in shapes, such as the `object_id` or object address in case it has been observed and the object has to be moved.
4df8c0e to
f2534e3
Compare
And get rid of the `obj_to_id_tbl` It's no longer needed, the `object_id` is now stored inline in the object alongside instance variables. We still need the inverse table in case `_id2ref` is invoked, but we lazily build it by walking the heap if that happens. The `object_id` concern is also no longer a GC implementation concern, but a generic implementation. Co-Authored-By: Matt Valentine-House <matt@eightbitraptor.com>
f2534e3 to
3d9a88a
Compare
Use `st_foreach_with_replace` rather than to call `st_insert` from inside `st_foreach`, this saves from having to disable GC. Co-Authored-By: Peter Zhu <peter@peterzhu.ca>
| attr_index_t capacity; // Total capacity of the object with this shape | ||
| uint8_t type; | ||
| uint8_t heap_index; | ||
| uint8_t flags; | ||
| shape_id_t parent_id; | ||
| redblack_node_t *ancestor_index; | ||
| }; |
There was a problem hiding this comment.
What is the size of rb_shape now? We might want to move flags to the end of the struct because I think this will leave wasted space between flags and parent_id.
On master it's 40 bytes:
(lldb) p sizeof(struct rb_shape)
(unsigned long) 40
I'm not sure how much we care about shape memory usage, but maybe we could split the type field. We might only need 4 bits for type.
There was a problem hiding this comment.
It's unchanged, I've put the flags here because there was some padding:
diff --git a/shape.c b/shape.c
index fb739d3614..8aec985496 100644
--- a/shape.c
+++ b/shape.c
@@ -1434,6 +1434,7 @@ Init_default_shapes(void)
void
Init_shape(void)
{
+ fprintf(stderr, "sizeof(rb_shape_t) = %lu\n", sizeof(rb_shape_t));
#if SHAPE_DEBUG
/* Document-class: RubyVM::Shape
* :nodoc: */$ make -j run
sizeof(rb_shape_t) = 40
I'm not sure how much we care about shape memory usage
Not that much, but I think we could save some spaces by not using a full pointer for redblack_node_t *ancestor_index;. We could just have an offset. I think It would be nice if shapes were 32B instead of 40B.
From: ruby/ruby#13159 Co-Authored-By: Jean Boussier <jean.boussier@gmail.com>
Introduced in: ruby#13159 Now that there is no longer a unique TOO_COMPLEX shape with no children, checking `shape->type == TOO_COMPLEX` is incorrect.
Introduced in: #13159 Now that there is no longer a unique TOO_COMPLEX shape with no children, checking `shape->type == TOO_COMPLEX` is incorrect.
**What does this PR do?** This PR fixes three issues in the profiler when used with latest ruby-head: 1. It's no longer possible to ask the object_id from a T_IMEMO object. This showed up as a Ruby VM crash with an error message "T_IMEMO can't have an object_id". (See ruby/ruby#13347 for the upstream change) 2. Creating new instances of a class is now inlined into the caller, and there is no longer a frame to represent the new. This broke some of our tests that expected the stack from allocating an object to have the `new` at the top. (See ruby/ruby#13080 for the upstream change) 3. Object ids now count towards the size of objects. This broke some of our tests that asserted on size of profiled objects. (See ruby/ruby#13159 for the upstream change) **Motivation:** Fix support for Ruby 3.5. **Additional Notes:** N/A **How to test the change?** I've updated our specs to cover these changes. Unfortunately, we don't yet test with Ruby 3.5 in CI, so you'll have to test manually if you want to see the fixes working with 3.5. (Note that these changes showed up after 3.5.0-preview1, so testing on -preview1 is not enough)
And get rid of the
obj_to_id_tblIt's no longer needed, the
object_idis now stored inlinein the object.
We still need the inverse table in case
_id2refis invoked, butwe lazily build it by walking the heap if that happens.