Move `object_id` in attributes by casperisfine · Pull Request #13159 · ruby/ruby

casperisfine · 2025-04-23T14:00:51Z

And get rid of the obj_to_id_tbl

It's no longer needed, the object_id is now stored inline
in the object.

We still need the inverse table in case _id2ref is invoked, but
we lazily build it by walking the heap if that happens.

cloudbees-smart-tests · 2025-04-23T14:11:41Z

✅ All Tests passed!

✖️no tests failed ✔️62066 tests passed(1 flake)

casperisfine · 2025-04-24T10:03:05Z

+    }
+
+    if (rb_shape_has_object_id(shape)) {
+        // We could avoid locking if the object isn't shareable


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?

byroot · 2025-05-05T07:48:59Z

+    // 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();


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.

byroot · 2025-05-05T07:51:03Z

+#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;


Perhaps we should move object_id related logic in its own file? Might be followup worthy.

byroot · 2025-05-05T07:56:28Z

+    if (FL_TEST(obj, FL_EXIVAR)) {
+        rb_free_generic_ivar((VALUE)obj);
+        FL_UNSET(obj, FL_EXIVAR);
+    }


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 > 0

It 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.

byroot · 2025-05-05T11:42:38Z

+bool
+rb_shape_too_complex_p(rb_shape_t *shape)
+{
+    return shape->flags & SHAPE_FL_TOO_COMPLEX;


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.

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.

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 <[email protected]>

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 <[email protected]>

tenderlove

This is fine with me. I think we might have an opportunity to decrease the size of rb_shape, but I'm fine if we merge this as-is.

tenderlove · 2025-05-07T16:40:45Z

+    uint8_t flags;
    shape_id_t parent_id;
    redblack_node_t *ancestor_index;
 };


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.

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.

peterzhu2118

Makes sense to me!

From: ruby/ruby#13159 Co-Authored-By: Jean Boussier <[email protected]>

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)

casperisfine force-pushed the object_id-in-shape branch 3 times, most recently from 1aae814 to d8c231b Compare April 24, 2025 09:31

casperisfine commented Apr 24, 2025

View reviewed changes

casperisfine force-pushed the object_id-in-shape branch 3 times, most recently from f1b501e to 63e2d8d Compare April 25, 2025 11:56

casperisfine commented Apr 29, 2025

View reviewed changes

Comment thread gc.c Outdated

casperisfine force-pushed the object_id-in-shape branch 3 times, most recently from af67723 to 8ff0247 Compare April 30, 2025 15:13

eightbitraptor force-pushed the object_id-in-shape branch from 1cd4964 to 0efa5fb Compare May 1, 2025 18:54

casperisfine force-pushed the object_id-in-shape branch 7 times, most recently from b58815c to 6adbaaf Compare May 4, 2025 11:24

byroot reviewed May 5, 2025

View reviewed changes

casperisfine force-pushed the object_id-in-shape branch 4 times, most recently from 7f53212 to 2c256dc Compare May 5, 2025 10:43

byroot reviewed May 5, 2025

View reviewed changes

casperisfine force-pushed the object_id-in-shape branch 4 times, most recently from 7f8d390 to dc039da Compare May 5, 2025 13:59

casperisfine commented May 5, 2025

View reviewed changes

Comment thread variable.c Outdated

casperisfine force-pushed the object_id-in-shape branch from dc039da to 1f95d2a Compare May 5, 2025 14:37

byroot added 3 commits May 6, 2025 14:39

Refactor OBJ_TOO_COMPLEX_SHAPE_ID to not be referenced outside shape.h

e6d3ed3

Also refactor checks for `->type == SHAPE_OBJ_TOO_COMPLEX`.

shape.c: refactor frozen shape to no longer be final

0bd9057

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.

casperisfine force-pushed the object_id-in-shape branch 9 times, most recently from 4df8c0e to f2534e3 Compare May 7, 2025 14:36

casperisfine force-pushed the object_id-in-shape branch from f2534e3 to 3d9a88a Compare May 7, 2025 14:50

peterzhu2118 reviewed May 7, 2025

View reviewed changes

Comment thread gc.c Outdated

Comment thread gc.c

byroot and others added 2 commits May 7, 2025 17:56

Get rid of RB_GC_VM_ID_TO_OBJ_TABLE_KEYS

4cb0e57

Refactor id_to_obj_tbl compaction

30c33d4

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 <[email protected]>

tenderlove approved these changes May 7, 2025

View reviewed changes

peterzhu2118 approved these changes May 7, 2025

View reviewed changes

byroot merged commit cf9046c into ruby:master May 8, 2025

peterzhu2118 added a commit to ruby/mmtk that referenced this pull request May 8, 2025

Sync "Move object_id in attributes"

6d19ead

From: ruby/ruby#13159 Co-Authored-By: Jean Boussier <[email protected]>

peterzhu2118 mentioned this pull request May 8, 2025

Sync "Move object_id in attributes" ruby/mmtk#31

Merged

byroot added a commit to byroot/ruby that referenced this pull request May 8, 2025

Fix remove_instance_variable on complex objects

19afee5

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.

byroot mentioned this pull request May 8, 2025

Fix remove_instance_variable on complex objects #13280

Merged

byroot added a commit that referenced this pull request May 8, 2025

Fix remove_instance_variable on complex objects

b67711b

Introduced in: #13159 Now that there is no longer a unique TOO_COMPLEX shape with no children, checking `shape->type == TOO_COMPLEX` is incorrect.

ivoanjo mentioned this pull request May 16, 2025

[PROF-11809] Fix profiler compatibility with ruby-head (3.5) DataDog/dd-trace-rb#4656

Merged

Uh oh!

Conversation

casperisfine commented Apr 23, 2025

Uh oh!

cloudbees-smart-tests Bot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Tests passed!

Uh oh!

casperisfine Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

byroot May 5, 2025

Choose a reason for hiding this comment

Uh oh!

byroot May 5, 2025

Choose a reason for hiding this comment

Uh oh!

byroot May 5, 2025

Choose a reason for hiding this comment

Uh oh!

byroot May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tenderlove left a comment

Choose a reason for hiding this comment

Uh oh!

tenderlove May 7, 2025

Choose a reason for hiding this comment

Uh oh!

casperisfine May 7, 2025

Choose a reason for hiding this comment

Uh oh!

peterzhu2118 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cloudbees-smart-tests Bot commented Apr 23, 2025 •

edited

Loading