diff --git a/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs b/Assets/Tests/Editor/HotReload/HotReloadEntryResolutionTests.cs index 386579953b..7358220a6f 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, @@ -91,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 @@ -114,6 +157,7 @@ public void ResolveEntries_WhenShimMethodIsMissing_FailsTheFileAtomically() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -158,6 +202,7 @@ public void ResolveEntries_WhenAddedMethodHasNoUsableCounter_FailsTheFileAtomica HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -197,6 +242,7 @@ public void ResolveEntries_WhenEntryIsAnAddedMethod_ResolvesWithoutAnOriginalMet HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -233,6 +279,7 @@ public void ResolveEntries_RecordsTheAddedMembersEachEntryCalls() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -273,6 +320,7 @@ public void ResolveEntries_WhenACallNamesNoAddedEntry_FailsTheFileAtomically() HotReloadEntryResolution.Result result = HotReloadEntryResolution.ResolveEntries( TestAssemblyHome, FileHomeResolver, + _matcher.Resolve, FilePath, ShimAssembly, entries, @@ -310,6 +358,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..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; @@ -197,6 +198,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 +229,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..d260abffa2 100644 --- a/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs +++ b/Packages/src/Editor/FirstPartyTools/HotReload/Patching/HotReloadMethodMatcher.cs @@ -1,3 +1,5 @@ +using System; +using System.Collections.Generic; using System.IO; using System.Reflection; @@ -15,20 +17,60 @@ 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. + /// 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 static class HotReloadMethodMatcher + 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 + /// 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( + /// The matcher was disposed: its run is over. + /// The loader returned no image. + public HotReloadMethodMatchResult Resolve( HotReloadTypeHome home, string typeMetadataName, string methodName, @@ -41,18 +83,37 @@ public static 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)); } - // 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); + 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) @@ -79,6 +140,23 @@ public static 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( TypeDefinition typeDefinition, string methodName,