jacksondunstan / UnityNativeScripting

Unity Scripting in C++
https://jacksondunstan.com/articles/3938
MIT License
1.33k stars 135 forks source link

[#51] Fix performance issue with Object Store #52

Closed AustinSmith13 closed 4 years ago

AustinSmith13 commented 4 years ago

When BaseMaxSimultaneous was set to a large number the object-store was spending to much time finding the handle. This was fixed using a dictionary where the keys have the objects full hash.

AustinSmith13 commented 4 years ago

After looking at this in the morning I realized I'm keying with hashcodes which is not guaranteed to be unique.

jacksondunstan commented 4 years ago

@AustinSmith13 let me know if you fix the uniqueness problem and I'll review. Thanks for the PR! 👍

AustinSmith13 commented 4 years ago

@jacksondunstan After a little bit of research object.GetHashCode() returns a unique index assigned to the object by the CLR when the object is created and released from garbage collection.

So I don't think its likely to have a hash-code duplicate in the same app-domain but maybe after collection which I don't think is a problem.

I did implement a version with hash collision handling but it less nice looking and is probably slower. (but not by much)

I am thinking about keeping it the way it is, but microsoft advises against it here https://docs.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=netframework-4.8

public static int GetHandle(object obj)
{
    // Null is always zero
    if (object.ReferenceEquals(obj, null))
    {
        return 0;
    }

    lock (objects)
    {
        // Look up the handle in the hash table
        uint hash = (uint)obj.GetHashCode();
        List<int> handleBucket = null;

        if (handleBucketByHash.TryGetValue(hash, out handleBucket))
        {
            for (int i = 0; i < handleBucket.Count; i++)
            {
                int handleInBucket = handleBucket[i];
                object objectInBucket = objects[handleInBucket];

                if (object.ReferenceEquals(objectInBucket, obj))
                {
                    return handleInBucket;
                }
            }
        }
    }

    // Object not found
    return Store(obj);
}

I've tried to cause hash collisions by creating and destroying thousands of objects per frame. No collisions.

If you are fine with whats there I would like to keep it the way it is without any collision handling for slight performance benefit.

AustinSmith13 commented 4 years ago

Sorry about constantly changing this I can't help but try to make things perfect. I realize a better way of doing this would be to use a single dictionary Dictionary<object, int>. Dictionaries already consider hash collisions for there keys. I'll have this change ready for review later today.

AustinSmith13 commented 4 years ago

@jacksondunstan This is ready for review.

After looking into how dictionaries work, I realized they already do everything I was previously trying to do. Internally they call GetHashCode and check equality for collisions.