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

Allow passing non heap objects to WeakRef #4293

Merged
merged 2 commits into from
Apr 19, 2017

Conversation

jhass
Copy link
Member

@jhass jhass commented Apr 14, 2017

This should allow at least pointers to the data section to work seamlessly with WeakRef and in some cases even allow stack pointers to be passed, for the crazy. This should increase the usability of WeakRef since it allows passing static data for testing or other purposes.

This also renames WeakRef#target to WeakRef#get WeakRef#value. get seems to be the more general, more widely used and shorter name. value is consistent with Pointer. Additionally target feels like it's leaking implementation details as
that's the name of the internal instance variable too.

As expected this is slightly slower for the heap case, given the additional check, and slightly faster for the non-heap case, given the check is less expensive than registering a pointer with GC. A quick benchmark with collection disabled so the second testcase wouldn't crash:

            WeakRef heap  14.42M ( 69.36ns) (±11.41%)       fastest
        WeakRef constant  10.55M (  94.8ns) (±45.52%)  1.37× slower
    CheckingWeakRef heap   9.14M ( 109.4ns) (±45.29%)  1.58× slower
CheckingWeakRef constant  11.48M ( 87.07ns) (±83.27%)  1.26× slower

This should allow at least pointers to the data section
to work seamlessly with WeakRef and in some cases even
allow stack pointers to be passed, for the crazy
@bcardiff
Copy link
Member

Java use #get. C# use #target.

I think I picked from C# back then.

I could agree that changing to #value since it would match the Pointer(T) method. But I feel that #get is too generic.

@jhass
Copy link
Member Author

jhass commented Apr 14, 2017

I could live with value, what do others think?

@RX14
Copy link
Contributor

RX14 commented Apr 14, 2017

#value to be consistent with Pointer looks good to me.

value is consistent with pointer

Additionally target feels like it's leaking implementation details as
that's the name of the internal instance variable too
@jhass jhass force-pushed the feature/non_heap_weak_ref branch from 4fe3a04 to 61e6349 Compare April 14, 2017 16:42
@asterite
Copy link
Member

I like #value too :-)

@asterite
Copy link
Member

@jhass Nice! I always test WeakRef with WeakRef.new("foo") and see it crash and then remember "oh, it's not in the heap". It's something ugly that should probably not crash at all, so this PR is very welcome! :-)

@bcardiff bcardiff added this to the Next milestone Apr 19, 2017
@bcardiff bcardiff merged commit f40bcb3 into crystal-lang:master Apr 19, 2017
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