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

Fix ReferenceStorage(T) atomic if T has no inner pointers #14845

Merged

Conversation

HertzDevil
Copy link
Contributor

It turns out the fix in #14730 made all ReferenceStorage objects non-atomic; Crystal::ReferenceStorageType#reference_type returns a reference type, whose #has_inner_pointers? always returns true since the reference itself is a pointer:

# CRYSTAL_TRACE=gc crystal run -Dtracing ...

class Foo
end

class Bar
  @x = ""
end

Pointer(ReferenceStorage(Foo)).malloc(1000)
Pointer(ReferenceStorage(Bar)).malloc(1000)
gc.malloc 992475497830875 thread=0x1f716cc00:? fiber=0x1026e6f00:main size=4000 duration=833
gc.malloc 992475497835083 thread=0x1f716cc00:? fiber=0x1026e6f00:main size=16000 duration=2458

This PR fixes that again: (note the presence of atomic=1 on the first line)

gc.malloc 992584569601208 thread=0x1f716cc00:? fiber=0x10259ef00:main size=4000 atomic=1 duration=708
gc.malloc 992584569609041 thread=0x1f716cc00:? fiber=0x10259ef00:main size=16000 duration=6042

@straight-shoota straight-shoota added this to the 1.14.0 milestone Jul 30, 2024
@straight-shoota straight-shoota changed the title Make ReferenceStorage(T) atomic if T has no pointer ivars Fix ReferenceStorage(T) atomic if T has no inner pointers Aug 6, 2024
@straight-shoota straight-shoota merged commit b06aad8 into crystal-lang:master Aug 6, 2024
62 checks passed
@HertzDevil HertzDevil deleted the bug/reference-storage-atomic2 branch August 13, 2024 12:52
@straight-shoota straight-shoota modified the milestones: 1.14.0, 1.13.2 Aug 15, 2024
straight-shoota pushed a commit that referenced this pull request Aug 20, 2024
It turns out the fix in #14730 made all `ReferenceStorage` objects non-atomic; `Crystal::ReferenceStorageType#reference_type` returns a reference type, whose `#has_inner_pointers?` always returns true since the reference itself is a pointer.
This PR fixes that again by adding a special case for `ReferenceStorage`.
@crysbot
Copy link

crysbot commented Aug 21, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/crystal-1-13-2-is-released/7110/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants