#[Bug] HashCode.Combine returns different values for Burst and non-Burst code

1 messages · Page 1 of 1 (latest)

sharp siren
cedar pagoda
#

Thanks, we're looking into it

#

Have you written up a bug?

sharp siren
# cedar pagoda Have you written up a bug?

I'm not the original bug catcher so I didn't. However I've already posted a reproducible code in that thread by the name Laicasaane. Do you want a bug report for formal bug fixing process?

cedar pagoda
#

If you don't mind

sharp siren
wooden lotus
#

it was reported back in July IN-106860

#

I was under the assumption that it was (weirdly) intended considering the burst documentation states

Do note that Burst does not follow .NET's implementation

#

Which tbh I feel like means it should have never been implemented in the first place

frail linden
#

that's a huge footgun since it's not really possible to know in a large codebase whether a function gets called from burst, mono, or both, and it can completely silently break things like passing a NativeHashMap from managed code to a job and back

#

if it can't be implemented the same then imo it should just fail to compile with burst entirely

cedar pagoda
#

Followed up in thread, it is intended, it's unlikely to change and the fix is going to be a documentation fix

#

if you need a deterministic hash then it needs to be explicit

frail linden
#

welp time to ban usage project-wide via CI then

#

is there no hope for just turning it into a hard error on burst compile?

#

it doesn't seem like it'd be even possible to use it "correctly" because something as innocent as setting a breakpoint while attached to the editor will seemingly change behavior, as well as burst hot loading after async compilation (besides the already mentioned common usecase of passing a hashmap between managed and burst code)

cedar pagoda
#

is there no hope for just turning it into a hard error on burst compile?

Maybe? Sounds like a breaking API change. Maybe we can roll out a way to taint thinks via a configuration file or something

wooden lotus
wooden lotus
#

i should add, the main reason i think this is terrible behaviour is even if you have all your code burst compiled, if you hook up a debugger to a job suddenly that one job is no longer burst compiled

#

and it breaks everything

#

hooking up a debugger should never just break your app, especially without any notice

#

this is a mistake on the level of, overriding null comparison for gameobjects

cedar pagoda
#

It is very unfortunate to have that sort of hidden behavior although it occurs in more than a couple of places with burst v non-burst code. I do agree it should be clearer that this is an API doesn't have identical semantics

wooden lotus
#

to me that's irrelevant
it's not acceptable to allow a piece of safe code to break the ability to use a debugger
i've been thinking since the time this post came up for a case where a user would ever use HashCode.Combine and i can not think of one - it is never safe to use, therefore it should not be usable

sharp siren
unreal olive
#

Oh wow I wonder how many bugs we have that we haven't been able to explain that's caused by this. It's even more awkward since Burst is background compiled. For our game it takes about 2-3mins to compile our biggest assembly. So I suppose after burst is enabled for that assembly it's likely a lot of stuff that relies on the hashcode being stable for the session just breaks for "no reason"

#

Is there any immediate replacement we can use? I suppose we just have to do manual hashing?

wooden lotus
#

but async compilation does the opposite, randomly turns it on at different times

#

imagine using the same hashmap in 2 different jobs that finish compiling at different times

raven tapir
#

Even a simple use case such as:

  • populating a hashmap from a burst job
  • reading from the hashmap outside of burst
    seems very reasonable to me, yet it will fail in very unexpected ways if hashes are different
grizzled grove
#

+1 This is really bad, this makes hashcode.combine unusable as other people have said

grizzled grove
cedar pagoda
#

there's also potential for floating point inconsistencies due first to mono idiosyncracies and autovectorization, but I don't have an enumerated list there

rugged oasis
#

Wow, really? Glad that I avoid that then. I usually just generate GetHashCode() using Rider and ticking off "Use HashCode.Combine()". I'm always under the assumption that anything non blittable API under System is not Burst compatible.

raven tapir
#

Also, when we will get the new features of C#, I hope the record structs auto-generated GetHashCode will not suffer from the same problem 👀

sharp siren
#

The bright side is that you can always provide your own GetHashCode for records.

#

Or any generated operation.

raven tapir
#

I was looking forward to having them auto-generated and not clutter the code with boilerplate to be fair 😅

sharp siren
#

You can source-generate them instead of letting the compiler do that.

raven tapir
#

yeah, that could definitely be a nice solution

sharp siren
#

For me, I hope Unity Burst can provide these features out of box, just like the generator for Unity.Properties.

frail linden
#

necroing this thread because i just ran into this again, and what's even worse is that fixing the GetHashCode() implementation to remove HashCode.Combine failed to trigger a burst compile and left me with a poisoned Burst Cache that still has the broken version

#

if i didn't recall this being a problem it would be absolute hell to debug

#

i really really really really want a burst compiler error for HashCode.Combine

#

there's no reasonable way for a code analyzer to detect what code burst is going to compile, so the diagnostic really has to be in the burst compiler itself

frail linden
sharp siren
#

Since my last comment I've spent time migrating all types I authored away from HashCode. Since I envisioned that the need for dictionary would only increase.

frail linden
#

yea this is something that slipped through in autogenerated boilerplate