Make Refs compare equal when their contents are equal

[ararslan]

Currently Ref(a) == Ref(b) is always false, even when a == b. This changes the behavior such that Ref(a) == Ref(b) if a == b, and likewise for isequal and isapprox.

Fixes #31813.

[yuyichao]

Why is this a minor change if this is a user visible behavior change that wasn't an error before?

[ararslan]

Seems unlikely to me that anyone has been relying on Refs never comparing equal, but I could well be wrong.

[yuyichao]

Compare, maybe not, but not that unlikely for Dict/Set/unique. Which also make me realize that you didn't implement hash.

[ararslan]

but not that unlikely for Dict/Set/unique.

That's a good point, I hadn't thought of that.

Which also make me realize that you didn't implement hash.

Good call, I can do that.

[yuyichao]

(And other than the technical aspect of the behavioral change being breaking (and the easily fixable missing hash), I don't have preference either way..........)

[ararslan]

What's the most sensible way to implement hashing for Refs? I have

const hash_ref_seed = UInt === UInt64 ? 0x039511594a926cf3 : 0x56cdecdd
hash(r::RefValue, h::UInt) = hash(hash_ref_seed ⊻ hash(r.x), h)

locally but I want to make sure that's sensible before pushing.

[chethega]

I agree with yuyichao: Ref currently currently compare and hashes like a mutable struct, and you propose that it should behave like a container / Array. Both are reasonabe semantics with lots of precedent in Base, but changing between them is definitely breaking.

FWIW, I prefer the current behavior. Mutation semantics (out-parameters, shared state, etc) is the entire point of ever using a Ref, and comparison should reflect that.

[ararslan]

That's a very fair assessment. I'm fine closing this if the consensus is to keep things how they are.

[StefanKarpinski]

What's the most sensible way to implement hashing for Refs?

Based on the current equality behavior, it should be identity hashing.

[chethega]

FWIW, we could speed up by hash2(x::Base.RefValue, h=zero(UInt))=hash(reinterpret(UInt,pointer_from_objref(x)), h).

In general, the generic fallback hash(@nospecialize(x), h::UInt) = hash_uint(3h - objectid(x)) is pretty slow, because of the expensive objectid.

If we wanted more speed, we could use pointer_from_objref for mutable types and have a faster @generated for other types.

For bitstype (very important special case!), we should hash the object as a binary blob, after zeroing structure padding. This should be faster because the speed of hashing would scale in sizeof(T) and not in the number of fields. To wit:

julia> struct foo1
       x::UInt64
       end

julia> struct foo2
       x1::UInt8
       x2::UInt8
       x3::UInt8
       x4::UInt8
       x5::UInt8
       x6::UInt8
       x7::UInt8
       x8::UInt8
       end
julia> x=foo1(UInt(1)); y=foo2(ntuple(i->UInt8(i), 8)...);
julia> @btime hash($x);
  42.081 ns (1 allocation: 16 bytes)
julia> @btime hash($y);
  134.079 ns (1 allocation: 16 bytes)

Imo the easiest way of getting there would be to change what we emit for Ref: We could ensure that setindex!(x::Base.RefValue, args...) and creation of Ref always zeros structure padding. I think this would only induce something between no and very modest runtime overhead. If we did that, then fast generic hashing of bitstypes would be trivial: We would simply R=Ref(x); ptr = reinterpret(Ptr{UInt8}, pointer_from_objref(R)) and then hash the binary blob, hopefully with an @generated implementation that emits simd instructions when available. Example (using above types):

julia> function hash_n(x)
       r=Ref(x)
       ptr = reinterpret(Ptr{UInt64}, pointer_from_objref(r))
       u=unsafe_load(ptr)
       return Base.hash_uint(u+typeof(u).name.hash)
       end
julia> @btime hash_n($x);
  3.951 ns (0 allocations: 0 bytes)

julia> @btime hash_n($y);
  3.961 ns (0 allocations: 0 bytes)

[yuyichao]

pointer_from_objref is not guaranteed to return the same result every time. It merely guarantees that the content you write and read from that address reflects (in) the contents of the object if done within the lifetime of that objects. Local stack optimization can easily achieve this guarantee without a stable address.

[chethega]

pointer_from_objref is not guaranteed to return the same result every time.

Ok, I'm intrigued: What are situations where this could be violated?

I get that pointer_from_objref can give a pointer on the stack. But can there be a case where x is stack allocated, its pointer_from_objref is taken and incorporated into some calculation, and then x is allocated a second time on the heap and the reference escapes? I thought we only stack allocate if we can prove that no reference escapes.

For example: Suppose x is mutable, and I take ptr_flip=~reinterpret(UInt, pointer_from_objref(x)), and later read from reinterpret(Ptr{some_typ}, ~ptr_flip), within the lifetime of x. Do we make guarantees that this is valid?

Afaiu Base.hash_uint is bijective on UInt64, so the pointer is recoverable, just slightly more complicated than flipping its bits.

[yuyichao]

For example: Suppose x is mutable, and I take ptr_flip=~reinterpret(UInt, pointer_from_objref(x)), and later read from reinterpret(Ptr{some_typ}, ~ptr_flip), within the lifetime of x. Do we make guarantees that this is valid?

If the two are in the same GC.@preserve then definitely. Otherwise, not necessarily. The compiler could proof that the content within the object is unused and therefore the object is dead in between the two usage.

[yuyichao]

But can there be a case where x is stack allocated, its pointer_from_objref is taken and incorporated into some calculation, and then x is allocated a second time on the heap and the reference escapes?

I don't think the compiler is currently doing this (though with combination of store to load forwarding to remove dead uses it COULD, just not from a single pass) but this is certainly allowed.

[yuyichao]

Also, AFAICT, #31885 (comment) and your reply to it is OT for this PR.
And to continue on the OT, object_id is only slow because it's not specialized and IIRC it blocks allocation elimination, the latter being exactly why what you want won't work. The correct way to fix this is to optimize object_id in codegen (or inference) like we do for many other functions and be particularly careful to not let object lifetime calculation make the return value unstable.

[JeffBezanson]

From triage: we noted that Ptr is a subtype of Ref, and it should certainly compare only the address. We also find the argument in #31885 (comment) pretty convincing. So we don't need to make this change.