Prevent ResourceHandle ValidityBit Collisions over Time

Author: raquelpeces

Packages/com.unity.render-pipelines.core/Runtime/RenderGraph/RenderGraphResources.cs

@@ -20,23 +20,28 @@ internal enum RenderGraphResourceType
         // Note on handles validity.
         // PassData classes used during render graph passes are pooled and because of that, when users don't fill them completely,
         // they can contain stale handles from a previous render graph execution that could still be considered valid if we only checked the index.
-        // In order to avoid using those, we incorporate the execution index in a 16 bits hash to make sure the handle is coming from the current execution.
+        // In order to avoid using those, we incorporate the execution index in a hash to make sure the handle is coming from the current execution.
         // If not, it's considered invalid.
-        // We store this validity mask in the upper 16 bits of the index.
-        const uint kValidityMask = 0xFFFF0000;
+        // The validity is stored in a separate field, allowing for a full 32-bit validity check. (2^32 =~ 4B (at 60fps it is more than 2 years) 4,294,967,295 unique execution indices before a collision)
+        // The m_VersionIndex field contains both index and version:
+        // - Index (lower 16 bits = 65,535 unique indices)
+        // - Version (upper 16 bits with one for not versioned. 15 bits = 32767 unique versions).
+        //   - Bit 31 is used as a "not versioned" flag for faster checking.
         const uint kIndexMask = 0xFFFF;
+        const uint kVersionMask = 0x7FFF0000;
+        const int kVersionShift = 16;
+        const uint kNotVersionedBit = 0x80000000;
 
-        private readonly uint m_Value;
-        private readonly int m_Version;
+        private readonly uint m_VersionIndex;
+        private readonly uint m_Validity;
         private readonly RenderGraphResourceType m_Type;
 
-        static uint s_CurrentValidBit = 1 << 16;
-        static uint s_SharedResourceValidBit = 0x7FFF << 16;
+        static uint s_CurrentValidBit = 1;
 
         public int index
-        { 
+        {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get { return (int)(m_Value & kIndexMask); }
+            get { return (int)(m_VersionIndex & kIndexMask); }
         }
         public int iType
         {
@@ -46,34 +51,38 @@ public int iType
         public int version
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get { return m_Version; }
+            get
+            {
+                return (m_VersionIndex & kNotVersionedBit) != 0 ? -1 : (int)((m_VersionIndex & kVersionMask) >> kVersionShift);
+            }
         }
         public RenderGraphResourceType type
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
             get { return m_Type; }
         }
 
-        internal ResourceHandle(int value, RenderGraphResourceType type, bool shared)
+        internal ResourceHandle(int index, RenderGraphResourceType type, bool shared)
         {
-            Debug.Assert(value <= 0xFFFF);
-            m_Value = ((uint)value & kIndexMask) | (shared ? s_SharedResourceValidBit : s_CurrentValidBit);
+            Debug.Assert(index > 0 && index <= 0xFFFF, "ResourceHandle: Invalid index, values should be >0 && <65536");
+            m_VersionIndex = ((uint)index & kIndexMask) | kNotVersionedBit;
+            m_Validity = s_CurrentValidBit;
             m_Type = type;
-            m_Version = -1;
         }
 
         internal ResourceHandle(in ResourceHandle h, int version)
         {
-            this.m_Value = h.m_Value;
-            this.m_Type = h.type;
-            this.m_Version = version;
+            Debug.Assert(version >= 0 && version <= 0x7FFF, "ResourceHandle: Invalid version, values should be >=0 && <32768");
+            uint versionBits = ((uint)version << kVersionShift) & kVersionMask;
+            m_VersionIndex = (h.m_VersionIndex & kIndexMask) | versionBits;
+            m_Validity = h.m_Validity;
+            m_Type = h.type;
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public bool IsValid()
         {
-            var validity = m_Value & kValidityMask;
-            return validity != 0 && (validity == s_CurrentValidBit || validity == s_SharedResourceValidBit);
+            return m_Validity != 0 && (m_Validity == s_CurrentValidBit);
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
@@ -82,8 +91,8 @@ public bool IsNull()
             if (index == 0)
             {
                 // Make sure everything is zero
-                Debug.Assert(m_Value == 0);
-                Debug.Assert(m_Version == 0);
+                Debug.Assert(m_VersionIndex == 0);
+                Debug.Assert(m_Validity == 0);
                 return true;
             }
             return false;
@@ -92,20 +101,20 @@ public bool IsNull()
         static public void NewFrame(int executionIndex)
         {
             uint previousValidBit = s_CurrentValidBit;
-            // Scramble frame count to avoid collision when wrapping around.
-            s_CurrentValidBit = (uint)(((executionIndex >> 16) ^ (executionIndex & 0xffff) * 58546883) << 16);
+
+            var hasher = HashFNV1A32.Create();
+            hasher.Append(executionIndex);
+            s_CurrentValidBit = (uint)hasher.value;
             // In case the current valid bit is 0, even though perfectly valid, 0 represents an invalid handle, hence we'll
             // trigger an invalid state incorrectly. To account for this, we actually skip 0 as a viable s_CurrentValidBit and
             // start from 1 again.
-            // In the same spirit, s_SharedResourceValidBit is reserved for shared textures so we should never use it otherwise
-            // resources could be considered valid at frame N+1 (because shared) even though they aren't.
-            if (s_CurrentValidBit == 0 || s_CurrentValidBit == s_SharedResourceValidBit)
+            if (s_CurrentValidBit == 0)
             {
                 // We need to make sure we don't pick the same value twice.
                 uint value = 1;
-                while (previousValidBit == (value << 16))
+                while (previousValidBit == value)
                     value++;
-                s_CurrentValidBit = (value << 16);
+                s_CurrentValidBit = value;
             }
         }
 
@@ -114,14 +123,14 @@ public bool IsVersioned
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
             get
             {
-                return m_Version >= 0;
+                return (m_VersionIndex & kNotVersionedBit) == 0;
             }
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public bool Equals(ResourceHandle hdl)
         {
-            return hdl.m_Value == this.m_Value && hdl.m_Version == this.m_Version && hdl.type == this.type;
+            return hdl.m_VersionIndex == this.m_VersionIndex && hdl.m_Validity == this.m_Validity && hdl.type == this.type;
         }
 
         public static bool operator ==(ResourceHandle lhs, ResourceHandle rhs) => lhs.Equals(rhs);
@@ -133,8 +142,8 @@ public bool Equals(ResourceHandle hdl)
         public override int GetHashCode()
         {
             var hashCode = HashFNV1A32.Create();
-            hashCode.Append(m_Value);
-            hashCode.Append(m_Version);
+            hashCode.Append(m_VersionIndex);
+            hashCode.Append(m_Validity);
             hashCode.Append(m_Type);
             return hashCode.value;
         }

Packages/com.unity.render-pipelines.core/Tests/Editor/RenderGraphTests.cs

@@ -1414,62 +1414,142 @@ public void RenderGraphMultisampledShaderResolvePassMustHaveOneColorAttachment()
             m_Camera.Render();
         }
 
-        /*
-        // Disabled for now as version management is not exposed to user code
+
         [Test]
-        public void VersionManagement()
+        public void ResourceHandleVersionManagement()
         {
+            TextureHandle texture0 = m_RenderGraph.CreateTexture(new TextureDesc(Vector2.one) { colorFormat = GraphicsFormat.R8G8B8A8_UNorm });
 
-            TextureHandle texture0;
-            TextureHandle texture0v1;
-            TextureHandle texture0v2;
-            texture0 = m_RenderGraph.CreateTexture(new TextureDesc(Vector2.one) { colorFormat = GraphicsFormat.R8G8B8A8_UNorm });
+            // Handles are unversioned by default.
+            Assert.IsFalse(texture0.handle.IsVersioned);
+            Assert.AreEqual(-1, texture0.handle.version);
 
-            TextureHandle texture1;
-            texture1 = m_RenderGraph.CreateTexture(new TextureDesc(Vector2.one) { colorFormat = GraphicsFormat.R8G8B8A8_UNorm });
+            int iterations = 1000;
+            for (int i = 0; i < iterations; ++i)
+            {
+                int writePassIndex = m_RenderGraph.m_RenderPasses.Count;
+                // Writing bumps version
+                using (var builder = m_RenderGraph.AddRasterRenderPass<RenderGraphTestPassData>("Async_TestPassWrite"+i, out var passData))
+                {
+                    builder.SetRenderAttachment(texture0, 0, AccessFlags.Write);
+                    builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
+                }
 
-            // Handles are unversioned by default. Unversioned handles use an implicit version "the latest version" depending on their
-            // usage context.
-            Assert.AreEqual(false, texture0.handle.IsVersioned);
+                // Access the versioned handle from the render pass
+                var writePass = m_RenderGraph.m_RenderPasses[writePassIndex];
+                var versionedWriteHandle = writePass.colorBufferAccess[0].textureHandle.handle;
+                Assert.IsTrue(versionedWriteHandle.IsVersioned, $"Write handle should be versioned at iteration {i}");
+                Assert.AreEqual(i + 1, versionedWriteHandle.version, $"Write version should be {i + 1} at iteration {i}");
 
-            // Writing bumps the version
-            using (var builder = m_RenderGraph.AddRasterRenderPass<RenderGraphTestPassData>("Async_TestPass0", out var passData))
-            {
-                texture0v1 = builder.UseTexture(texture0, AccessFlags.ReadWrite);
-                Assert.AreEqual(true, texture0v1.handle.IsVersioned);
-                Assert.AreEqual(1, texture0v1.handle.version);
-                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
-            }
+                int readPassIndex = m_RenderGraph.m_RenderPasses.Count;
 
-            // Writing again bumps again
-            using (var builder = m_RenderGraph.AddRasterRenderPass<RenderGraphTestPassData>("Async_TestPass1", out var passData))
-            {
-                texture0v2 = builder.UseTexture(texture0, AccessFlags.ReadWrite);
-                Assert.AreEqual(true, texture0v2.handle.IsVersioned);
-                Assert.AreEqual(2, texture0v2.handle.version);
-                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
-            }
+                // Reading leaves the version alone
+                using (var builder = m_RenderGraph.AddRasterRenderPass<RenderGraphTestPassData>("Async_TestPassRead"+i, out var passData))
+                {
+                    builder.SetRenderAttachment(texture0, 0, AccessFlags.Read);
+                }
 
-            // Reading leaves the version alone
-            using (var builder = m_RenderGraph.AddRasterRenderPass<RenderGraphTestPassData>("Async_TestPass2", out var passData))
-            {
-                var versioned = builder.UseTexture(texture0, AccessFlags.Read);
-                Assert.AreEqual(true, versioned.handle.IsVersioned);
-                Assert.AreEqual(2, versioned.handle.version);
-                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
+                // Access the versioned handle from the read pass
+                var readPass = m_RenderGraph.m_RenderPasses[readPassIndex];
+                var versionedReadHandle = readPass.colorBufferAccess[0].textureHandle.handle;
+                Assert.IsTrue(versionedReadHandle.IsVersioned, $"Read handle should be versioned at iteration {i}");
+                Assert.AreEqual(i + 1, versionedReadHandle.version, $"Read version should remain {i + 1} at iteration {i}");
             }
+        }
+
+        [Test]
+        public void ResourceHandleValidityBitManagement()
+        {
+            // Reset validityBit hash to execution 0
+            ResourceHandle.NewFrame(0);
+
+            // Create a handle and verify it has the current validity bit
+            TextureHandle texture0 = m_RenderGraph.CreateTexture(new TextureDesc(Vector2.one) { colorFormat = GraphicsFormat.R8G8B8A8_UNorm });
+            Assert.IsTrue(texture0.handle.IsValid(), "Newly created handle should be valid");
+            Assert.IsFalse(texture0.handle.IsNull(), "Newly created handle should not be null");
 
-            // Writing to an old version is not supported it would lead to a divergent version timeline for the resource
-            using (var builder = m_RenderGraph.AddRasterRenderPass<RenderGraphTestPassData>("Async_TestPass2", out var passData))
+            const int iterations = 216000; // 216k frames is like 1 hour at 60fps
+            for (int i = 1; i < iterations; ++i)
             {
-                // If you want do achieve this and avoid copying the move should be used
-                Assert.Throws<System.InvalidOperationException>(() =>
-                {
-                    var versioned = builder.UseTexture(texture0v1, AccessFlags.ReadWrite);
-                });
-                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
+                // Simulate a new frame with a new execution index
+                ResourceHandle.NewFrame(i);
+
+                // The stored handle should now be invalid because the validity bit has changed
+                Assert.IsFalse(texture0.IsValid(), "Handle from previous frame should be invalid");
             }
-        }*/
+        }
+
+        [Test]
+        public void ResourceHandleCheckValues()
+        {
+            // Check NullHandle
+            TextureHandle nullHandle = TextureHandle.nullHandle;
+            Assert.IsTrue(nullHandle.handle.IsNull(), "TextureHandle.nullHandle should return true to IsNull comparision.");
+
+            // Check a new created texture
+            TextureHandle defaultTexture = m_RenderGraph.CreateTexture(new TextureDesc(Vector2.one) { colorFormat = GraphicsFormat.R8G8B8A8_UNorm });
+            // Handles are unversioned by default.
+            Assert.IsTrue(defaultTexture.handle.IsValid(), "Newly created handle should be valid");
+            Assert.IsFalse(defaultTexture.handle.IsNull(), "Newly created handle should not be null");
+            Assert.IsFalse(defaultTexture.handle.IsVersioned, "Newly created handle should not be versioned");
+            Assert.AreEqual(-1, defaultTexture.handle.version, "Newly created handle version should be -1");
+
+
+            /* Check limits for version */
+            // Version has 15 bits, so the max valid number should be 2^15 - 1 =
+            int maxVersion = (int)Math.Pow(2,15) - 1;
+            ResourceHandle resourceVersionMax = new ResourceHandle(defaultTexture.handle, maxVersion);
+            Assert.IsTrue(resourceVersionMax.IsValid());
+            Assert.IsTrue(resourceVersionMax.IsVersioned);
+            Assert.AreEqual(maxVersion, resourceVersionMax.version);
+
+            ResourceHandle resourceVersionMin = new ResourceHandle(defaultTexture.handle, 0);
+            Assert.IsTrue(resourceVersionMin.IsValid());
+            Assert.IsTrue(resourceVersionMin.IsVersioned);
+            Assert.AreEqual(0, resourceVersionMin.version);
+
+            // Testing Out Of Range values makes Yamato jobs to fail.
+            // The test passes because the assert is correctly triggered, but it also fails the job because of the --fail-on-assert command, which is expected to fail the test if any assert is triggered.
+            // Need to comment this checks for now, until this is fixed.
+
+//            ResourceHandle resourceVersionOutOfRangeMax = new ResourceHandle(resourceVersionMax, maxVersion+1);
+//            ResourceHandle resourceVersionOutOfRangeMin = new ResourceHandle(resourceVersionMin, -1);
+//
+//#if UNITY_ASSERTIONS
+//            // If we increase the version to one more it should log an error
+//            LogAssert.Expect(LogType.Assert, "ResourceHandle: Invalid version, values should be >=0 && <32768");
+//            LogAssert.Expect(LogType.Assert, "ResourceHandle: Invalid version, values should be >=0 && <32768");
+//#endif
+
+
+            /* Check limits for index */
+            int maxIndex = (int)Math.Pow(2,16) - 1;
+            ResourceHandle resourceIndexMax = new ResourceHandle(maxIndex, RenderGraphResourceType.Texture, false);
+            Assert.IsTrue(resourceIndexMax.IsValid());
+            Assert.IsFalse(resourceIndexMax.IsVersioned);
+            Assert.AreEqual(maxIndex, resourceIndexMax.index);
+
+            ResourceHandle resourceIndexMin = new ResourceHandle(1, RenderGraphResourceType.Texture, false);
+            Assert.IsTrue(resourceIndexMin.IsValid());
+            Assert.IsFalse(resourceIndexMin.IsVersioned);
+            Assert.AreEqual(1, resourceIndexMin.index);
+
+            // Testing Out Of Range values makes Yamato jobs to fail.
+            // The test passes because the assert is correctly triggered, but it also fails the job because of the --fail-on-assert command, which is expected to fail the test if any assert is triggered.
+            // Need to comment this checks for now, until this is fixed.
+
+//            ResourceHandle resourceIndexOutOfRangeMax = new ResourceHandle(maxIndex+1, RenderGraphResourceType.Texture, false);
+//            ResourceHandle resourceIndexOutOfRange0 = new ResourceHandle(0, RenderGraphResourceType.Texture, false);
+//            ResourceHandle resourceIndexOutOfRangeNegative = new ResourceHandle(-1, RenderGraphResourceType.Texture, false);
+//
+//#if UNITY_ASSERTIONS
+//            // If we increase the version to one more it should log an error
+//            LogAssert.Expect(LogType.Assert, "ResourceHandle: Invalid index, values should be >0 && <65536");
+//            LogAssert.Expect(LogType.Assert, "ResourceHandle: Invalid index, values should be >0 && <65536");
+//            LogAssert.Expect(LogType.Assert, "ResourceHandle: Invalid index, values should be >0 && <65536");
+//#endif
+
+        }
 
         class RenderGraphAsyncRequestTestData
         {
@@ -2620,7 +2700,7 @@ public void CanMergeBackBufferAndCustomRenderTargets()
                     builder.SetRenderFunc(static (UVOriginPassData data, RasterGraphContext context) =>{ });
                     builder.AllowPassCulling(false);
                 }
-                
+
                 var result = m_RenderGraph.CompileNativeRenderGraph(m_RenderGraph.ComputeGraphHash());
                 var passes = result.contextData.GetNativePasses();