From e67a0699918332fa3ee5b06d7bd12253e3ab27a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Sat, 23 Nov 2024 20:10:03 +0100 Subject: [PATCH 1/4] Fix excessive allocations in EmbeddedResourceFileLoader Previously we'd load the entire resource on every invocation of GetControlBuilder, which might be a number of times per request in case of markup controls. --- .../Hosting/EmbeddedMarkupFileLoader.cs | 19 +-- src/Framework/Framework/Hosting/MarkupFile.cs | 8 ++ src/Framework/Testing/DotvvmTestHelper.cs | 12 ++ src/Tests/Runtime/MarkupLoaderTests.cs | 112 ++++++++++++++++++ 4 files changed, 144 insertions(+), 7 deletions(-) create mode 100644 src/Tests/Runtime/MarkupLoaderTests.cs diff --git a/src/Framework/Framework/Hosting/EmbeddedMarkupFileLoader.cs b/src/Framework/Framework/Hosting/EmbeddedMarkupFileLoader.cs index c424c37c7a..05e8e29dcf 100644 --- a/src/Framework/Framework/Hosting/EmbeddedMarkupFileLoader.cs +++ b/src/Framework/Framework/Hosting/EmbeddedMarkupFileLoader.cs @@ -1,12 +1,15 @@ using System; +using System.Buffers; using System.Collections.Generic; using System.IO; using System.Reflection; using System.Text; using DotVVM.Framework.Configuration; +using DotVVM.Framework.Utils; namespace DotVVM.Framework.Hosting { + /// Read markup files from embedded resources, if the virtual path has the following format: embedded://{AssemblyName}/{ResourceName} public class EmbeddedMarkupFileLoader : IMarkupFileLoader { /// @@ -23,7 +26,7 @@ public class EmbeddedMarkupFileLoader : IMarkupFileLoader if (resourceName.IndexOf('/') == -1 || resourceName.IndexOf('/') == 0) { - throw new ArgumentException("Wrong string format", "virtualPath"); + throw new ArgumentException("Wrong embedded file format. Use `embedded://{AssemblyName}/{ResourceName}`", "virtualPath"); } string assemblyName = resourceName.Substring(0, resourceName.IndexOf('/')); @@ -37,20 +40,22 @@ public class EmbeddedMarkupFileLoader : IMarkupFileLoader //no such assembly found catch (FileLoadException) { - throw new ArgumentException("No such assembly was found", "virtualPath"); + throw new ArgumentException($"Assembly '{assemblyName}' was not found", "virtualPath"); } //no such resource found resourceName = resourceName.Replace('/', '.'); if (assembly.GetManifestResourceInfo(resourceName) == null) { - throw new ArgumentException("No such resource was found", "virtualPath"); + throw new ArgumentException($"Resource '{resourceName}' was not found in assembly '{assembly.FullName}'", "virtualPath"); } - //load the file - using (Stream stream = assembly.GetManifestResourceStream(resourceName)!) - using (StreamReader sr = new StreamReader(stream)) - return new MarkupFile(virtualPath, virtualPath, sr.ReadToEnd()); + return new MarkupFile(virtualPath, virtualPath, () => { + //load the file + using (Stream stream = assembly.GetManifestResourceStream(resourceName)!) + using (var reader = new StreamReader(stream)) + return reader.ReadToEnd(); + }); } /// diff --git a/src/Framework/Framework/Hosting/MarkupFile.cs b/src/Framework/Framework/Hosting/MarkupFile.cs index 23d9870730..e7b190a52f 100644 --- a/src/Framework/Framework/Hosting/MarkupFile.cs +++ b/src/Framework/Framework/Hosting/MarkupFile.cs @@ -58,6 +58,14 @@ public MarkupFile(string fileName, string fullPath) }; } + public MarkupFile(string fileName, string fullPath, Func readContent, DateTime lastWriteDateTimeUtc = default) + { + FileName = fileName; + FullPath = fullPath; + ReadContent = readContent; + LastWriteDateTimeUtc = lastWriteDateTimeUtc; + } + public MarkupFile(string fileName, string fullPath, string contents, DateTime lastWriteDateTimeUtc = default) { FileName = fileName; diff --git a/src/Framework/Testing/DotvvmTestHelper.cs b/src/Framework/Testing/DotvvmTestHelper.cs index f265e21006..269b42981f 100644 --- a/src/Framework/Testing/DotvvmTestHelper.cs +++ b/src/Framework/Testing/DotvvmTestHelper.cs @@ -100,11 +100,23 @@ public static void RegisterMockServices(IServiceCollection services) var config = CreateConfiguration(); config.ExperimentalFeatures.UseDotvvmSerializationForStaticCommandArguments.Enable(); config.RouteTable.Add("TestRoute", "TestRoute", "TestView.dothtml"); + config.Diagnostics.Apply(config); config.Freeze(); return config; }); public static DotvvmConfiguration DefaultConfig => _defaultConfig.Value; + private static Lazy _debugConfig = new Lazy(() => { + var config = CreateConfiguration(); + config.ExperimentalFeatures.UseDotvvmSerializationForStaticCommandArguments.Enable(); + config.RouteTable.Add("TestRoute", "TestRoute", "TestView.dothtml"); + config.Debug = true; + config.Diagnostics.Apply(config); + config.Freeze(); + return config; + }); + public static DotvvmConfiguration DebugConfig => _debugConfig.Value; + public static DotvvmConfiguration CreateConfiguration(Action? customServices = null) => DotvvmConfiguration.CreateDefault(s => { s.AddSingleton(); diff --git a/src/Tests/Runtime/MarkupLoaderTests.cs b/src/Tests/Runtime/MarkupLoaderTests.cs new file mode 100644 index 0000000000..1c8eaf1ee4 --- /dev/null +++ b/src/Tests/Runtime/MarkupLoaderTests.cs @@ -0,0 +1,112 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using DotVVM.Framework.Compilation; +using DotVVM.Framework.Configuration; +using DotVVM.Framework.Controls; +using DotVVM.Framework.Hosting; +using DotVVM.Framework.Runtime; +using DotVVM.Framework.Testing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace DotVVM.Framework.Tests.Runtime +{ + [TestClass] + public class MarkupLoaderTests: IDisposable + { + readonly string compilationPageResource = DotvvmTestHelper.DebugConfig.RouteTable[DotvvmCompilationPageConfiguration.DefaultRouteName].VirtualPath; + + readonly List tempFiles = []; + + [TestMethod] + public void EmbeddedResource() + { + var loader = new EmbeddedMarkupFileLoader(); + var file = loader.GetMarkup(DotvvmTestHelper.DebugConfig, compilationPageResource); + + XAssert.StartsWith("@viewModel DotVVM.Framework.Diagnostics.CompilationPageViewModel", file.ReadContent()); + } + +#if DotNetCore + [TestMethod] + public void EmbeddedResourceLazyAllocation() + { + var loader = new EmbeddedMarkupFileLoader(); + // warmup for assembly loading and such + loader.GetMarkup(DotvvmTestHelper.DebugConfig, compilationPageResource); + + // GetMarkup allocates constant memory, as it is being called repeatedly if file reloading is enabled + var a = GC.GetAllocatedBytesForCurrentThread(); + var file = loader.GetMarkup(DotvvmTestHelper.DebugConfig, compilationPageResource); + var b = GC.GetAllocatedBytesForCurrentThread(); + XAssert.InRange(b - a, 0, 1000); + + // ReadContent actually reads the file and allocates the string + a = GC.GetAllocatedBytesForCurrentThread(); + var content = file.ReadContent(); + b = GC.GetAllocatedBytesForCurrentThread(); + XAssert.InRange(content.Length, 1000, int.MaxValue); + XAssert.InRange(b - a, content.Length * 2, content.Length * 5); + } +#endif + + [TestMethod] + [DataRow(true)] + [DataRow(false)] + public void FileReloading(bool debug) + { + var directory = MakeTempDir(); + var file = Path.Combine(directory, "test.dotcontrol"); + File.WriteAllText(file, "@viewModel string\n\n"); + + var config = debug ? DotvvmTestHelper.DebugConfig : DotvvmTestHelper.DefaultConfig; + + var controlBuilder = config.ServiceProvider.GetRequiredService(); + var builder0 = controlBuilder.GetControlBuilder(file); + Assert.AreEqual(typeof(string), builder0.descriptor.DataContextType); + + var builderUnchanged = controlBuilder.GetControlBuilder(file); + + Assert.AreSame(builder0.builder, builderUnchanged.builder); // same Lazy instance + + File.WriteAllText(file, "@viewModel int\n\n"); + + var builderChanged = controlBuilder.GetControlBuilder(file); + var control = builderChanged.builder.Value.BuildControl(config.ServiceProvider.GetRequiredService(), config.ServiceProvider); + if (debug) + { + Assert.AreEqual(typeof(int), builderChanged.descriptor.DataContextType); + Assert.AreNotSame(builder0.builder, builderChanged.builder); // different Lazy instance + XAssert.Equal(["Changed"], control.GetThisAndAllDescendants().OfType().Select(c => c.Text)); + } + else + { + // not reloaded in Release mode by default + Assert.AreEqual(typeof(string), builderChanged.descriptor.DataContextType); + Assert.AreSame(builder0.builder, builderChanged.builder); // different Lazy instance + XAssert.Equal(["Initial"], control.GetThisAndAllDescendants().OfType().Select(c => c.Text)); + } + } + + public string MakeTempDir() + { + var path = Path.Combine(Path.GetTempPath(), "dotvvm-tests-tmp-" + Path.GetRandomFileName()); + Directory.CreateDirectory(path); + tempFiles.Add(path); + return path; + } + + public void Dispose() + { + foreach (var file in tempFiles) + { + if (Directory.Exists(file)) + Directory.Delete(file, true); + else if (File.Exists(file)) + File.Delete(file); + } + } + } +} From c732af6009a77399fa372ad8b46d57190e2819a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Herceg?= Date: Fri, 29 Nov 2024 14:23:51 +0100 Subject: [PATCH 2/4] Tried to add delay in file reloading test --- src/Tests/Runtime/MarkupLoaderTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Tests/Runtime/MarkupLoaderTests.cs b/src/Tests/Runtime/MarkupLoaderTests.cs index 1c8eaf1ee4..25fc987a63 100644 --- a/src/Tests/Runtime/MarkupLoaderTests.cs +++ b/src/Tests/Runtime/MarkupLoaderTests.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Threading; using DotVVM.Framework.Compilation; using DotVVM.Framework.Configuration; using DotVVM.Framework.Controls; @@ -72,6 +73,7 @@ public void FileReloading(bool debug) Assert.AreSame(builder0.builder, builderUnchanged.builder); // same Lazy instance File.WriteAllText(file, "@viewModel int\n\n"); + Thread.Sleep(1000); var builderChanged = controlBuilder.GetControlBuilder(file); var control = builderChanged.builder.Value.BuildControl(config.ServiceProvider.GetRequiredService(), config.ServiceProvider); From 85e94715d5a120d0f1ab61d4bcc698c37130e4cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Fri, 29 Nov 2024 17:16:31 +0100 Subject: [PATCH 3/4] Print message with timestamp to see what the hell is the GA filesystem doing --- src/Tests/Runtime/MarkupLoaderTests.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Tests/Runtime/MarkupLoaderTests.cs b/src/Tests/Runtime/MarkupLoaderTests.cs index 25fc987a63..a4bb7daf0d 100644 --- a/src/Tests/Runtime/MarkupLoaderTests.cs +++ b/src/Tests/Runtime/MarkupLoaderTests.cs @@ -61,24 +61,28 @@ public void FileReloading(bool debug) var directory = MakeTempDir(); var file = Path.Combine(directory, "test.dotcontrol"); File.WriteAllText(file, "@viewModel string\n\n"); + var changedTime1 = File.GetLastWriteTimeUtc(file); var config = debug ? DotvvmTestHelper.DebugConfig : DotvvmTestHelper.DefaultConfig; var controlBuilder = config.ServiceProvider.GetRequiredService(); var builder0 = controlBuilder.GetControlBuilder(file); Assert.AreEqual(typeof(string), builder0.descriptor.DataContextType); - + var builderUnchanged = controlBuilder.GetControlBuilder(file); Assert.AreSame(builder0.builder, builderUnchanged.builder); // same Lazy instance File.WriteAllText(file, "@viewModel int\n\n"); - Thread.Sleep(1000); var builderChanged = controlBuilder.GetControlBuilder(file); var control = builderChanged.builder.Value.BuildControl(config.ServiceProvider.GetRequiredService(), config.ServiceProvider); if (debug) { + var changedTime2 = File.GetLastWriteTimeUtc(file); + if (changedTime1 == changedTime2) + Assert.Fail($"File system resolution is probably too low ({changedTime1:o} == {changedTime2:o}), ignores changes or something."); + Assert.AreEqual(typeof(int), builderChanged.descriptor.DataContextType); Assert.AreNotSame(builder0.builder, builderChanged.builder); // different Lazy instance XAssert.Equal(["Changed"], control.GetThisAndAllDescendants().OfType().Select(c => c.Text)); From c0da866864dcd0dbdcb22b8216450ec07e5c5e16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Sat, 30 Nov 2024 10:36:46 +0100 Subject: [PATCH 4/4] ok whatever, make it inconclusive It failed with: Assert.Fail failed. File system resolution is probably too low (2024-11-29T16:22:57.2569233Z == 2024-11-29T16:22:57.2569233Z), ignores changes or something. --- src/Tests/Runtime/MarkupLoaderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tests/Runtime/MarkupLoaderTests.cs b/src/Tests/Runtime/MarkupLoaderTests.cs index a4bb7daf0d..8a9b5e1c47 100644 --- a/src/Tests/Runtime/MarkupLoaderTests.cs +++ b/src/Tests/Runtime/MarkupLoaderTests.cs @@ -81,7 +81,7 @@ public void FileReloading(bool debug) { var changedTime2 = File.GetLastWriteTimeUtc(file); if (changedTime1 == changedTime2) - Assert.Fail($"File system resolution is probably too low ({changedTime1:o} == {changedTime2:o}), ignores changes or something."); + Assert.Inconclusive($"File system resolution is probably too low ({changedTime1:o} == {changedTime2:o}), ignores changes or something."); Assert.AreEqual(typeof(int), builderChanged.descriptor.DataContextType); Assert.AreNotSame(builder0.builder, builderChanged.builder); // different Lazy instance