From 1bcbe0362190c3648e9e4d02402ba6f2f722ed50 Mon Sep 17 00:00:00 2001 From: hatayama Date: Wed, 7 Oct 2026 05:34:59 +0900 Subject: [PATCH 1/3] Make the method matcher a per-run instance and pin its read-once tests Entry resolution and the unchanged-patch peel now each create one matcher per group and pass its Resolve on as a delegate. The matcher still reads the compiled image on every call, so the new tests for one read per image, answers after the file is deleted or replaced, the per-home loaded-assembly check, the loader contract and disposal fail until the next commit. --- .../HotReloadEntryResolutionTests.cs | 10 + .../HotReload/HotReloadMethodMatcherTests.cs | 371 +++++++++++++++++- .../HotReloadSpikeS4ArtifactPatchTests.cs | 6 +- .../HotReloadUnchangedPatchPeelTests.cs | 3 +- .../HotReload/HotReloadEntryApplier.cs | 7 +- .../HotReload/HotReloadEntryResolution.cs | 10 +- .../HotReloadGroupEntryPreparation.cs | 16 +- .../Patching/HotReloadMethodMatcher.cs | 46 ++- 8 files changed, 451 insertions(+), 18 deletions(-) diff --git a/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs b/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs index 386579953b..bfc3b96fe3 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs @@ -26,16 +26,19 @@ public class HotReloadEntryResolutionTests private static Assembly ShimAssembly => typeof(HotReloadHandwrittenShims).Assembly; private HotReloadDomainTestScope _scope; + private HotReloadMethodMatcher _matcher; [SetUp] public void SetUp() { _scope = new HotReloadDomainTestScope(); + _matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); } [TearDown] public void TearDown() { + _matcher.Dispose(); _scope.Dispose(); } @@ -80,6 +83,7 @@ public void ResolveEntries_WhenEveryEntryResolves_ReportsAllResolved() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -114,6 +118,7 @@ public void ResolveEntries_WhenShimMethodIsMissing_FailsTheFileAtomically() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -158,6 +163,7 @@ public void ResolveEntries_WhenAddedMethodHasNoUsableCounter_FailsTheFileAtomica HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -197,6 +203,7 @@ public void ResolveEntries_WhenEntryIsAnAddedMethod_ResolvesWithoutAnOriginalMet HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -233,6 +240,7 @@ public void ResolveEntries_RecordsTheAddedMembersEachEntryCalls() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -273,6 +281,7 @@ public void ResolveEntries_WhenACallNamesNoAddedEntry_FailsTheFileAtomically() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -310,6 +319,7 @@ public void ResolveEntries_WhenATransplantShimTypeFailedToBind_FailsTheEntry() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, diff --git a/Assets/Tests/Editor/HotReload/HotReloadMethodMatcherTests.cs b/Assets/Tests/Editor/HotReload/HotReloadMethodMatcherTests.cs index 5ffbb1f0b9..f32b3a7a3d 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadMethodMatcherTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadMethodMatcherTests.cs @@ -1,6 +1,9 @@ +using System; using System.IO; using System.Reflection; +using Mono.Cecil; + using NUnit.Framework; using UnityEngine; @@ -10,13 +13,41 @@ namespace io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload { /// - /// EditMode coverage for resolution and overload selection. + /// EditMode coverage for resolution and overload selection, + /// and for how often one matcher reads a compiled image. /// public class HotReloadMethodMatcherTests { private const string TestAssemblyName = "UnityCLILoop.Tests.Editor.HotReload"; private const string FixtureTypeMetadataName = "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadCoreFixture"; + private const string SameNameFixtureTypeMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadPeelSameNameFixture"; + // Cecil names a nested type with '/', the separator the worker's rows carry. + private const string NestedFixtureTypeMetadataName = + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.HotReloadBindingSplitNestedRegistry/Inner"; + private const string DomainTypeMetadataName = "io.github.hatayama.UnityCliLoop.FirstPartyTools.HotReloadDomain"; + private const string NotLoadedAssemblyName = "NotLoaded.Assembly.ForTest"; + + private string _tempDirectory; + + [SetUp] + public void SetUp() + { + _tempDirectory = Path.Combine( + Path.GetTempPath(), + "uloop-method-matcher-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(_tempDirectory); + } + + [TearDown] + public void TearDown() + { + if (Directory.Exists(_tempDirectory)) + { + Directory.Delete(_tempDirectory, recursive: true); + } + } // The test assembly is itself a project assembly, so its compiled image is the one the // matcher reads: a ScriptAssemblies home naming it is what production passes in. @@ -32,13 +63,37 @@ private static string TestAssemblyDllPath } } + // The matcher's own assembly, Patching, is a second project assembly with an image of its + // own, so an entry whose home names it must be answered from that image and not from the + // test assembly's. + private static HotReloadTypeHome PatchingAssemblyHome + { + get + { + Assembly patchingAssembly = typeof(HotReloadMethodMatcher).Assembly; + return HotReloadTypeHome.ScriptAssemblies(patchingAssembly.GetName().Name, patchingAssembly.Location); + } + } + + private static MethodInfo StaticPingMethod => + typeof(HotReloadCoreFixture).GetMethod(nameof(HotReloadCoreFixture.StaticPing)); + + private static MethodInfo AddMethod => + typeof(HotReloadCoreFixture).GetMethod( + nameof(HotReloadCoreFixture.Add), + new[] { typeof(int), typeof(int) }); + + private string TempImagePath => Path.Combine(_tempDirectory, TestAssemblyName + ".dll"); + /// /// What: a known instance method resolves to the live MethodBase with matching MetadataToken identity. /// [Test] public void Resolve_KnownInstanceMethod_ReturnsLiveMethodBase() { - HotReloadMethodMatchResult result = HotReloadMethodMatcher.Resolve( + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + + HotReloadMethodMatchResult result = matcher.Resolve( TestAssemblyHome, FixtureTypeMetadataName, nameof(HotReloadCoreFixture.Add), @@ -62,7 +117,9 @@ public void Resolve_KnownInstanceMethod_ReturnsLiveMethodBase() [Test] public void Resolve_Overload_SelectsMatchingParameterTypes() { - HotReloadMethodMatchResult result = HotReloadMethodMatcher.Resolve( + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + + HotReloadMethodMatchResult result = matcher.Resolve( TestAssemblyHome, FixtureTypeMetadataName, nameof(HotReloadCoreFixture.Add), @@ -84,7 +141,9 @@ public void Resolve_Overload_SelectsMatchingParameterTypes() [Test] public void Resolve_ParameterTypeMismatch_ReturnsMethodNotFound() { - HotReloadMethodMatchResult result = HotReloadMethodMatcher.Resolve( + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + + HotReloadMethodMatchResult result = matcher.Resolve( TestAssemblyHome, FixtureTypeMetadataName, nameof(HotReloadCoreFixture.Add), @@ -95,13 +154,34 @@ public void Resolve_ParameterTypeMismatch_ReturnsMethodNotFound() Assert.That(result.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.MethodNotFound)); } + /// + /// What: a type the compiled image does not declare yields TypeNotFound. + /// + [Test] + public void Resolve_UnknownType_ReturnsTypeNotFound() + { + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + + HotReloadMethodMatchResult result = matcher.Resolve( + TestAssemblyHome, + "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload.NoSuchFixtureType", + nameof(HotReloadCoreFixture.StaticPing), + new string[0], + 0); + + Assert.That(result.Success, Is.False); + Assert.That(result.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.TypeNotFound)); + } + /// /// What: a static method with no parameters resolves correctly. /// [Test] public void Resolve_StaticMethod_ReturnsLiveMethodBase() { - HotReloadMethodMatchResult result = HotReloadMethodMatcher.Resolve( + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + + HotReloadMethodMatchResult result = matcher.Resolve( TestAssemblyHome, FixtureTypeMetadataName, nameof(HotReloadCoreFixture.StaticPing), @@ -124,14 +204,15 @@ public void Resolve_SameNameAndParameters_SelectsByGenericArity() "io.github.hatayama.UnityCliLoop.Tests.Editor.HotReload" + ".HotReloadSignatureChangeGenericCallerFixture"; string[] parameterTypeFullNames = { "System.Int32" }; + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); - HotReloadMethodMatchResult nonGeneric = HotReloadMethodMatcher.Resolve( + HotReloadMethodMatchResult nonGeneric = matcher.Resolve( TestAssemblyHome, typeMetadataName, nameof(HotReloadSignatureChangeGenericCallerFixture.Caller), parameterTypeFullNames, 0); - HotReloadMethodMatchResult generic = HotReloadMethodMatcher.Resolve( + HotReloadMethodMatchResult generic = matcher.Resolve( TestAssemblyHome, typeMetadataName, nameof(HotReloadSignatureChangeGenericCallerFixture.Caller), @@ -169,5 +250,281 @@ public void ResolveLoadedMethod_MvidMismatch_ReturnsStaleAssembly() Assert.That(result.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.StaleAssembly)); Assert.That(result.Method, Is.Null); } + + /// + /// What: entries of one compiled image, on two types and one of them nested, are all + /// answered from a single read, each with the live method reflection finds. + /// + [Test] + public void Resolve_SeveralEntriesOfOneImage_ReadsTheImageOnce() + { + CountingLoader loader = new CountingLoader(); + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + + HotReloadMethodMatchResult add = ResolveAdd(matcher, TestAssemblyHome); + HotReloadMethodMatchResult otherType = matcher.Resolve( + TestAssemblyHome, + SameNameFixtureTypeMetadataName, + nameof(HotReloadPeelSameNameFixture.StaticPing), + new string[0], + 0); + HotReloadMethodMatchResult nested = matcher.Resolve( + TestAssemblyHome, + NestedFixtureTypeMetadataName, + nameof(HotReloadBindingSplitNestedRegistry.Inner.Raise), + new[] { "System.Int32" }, + 0); + + AssertResolvedTo(add, AddMethod); + AssertResolvedTo( + otherType, + typeof(HotReloadPeelSameNameFixture).GetMethod(nameof(HotReloadPeelSameNameFixture.StaticPing))); + AssertResolvedTo( + nested, + typeof(HotReloadBindingSplitNestedRegistry.Inner).GetMethod( + nameof(HotReloadBindingSplitNestedRegistry.Inner.Raise))); + Assert.That(loader.Loads, Is.EqualTo(1)); + } + + /// + /// What: a run that resolves entries of two compiled images, alternating between them, + /// reads each image once and answers every entry from the image its home names. + /// + [Test] + public void Resolve_TwoImages_ReadsEachOnceAndAnswersFromItsOwn() + { + CountingLoader loader = new CountingLoader(); + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + MethodInfo domainDispose = typeof(HotReloadDomain).GetMethod(nameof(HotReloadDomain.Dispose)); + + for (int round = 0; round < 2; round++) + { + AssertResolvedTo(ResolveStaticPing(matcher, TestAssemblyHome), StaticPingMethod); + HotReloadMethodMatchResult fromPatchingImage = matcher.Resolve( + PatchingAssemblyHome, + DomainTypeMetadataName, + nameof(HotReloadDomain.Dispose), + new string[0], + 0); + AssertResolvedTo(fromPatchingImage, domainDispose); + } + + Assert.That(loader.Loads, Is.EqualTo(2)); + } + + /// + /// What: an image that is not there yet is reported missing without being remembered, so + /// once the file appears the same matcher reads it and resolves the next entry. + /// + [Test] + public void Resolve_MissingImage_IsLookedForAgainOnTheNextEntry() + { + HotReloadTypeHome home = HotReloadTypeHome.ScriptAssemblies(TestAssemblyName, TempImagePath); + CountingLoader loader = new CountingLoader(); + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + + HotReloadMethodMatchResult missing = ResolveStaticPing(matcher, home); + Assert.That(missing.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.CompiledAssemblyNotFound)); + Assert.That(loader.Loads, Is.EqualTo(0)); + + File.Copy(TestAssemblyDllPath, TempImagePath); + AssertResolvedTo(ResolveStaticPing(matcher, home), StaticPingMethod); + Assert.That(loader.Loads, Is.EqualTo(1)); + } + + /// + /// What: a read that throws reaches the caller and leaves nothing behind, so the next entry + /// of the same matcher reads the image again and resolves. + /// + [Test] + public void Resolve_ImageThatFailedToRead_IsReadAgainOnTheNextEntry() + { + CountingLoader loader = new CountingLoader { FailsReads = true }; + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + + Assert.Throws(() => ResolveStaticPing(matcher, TestAssemblyHome)); + + loader.FailsReads = false; + AssertResolvedTo(ResolveStaticPing(matcher, TestAssemblyHome), StaticPingMethod); + Assert.That(loader.Loads, Is.EqualTo(2)); + } + + /// + /// What: once a matcher has read an image, deleting the file does not change its answers: + /// a later entry of the same run still resolves from that read. + /// + [Test] + public void Resolve_ImageDeletedAfterTheFirstRead_StillAnswersFromThatRead() + { + HotReloadTypeHome home = CopyTestAssemblyHome(); + CountingLoader loader = new CountingLoader(); + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + AssertResolvedTo(ResolveStaticPing(matcher, home), StaticPingMethod); + + File.Delete(home.DllPath); + + AssertResolvedTo(ResolveAdd(matcher, home), AddMethod); + Assert.That(loader.Loads, Is.EqualTo(1)); + } + + /// + /// What: a matcher keeps answering from the image it read after the file is replaced by + /// another image, while a new matcher, as the next run makes, reads the replacement. + /// + [Test] + public void Resolve_ImageReplacedAfterTheFirstRead_SameMatcherKeepsItsRead_AndANewMatcherReadsTheNewImage() + { + HotReloadTypeHome home = CopyTestAssemblyHome(); + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + AssertResolvedTo(ResolveStaticPing(matcher, home), StaticPingMethod); + + File.Copy(typeof(HotReloadMethodMatcher).Assembly.Location, home.DllPath, overwrite: true); + + AssertResolvedTo(ResolveAdd(matcher, home), AddMethod); + using HotReloadMethodMatcher nextRunMatcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + HotReloadMethodMatchResult fromReplacement = ResolveAdd(nextRunMatcher, home); + Assert.That(fromReplacement.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.TypeNotFound)); + } + + /// + /// What: disposing a matcher twice is harmless, and a disposed matcher refuses to resolve + /// instead of answering from a read it has released. + /// + [Test] + public void Resolve_AfterDispose_Throws() + { + HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + AssertResolvedTo(ResolveStaticPing(matcher, TestAssemblyHome), StaticPingMethod); + + matcher.Dispose(); + Assert.DoesNotThrow(matcher.Dispose); + + Assert.Throws(() => ResolveStaticPing(matcher, TestAssemblyHome)); + } + + /// + /// What: an image whose assembly is not loaded reports AssemblyNotLoaded for every entry, + /// because the loaded-assembly check runs per entry, while the image itself is read once. + /// + [Test] + public void Resolve_AssemblyNotLoaded_IsReportedForEveryEntry_WithOneRead() + { + HotReloadTypeHome home = HotReloadTypeHome.ScriptAssemblies(NotLoadedAssemblyName, CopyTestAssembly()); + CountingLoader loader = new CountingLoader(); + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + + HotReloadMethodMatchResult first = ResolveStaticPing(matcher, home); + HotReloadMethodMatchResult second = ResolveStaticPing(matcher, home); + + Assert.That(first.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.AssemblyNotLoaded)); + Assert.That(second.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.AssemblyNotLoaded)); + Assert.That(loader.Loads, Is.EqualTo(1)); + } + + /// + /// What: two homes that name the same image under different assembly names share one read, + /// but each is checked against the loaded assemblies on its own, so the home whose assembly + /// is not loaded reports AssemblyNotLoaded after the other home resolved. + /// + [Test] + public void Resolve_SameImageUnderTwoHomes_ChecksTheLoadedAssemblyForEachHome() + { + HotReloadTypeHome notLoadedHome = + HotReloadTypeHome.ScriptAssemblies(NotLoadedAssemblyName, TestAssemblyDllPath); + CountingLoader loader = new CountingLoader(); + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + + AssertResolvedTo(ResolveStaticPing(matcher, TestAssemblyHome), StaticPingMethod); + HotReloadMethodMatchResult underNotLoadedName = ResolveStaticPing(matcher, notLoadedHome); + + Assert.That(underNotLoadedName.FailureReason, Is.EqualTo(HotReloadMethodMatchFailureReason.AssemblyNotLoaded)); + Assert.That(loader.Loads, Is.EqualTo(1)); + } + + /// + /// What: a loader that returns no image breaks its contract, so the matcher throws instead + /// of resolving against nothing, and remembers nothing: the next entry reads the image again. + /// + [Test] + public void Resolve_LoaderReturningNull_Throws_AndTheImageIsReadAgainOnTheNextEntry() + { + CountingLoader loader = new CountingLoader { ReturnsNoImage = true }; + using HotReloadMethodMatcher matcher = new HotReloadMethodMatcher(loader.Load); + + Assert.Throws(() => ResolveStaticPing(matcher, TestAssemblyHome)); + + loader.ReturnsNoImage = false; + AssertResolvedTo(ResolveStaticPing(matcher, TestAssemblyHome), StaticPingMethod); + Assert.That(loader.Loads, Is.EqualTo(2)); + } + + private static HotReloadMethodMatchResult ResolveStaticPing(HotReloadMethodMatcher matcher, HotReloadTypeHome home) + { + return matcher.Resolve( + home, + FixtureTypeMetadataName, + nameof(HotReloadCoreFixture.StaticPing), + new string[0], + 0); + } + + private static HotReloadMethodMatchResult ResolveAdd(HotReloadMethodMatcher matcher, HotReloadTypeHome home) + { + return matcher.Resolve( + home, + FixtureTypeMetadataName, + nameof(HotReloadCoreFixture.Add), + new[] { "System.Int32", "System.Int32" }, + 0); + } + + private static void AssertResolvedTo(HotReloadMethodMatchResult result, MethodInfo expected) + { + Assert.That(expected, Is.Not.Null, "Reflection must find the method the entry names."); + Assert.That(result.Success, Is.True, result.FailureReason + ": " + result.ErrorMessage); + Assert.That(result.Method, Is.EqualTo(expected)); + } + + // A copy of the test assembly's image keeps the loaded assembly's Mvid, so it resolves + // like the original while the test is free to delete or replace the file. + private HotReloadTypeHome CopyTestAssemblyHome() + { + return HotReloadTypeHome.ScriptAssemblies(TestAssemblyName, CopyTestAssembly()); + } + + private string CopyTestAssembly() + { + File.Copy(TestAssemblyDllPath, TempImagePath); + return TempImagePath; + } + + /// + /// A loader that counts every read it is asked for, failed ones included, and can be told + /// to fail them or to return no image. + /// + private sealed class CountingLoader + { + public int Loads { get; private set; } + + public bool FailsReads { get; set; } + + public bool ReturnsNoImage { get; set; } + + public AssemblyDefinition Load(string dllPath) + { + Loads++; + if (FailsReads) + { + throw new IOException("Simulated read failure: " + dllPath); + } + + if (ReturnsNoImage) + { + return null; + } + + return HotReloadMethodMatcher.ReadCompiledAssembly(dllPath); + } + } } } diff --git a/Assets/Tests/Editor/HotReload/HotReloadSpikeS4ArtifactPatchTests.cs b/Assets/Tests/Editor/HotReload/HotReloadSpikeS4ArtifactPatchTests.cs index 4c99ac30a1..5814e38f24 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadSpikeS4ArtifactPatchTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadSpikeS4ArtifactPatchTests.cs @@ -75,7 +75,8 @@ public async Task MethodMatcher_ResolvesMethodInsideRetainedArtifactHome() { LoadedArtifact artifact = await CompileAndLoadArtifactAsync("SpikeS4Artifact_Matcher"); - HotReloadMethodMatchResult matchResult = HotReloadMethodMatcher.Resolve( + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + HotReloadMethodMatchResult matchResult = matcher.Resolve( artifact.Home, IntroducedTypeMetadataName, "Compute", @@ -167,7 +168,8 @@ private static void PatchWithReturnTwo(MethodBase method) private static MethodBase ResolveOrFail(LoadedArtifact artifact, string methodName) { - HotReloadMethodMatchResult matchResult = HotReloadMethodMatcher.Resolve( + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + HotReloadMethodMatchResult matchResult = matcher.Resolve( artifact.Home, IntroducedTypeMetadataName, methodName, diff --git a/Assets/Tests/Editor/HotReload/HotReloadUnchangedPatchPeelTests.cs b/Assets/Tests/Editor/HotReload/HotReloadUnchangedPatchPeelTests.cs index a713c68032..dc3cedc726 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadUnchangedPatchPeelTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadUnchangedPatchPeelTests.cs @@ -265,10 +265,11 @@ private static void Revert(HotReloadGroupFile file) private static PeelRun RevertRows(HotReloadGroupFile file, params TransformWorkerUnchangedMethodDto[] rows) { int resolveCalls = 0; + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); HotReloadMethodResolver counting = (home, typeMetadataName, methodName, parameterTypeFullNames, genericArity) => { resolveCalls++; - return HotReloadMethodMatcher.Resolve( + return matcher.Resolve( home, typeMetadataName, methodName, parameterTypeFullNames, genericArity); }; List outcomes = new List(); diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs index e57b591cbd..c6482ff97c 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs @@ -197,6 +197,11 @@ internal void RevertUnchangedPatchesPerFile( HotReloadEntryHomeResolver homeResolver = new HotReloadEntryHomeResolver(_domain, files[0].ProjectRoot); + // Why once for the group: every row of the group that names the same home resolves + // against the same compiled image, so the peel reads that image once instead of once + // for every row it resolves. + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + HotReloadMethodResolver resolveMethod = matcher.Resolve; foreach (HotReloadGroupFile file in files) { // Why a file left unapplied is left alone: a file the shim compile refused keeps @@ -223,7 +228,7 @@ internal void RevertUnchangedPatchesPerFile( unchangedMethods, file.Sinks.Outcomes, file.AssemblyResolvePath, - HotReloadMethodMatcher.Resolve); + resolveMethod); } } /// diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryResolution.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryResolution.cs index ec807516ca..27f20ad17f 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryResolution.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryResolution.cs @@ -18,9 +18,12 @@ internal static class HotReloadEntryResolution // accessor binders run once for the group instead of once per file. // Why addedCallees is passed in: a body can call an added member another file of the group // declares, so one file's entries alone cannot name every call. + // Why resolveMethod is passed in: the caller owns the run's matcher and decides how long it + // lives, so resolution never holds a matcher past the run. internal static Result ResolveEntries( HotReloadTypeHome fileHome, HotReloadEntryHomeResolver homeResolver, + HotReloadMethodResolver resolveMethod, string filePath, Assembly shimAssembly, TransformWorkerEntryDto[] entriesToPatch, @@ -29,6 +32,7 @@ internal static Result ResolveEntries( { Debug.Assert(fileHome != null, "fileHome must not be null."); Debug.Assert(homeResolver != null, "homeResolver must not be null."); + Debug.Assert(resolveMethod != null, "resolveMethod must not be null."); Debug.Assert(!string.IsNullOrEmpty(filePath), "filePath must not be empty."); Debug.Assert(shimAssembly != null, "shimAssembly must not be null."); Debug.Assert(entriesToPatch != null, "entriesToPatch must not be null."); @@ -42,6 +46,7 @@ internal static Result ResolveEntries( entriesToPatch[index], fileHome, homeResolver, + resolveMethod, shimAssembly, bindFailures, addedCallees, @@ -114,6 +119,7 @@ private static ResolvedEntryOutcome TryResolveEntry( TransformWorkerEntryDto entry, HotReloadTypeHome fileHome, HotReloadEntryHomeResolver homeResolver, + HotReloadMethodResolver resolveMethod, Assembly shimAssembly, IReadOnlyDictionary bindFailures, HotReloadAddedCalleeIndex addedCallees, @@ -148,6 +154,7 @@ private static ResolvedEntryOutcome TryResolveEntry( methodLabel, fileHome, homeResolver, + resolveMethod, shimAssembly, bindFailures, calledAddedMembers, @@ -224,6 +231,7 @@ private static ResolvedEntryOutcome TryResolveExistingMethod( string methodLabel, HotReloadTypeHome fileHome, HotReloadEntryHomeResolver homeResolver, + HotReloadMethodResolver resolveMethod, Assembly shimAssembly, IReadOnlyDictionary bindFailures, IReadOnlyList calledAddedMembers, @@ -245,7 +253,7 @@ private static ResolvedEntryOutcome TryResolveExistingMethod( // introduced-type artifact this domain retains rather than in the assembly the // edited file belongs to, and only the row knows which. HotReloadTypeHome home = homeResolver.Resolve(fileHome, entry.homeAssemblyName); - HotReloadMethodMatchResult matchResult = HotReloadMethodMatcher.Resolve( + HotReloadMethodMatchResult matchResult = resolveMethod( home, entry.typeMetadataName, entry.methodName, diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadGroupEntryPreparation.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadGroupEntryPreparation.cs index 5096287086..e76053c95f 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadGroupEntryPreparation.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadGroupEntryPreparation.cs @@ -45,12 +45,24 @@ internal static IReadOnlyList PrepareGroup( // Why once for the group: a body can call an added member another file of the group // declares, and only the whole group's entries name every added member it may call. HotReloadAddedCalleeIndex addedCallees = new HotReloadAddedCalleeIndex(entriesToPatch); + // Why once for the group: every entry of the group that names the same home resolves + // against the same compiled image, and reading it per entry made the patch stage grow + // with the number of live patches in a file. + using HotReloadMethodMatcher matcher = HotReloadMethodMatcher.CreateReadingFromDisk(); + HotReloadMethodResolver resolveMethod = matcher.Resolve; List prepared = new List(context.Files.Count); foreach (HotReloadGroupFile file in context.Files) { prepared.Add( - PrepareFile(compileResult, homeResolver, file, entriesByFile, bindFailures, addedCallees)); + PrepareFile( + compileResult, + homeResolver, + resolveMethod, + file, + entriesByFile, + bindFailures, + addedCallees)); } return prepared; @@ -59,6 +71,7 @@ internal static IReadOnlyList PrepareGroup( private static HotReloadPreparedGroupFile PrepareFile( HotReloadShimCompileResult compileResult, HotReloadEntryHomeResolver homeResolver, + HotReloadMethodResolver resolveMethod, HotReloadGroupFile file, Dictionary> entriesByFile, Dictionary bindFailures, @@ -79,6 +92,7 @@ private static HotReloadPreparedGroupFile PrepareFile( HotReloadEntryResolution.Result resolution = HotReloadEntryResolution.ResolveEntries( file.Home, homeResolver, + resolveMethod, file.AssemblyResolvePath, compileResult.Assembly, entries, diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs index 48f488a702..04f236380c 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs @@ -1,3 +1,4 @@ +using System; using System.IO; using System.Reflection; @@ -15,20 +16,53 @@ internal delegate HotReloadMethodMatchResult HotReloadMethodResolver( string[] parameterTypeFullNames, int genericArity); + /// Reads the compiled image at a path; the shape of the matcher's assembly loader. + internal delegate AssemblyDefinition HotReloadCompiledAssemblyLoader(string dllPath); + /// /// Resolves a hot-reload manifest entry (type metadata name + method name + parameter type /// full names) to the matching MethodBase in the running AppDomain, using Cecil metadata /// tokens and an Mvid guard against stale script assemblies. /// - internal static class HotReloadMethodMatcher + internal sealed class HotReloadMethodMatcher : IDisposable { + private readonly HotReloadCompiledAssemblyLoader _loadAssembly; + + /// + /// How a compiled image is read; production passes ReadCompiledAssembly, and tests count + /// the reads. + /// + internal HotReloadMethodMatcher(HotReloadCompiledAssemblyLoader loadAssembly) + { + if (loadAssembly == null) + { + throw new ArgumentNullException(nameof(loadAssembly)); + } + + _loadAssembly = loadAssembly; + } + + /// A matcher for one run that reads the compiled images from disk. + internal static HotReloadMethodMatcher CreateReadingFromDisk() + { + return new HotReloadMethodMatcher(ReadCompiledAssembly); + } + + /// The loader production uses: reads the whole compiled image at the path. + internal static AssemblyDefinition ReadCompiledAssembly(string dllPath) + { + // InMemory: the DLL is the currently loaded script assembly; keep no file handle on it. + ReaderParameters readerParameters = new ReaderParameters { InMemory = true }; + return AssemblyDefinition.ReadAssembly(dllPath, readerParameters); + } + /// /// Resolves on inside /// the assembly names, whose parameters and generic arity match /// and /// exactly (Cecil FullName, no this). /// - public static HotReloadMethodMatchResult Resolve( + public HotReloadMethodMatchResult Resolve( HotReloadTypeHome home, string typeMetadataName, string methodName, @@ -50,9 +84,7 @@ public static HotReloadMethodMatchResult Resolve( $"Compiled assembly not found at '{dllPath}'. Compile the project first."); } - // InMemory: the DLL is the currently loaded script assembly; keep no file handle on it. - ReaderParameters readerParameters = new ReaderParameters { InMemory = true }; - using AssemblyDefinition assemblyDefinition = AssemblyDefinition.ReadAssembly(dllPath, readerParameters); + using AssemblyDefinition assemblyDefinition = _loadAssembly(dllPath); TypeDefinition typeDefinition = assemblyDefinition.MainModule.GetType(typeMetadataName); if (typeDefinition == null) @@ -79,6 +111,10 @@ public static HotReloadMethodMatchResult Resolve( return ResolveLoadedMethod(home, compiledMvid, metadataToken); } + public void Dispose() + { + } + private static MethodDefinition FindMatchingMethod( TypeDefinition typeDefinition, string methodName, From fb95e76c680d8c6b9a55a3a9bceba9877f344364 Mon Sep 17 00:00:00 2001 From: hatayama Date: Wed, 7 Oct 2026 05:37:08 +0900 Subject: [PATCH 2/3] Read each compiled image once per hot-reload matcher Every entry used to read the whole compiled assembly again and rebuild its type table, so the patch stage grew with the number of live patches in a file. The matcher now keeps each image it read successfully until it is disposed at the end of the group, while a missing or unreadable image is still looked for again by the next entry and the Mvid guard still runs for every entry. A disposed matcher refuses to resolve, and a loader that returns no image is reported instead of failing on a null read. --- .../Patching/HotReloadMethodMatcher.cs | 62 ++++++++++++++++--- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs b/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs index 04f236380c..d260abffa2 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.IO; using System.Reflection; @@ -20,13 +21,18 @@ internal delegate HotReloadMethodMatchResult HotReloadMethodResolver( internal delegate AssemblyDefinition HotReloadCompiledAssemblyLoader(string dllPath); /// - /// Resolves a hot-reload manifest entry (type metadata name + method name + parameter type - /// full names) to the matching MethodBase in the running AppDomain, using Cecil metadata - /// tokens and an Mvid guard against stale script assemblies. + /// Resolves hot-reload manifest entries (type metadata name + method name + parameter type + /// full names) to the matching MethodBase in the running AppDomain for one run, using Cecil + /// metadata tokens and an Mvid guard against stale script assemblies. Reads each compiled + /// image once and answers every later entry of the run from that read; the Mvid guard still + /// compares that read with the loaded assembly for every entry. /// internal sealed class HotReloadMethodMatcher : IDisposable { private readonly HotReloadCompiledAssemblyLoader _loadAssembly; + private readonly Dictionary _assembliesByDllPath = + new Dictionary(StringComparer.Ordinal); + private bool _disposed; /// /// How a compiled image is read; production passes ReadCompiledAssembly, and tests count @@ -62,6 +68,8 @@ internal static AssemblyDefinition ReadCompiledAssembly(string dllPath) /// and /// exactly (Cecil FullName, no this). /// + /// The matcher was disposed: its run is over. + /// The loader returned no image. public HotReloadMethodMatchResult Resolve( HotReloadTypeHome home, string typeMetadataName, @@ -75,16 +83,37 @@ public HotReloadMethodMatchResult Resolve( Debug.Assert(parameterTypeFullNames != null, "parameterTypeFullNames must not be null."); Debug.Assert(genericArity >= 0, "genericArity must not be negative."); - string dllPath = home.DllPath; - - if (!File.Exists(dllPath)) + if (_disposed) { - return HotReloadMethodMatchResult.Failure( - HotReloadMethodMatchFailureReason.CompiledAssemblyNotFound, - $"Compiled assembly not found at '{dllPath}'. Compile the project first."); + throw new ObjectDisposedException(nameof(HotReloadMethodMatcher)); } - using AssemblyDefinition assemblyDefinition = _loadAssembly(dllPath); + string dllPath = home.DllPath; + // Why a read answers the rest of the run even if the file changes on disk: the token + // and the Mvid below both come from that one read, and the Mvid guard still compares + // it with the loaded assembly for every entry, so no token is applied to an assembly + // it was not read from. + // Why only a read that succeeded is kept: an image that is missing or fails to read is + // looked for again by the next entry, as every entry did before, so one that appears + // or becomes readable later in the run is still found. + if (!_assembliesByDllPath.TryGetValue(dllPath, out AssemblyDefinition assemblyDefinition)) + { + if (!File.Exists(dllPath)) + { + return HotReloadMethodMatchResult.Failure( + HotReloadMethodMatchFailureReason.CompiledAssemblyNotFound, + $"Compiled assembly not found at '{dllPath}'. Compile the project first."); + } + + assemblyDefinition = _loadAssembly(dllPath); + if (assemblyDefinition == null) + { + throw new InvalidOperationException( + $"The compiled assembly loader returned no image for '{dllPath}'."); + } + + _assembliesByDllPath.Add(dllPath, assemblyDefinition); + } TypeDefinition typeDefinition = assemblyDefinition.MainModule.GetType(typeMetadataName); if (typeDefinition == null) @@ -111,8 +140,21 @@ public HotReloadMethodMatchResult Resolve( return ResolveLoadedMethod(home, compiledMvid, metadataToken); } + /// Releases every image this matcher read; a disposed matcher resolves nothing more. public void Dispose() { + if (_disposed) + { + return; + } + + _disposed = true; + foreach (AssemblyDefinition assemblyDefinition in _assembliesByDllPath.Values) + { + assemblyDefinition.Dispose(); + } + + _assembliesByDllPath.Clear(); } private static MethodDefinition FindMatchingMethod( From ee891dd72425a9e16881c59dde8db3d8552660ce Mon Sep 17 00:00:00 2001 From: hatayama Date: Wed, 7 Oct 2026 05:38:43 +0900 Subject: [PATCH 3/3] Pin that entry resolution goes through the run's resolver Existing-method entries are now resolved through the delegate the caller passes, so a test counts the calls to make sure the group's matcher is the one that answers. The peel's comments now say that only the first row resolved against an image reads it, which the name prefilter still spares along with the per-row matching. --- .../HotReloadEntryResolutionTests.cs | 39 +++++++++++++++++++ .../HotReload/HotReloadEntryApplier.cs | 15 +++---- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs b/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs index bfc3b96fe3..7358220a6f 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs @@ -95,6 +95,45 @@ public void ResolveEntries_WhenEveryEntryResolves_ReportsAllResolved() Assert.That(result.FailureOutcomes, Is.Empty); } + /// + /// What: every entry that patches an existing method is resolved through the resolver the + /// caller passes in, which is how one matcher of the run answers all of a group's entries. + /// + [Test] + public void ResolveEntries_ResolvesEachExistingMethodEntryThroughTheGivenResolver() + { + TransformWorkerEntryDto[] entries = + { + BuildExistingMethodEntry( + nameof(HotReloadCoreFixture.StaticPing), + new string[0], + "StaticPing__shim0"), + BuildExistingMethodEntry( + nameof(HotReloadCoreFixture.ReplaceableCompute), + new[] { "System.Int32" }, + "ReplaceableCompute__shim0") + }; + int resolveCalls = 0; + HotReloadMethodResolver counting = (home, typeMetadataName, methodName, parameterTypeFullNames, genericArity) => + { + resolveCalls++; + return _matcher.Resolve(home, typeMetadataName, methodName, parameterTypeFullNames, genericArity); + }; + + HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( + TestAssemblyHome, + FileHomeResolver, + counting, + FilePath, + ShimAssembly, + entries, + new Dictionary(), + new HotReloadAddedCalleeIndex(entries)); + + Assert.That(result.AllResolved, Is.True); + Assert.That(resolveCalls, Is.EqualTo(2)); + } + /// /// What: an entry naming a shim method the shim assembly does not declare fails the whole /// file — the result is not all-resolved, the failing row is reported Failed, and every diff --git a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs index c6482ff97c..4778f0b569 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/HotReloadEntryApplier.cs @@ -98,8 +98,8 @@ private HotReloadFileProcessResult ApplyPreparedFile( /// Returns how many Revert calls actually removed a live patch. /// /// - /// How a row is resolved; production passes HotReloadMethodMatcher.Resolve, and tests - /// count the calls. + /// How a row is resolved; production passes the Resolve of the matcher it made for the + /// group, and tests count the calls. /// internal int RevertUnchangedPatches( HotReloadTypeHome fileHome, @@ -138,11 +138,12 @@ internal int RevertUnchangedPatches( // resolved. HotReloadTypeHome home = homeResolver.Resolve(fileHome, unchanged.homeAssemblyName); - // Why by name before resolving: resolving a row reads the whole compiled assembly, - // and a row can only be peeled when the method it resolves to holds a live patch. - // That method carries the row's name, so a name no live patch has cannot lead to a - // peel. The name is only a prefilter: a row that passes is still resolved exactly, - // type, parameters and arity included. + // Why by name before resolving: the first row resolved against an image reads the + // whole compiled assembly, and every row is then matched against it, while a row can + // only be peeled when the method it resolves to holds a live patch. That method + // carries the row's name, so a name no live patch has cannot lead to a peel, and the + // filter spares that row both costs. The name is only a prefilter: a row that passes + // is still resolved exactly, type, parameters and arity included. if (!patchedMethodNames.Contains(unchanged.methodName)) { continue;