Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Implement Write Barriers #11793

Conversation

casperisfine
Copy link
Contributor

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.

But the *Descriptor classes and Arena have very few references and are only set in a few places, so it's relatively easy to implement.

cc @peterzhu2118

@casperisfine casperisfine requested a review from a team as a code owner February 3, 2023 13:38
@casperisfine casperisfine requested review from JasonLunn and removed request for a team February 3, 2023 13:38
@shaod2 shaod2 requested a review from haberman February 6, 2023 19:21
@shaod2 shaod2 added the ruby label Feb 6, 2023
@casperisfine
Copy link
Contributor Author

I just noticed the "Message" struct was also from protobuf, and easy to add write barriers for.

For context, here's we have a couple thousands of these instances in our heap that need to be mark on every minor GC:

 ["DATA(Google::Protobuf::Internal::Arena)", 1268],
 ["DATA(Message)", 1268],
...
 ["DATA(Google::Protobuf::EnumDescriptor)", 77],
 ["DATA(Google::Protobuf::FileDescriptor)", 68],

@casperisfine
Copy link
Contributor Author

@JasonLunn @haberman sorry for the ping, but is this on your radar? We have about 3.5k of these objects on our heap, and we'd love if we didn't have to mark them on every minor GC.

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 8, 2023
@haberman
Copy link
Member

haberman commented Feb 8, 2023

It looks like there is an assertion failure on Ruby 2.7:

....ruby: ruby/ext/google/protobuf_c/protobuf.c:404: void ObjectCache_Add(const void *, VALUE): Assertion `ObjectCache_Get(key) == val' failed.
Aborted (core dumped)

Also could you rebase the PR from main? There have been some fixes to the tests in the meantime.

@haberman
Copy link
Member

haberman commented Feb 8, 2023

It would be helpful to add comments above the struct members that must only be changed through RB_OBJ_WRITE(), to caution future readers/writers of this invariant.

@casperisfine
Copy link
Contributor Author

@haberman done. I rebased and added a comment above each reference to WB_PROTECTED.

@mkruskal-google mkruskal-google added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Feb 14, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 14, 2023
@haberman
Copy link
Member

Also could you rebase once more (sorry for the trouble, you caught us in the middle of a migration to GitHub Actions).

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.

But the `*Descriptor` classes and `Arena` have very few references
and are only set in a few places, so it's relatively easy to implement.
@casperisfine
Copy link
Contributor Author

@haberman done

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 14, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 14, 2023
@casperisfine
Copy link
Contributor Author

Sorry to be a bother, but is there any other blockers I can help with?

@mkruskal-google mkruskal-google added copybara-kick 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed copybara-kick labels Feb 22, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 22, 2023
@haberman
Copy link
Member

This was merged in d82d8a4

@casperisfine
Copy link
Contributor Author

🙌

Thank you!

@casperisfine casperisfine mentioned this pull request Apr 3, 2023
40 tasks
casperisfine pushed a commit to casperisfine/protobuf that referenced this pull request May 23, 2023
copybara-service bot pushed a commit that referenced this pull request May 25, 2023
Followup: #11793

Closes #12883

COPYBARA_INTEGRATE_REVIEW=#12883 from casperisfine:ruby-missing-write-barrier 990e4fd
FUTURE_COPYBARA_INTEGRATE_REVIEW=#12883 from casperisfine:ruby-missing-write-barrier 990e4fd
PiperOrigin-RevId: 535234582
copybara-service bot pushed a commit that referenced this pull request May 25, 2023
Followup: #11793

Closes #12883

COPYBARA_INTEGRATE_REVIEW=#12883 from casperisfine:ruby-missing-write-barrier 990e4fd
PiperOrigin-RevId: 535281041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants