Skip to content

Don't call hash tombstone compaction from GC compaction #13206

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

Merged
merged 1 commit into from
Apr 29, 2025

Conversation

tenderlove
Copy link
Member

Tombstone removal may possibly require allocation, and we're not allowed to allocate during GC. This commit also renames set_compact to set_gc_compact to differentiate tombstone removal compaction with GC object compaction.

Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but LGTM

Copy link

launchable-app bot commented Apr 29, 2025

All Tests passed!

✖️no tests failed ✔️61951 tests passed(4 flakes)

Tombstone removal may possibly require allocation, and we're not allowed
to allocate during GC.  This commit also renames `set_compact` to
`set_update_references` to differentiate tombstone removal compaction with GC
object compaction.

Co-Authored-By: Max Bernstein <max.bernstein@shopify.com>
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
@tenderlove tenderlove enabled auto-merge (rebase) April 29, 2025 19:57
end

x
GC.compact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah mmtk, doesn't have GC.compact:

    1) Failure:
  TC_Set#test_set_gc_compact_does_not_allocate [/Users/runner/work/ruby/ruby/src/test/ruby/test_set.rb:869]:
  pid 52362 exit 1
  | -:8:in '<main>': compact() function is unimplemented on this machine (NotImplementedError)
  | 	from -:8:in '<main>'
  .

Sorry for suggesting that change :/

@tenderlove tenderlove merged commit e6974be into ruby:master Apr 29, 2025
80 of 85 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants