Skip to content

Don't root named structs defined in Ruby#10144

Merged
byroot merged 1 commit intoruby:masterfrom
Shopify:struct-new-no-pin
Mar 1, 2024
Merged

Don't root named structs defined in Ruby#10144
byroot merged 1 commit intoruby:masterfrom
Shopify:struct-new-no-pin

Conversation

@casperisfine
Copy link
Copy Markdown
Contributor

@casperisfine casperisfine commented Feb 29, 2024

[Bug #20311]

rb_define_class_under assumes it's called from C and that the reference might be held in a C global variable, so it adds the class to the VM root.

In the case of Struct.new('Name') it's wasteful and make the struct immortal.

@launchable-app
Copy link
Copy Markdown

launchable-app bot commented Feb 29, 2024

Launchable Report

❌ Test session #2670538 failedos:macos-arm-oss test_opts:--repeat-count:2 test_task:test-all
🔔 no issues ✖️3 tests failed ✔️49669 tests passed

❌ Test session #2670601 failedos:ubuntu-20.04 test_opts: test_task:check
🔔 no issues ✖️1 test failed ✔️23785 tests passed

❌ Test session #2670633 failedos:macos-12 test_opts: test_task:check
🔔 no issues ✖️1 test failed ✔️24672 tests passed

Passed test sessions

✅ Test session #2670528 passed os:macos-arm-oss test_opts: test_task:check
✅ Test session #2670567 passed os:ubuntu-20.04 test_opts:--enable-shared--enable-load-relative test_task:check
✅ Test session #2670579 passed os:ubuntu-20.04 test_opts:--disable-yjit test_task:check
✅ Test session #2670587 passed os:ubuntu-20.04 test_opts:cppflags:-DVM_CHECK_MODE test_task:check
✅ Test session #2670626 passed os:macos-13 test_opts: test_task:check

Build: refs_pull_10144_merge_091e4d445778f424ec2bd29d0440f45560e96135

@byroot byroot requested a review from nobu February 29, 2024 12:19
@casperisfine casperisfine force-pushed the struct-new-no-pin branch 2 times, most recently from aff1481 to 34fc7b0 Compare February 29, 2024 15:54
@casperisfine casperisfine changed the title Don't pin named structs Don't root named structs defined in Ruby Feb 29, 2024
[Bug #20311]

`rb_define_class_under` assumes it's called from C and that the
reference might be held in a C global variable, so it adds the
class to the VM root.

In the case of `Struct.new('Name')` it's wasteful and make
the struct immortal.
@byroot byroot merged commit e626da8 into ruby:master Mar 1, 2024
@casperisfine casperisfine deleted the struct-new-no-pin branch March 14, 2024 19:14
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.

2 participants