diff --git a/Directory.Packages.props b/Directory.Packages.props index 4d80612a67a..fd90caa42d3 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -6,7 +6,7 @@ - + diff --git a/src/Docfx.Build/Docfx.Build.csproj b/src/Docfx.Build/Docfx.Build.csproj index c097f058543..7ea4fddf800 100644 --- a/src/Docfx.Build/Docfx.Build.csproj +++ b/src/Docfx.Build/Docfx.Build.csproj @@ -2,6 +2,7 @@ + diff --git a/src/Docfx.Build/TemplateProcessors/Preprocessors/JintProcessorHelper.cs b/src/Docfx.Build/TemplateProcessors/Preprocessors/JintProcessorHelper.cs index 5ff49f798cb..7a9fd707b4d 100644 --- a/src/Docfx.Build/TemplateProcessors/Preprocessors/JintProcessorHelper.cs +++ b/src/Docfx.Build/TemplateProcessors/Preprocessors/JintProcessorHelper.cs @@ -11,12 +11,14 @@ public static JsValue ConvertObjectToJsValue(Jint.Engine engine, object raw) { if (raw is IDictionary dict) { - var jsObject = new JsObject(engine); - foreach (var pair in dict) - { - jsObject.FastSetDataProperty(pair.Key, ConvertObjectToJsValue(engine, pair.Value)); - } - return jsObject; + // Build the object directly in the engine's hidden class ("shape") representation rather than + // storing a property descriptor per property. Every document of a given document type is handed + // to the template with the same property layout, so the objects built here end up sharing one + // interned shape and the property reads in the template script stay monomorphic across documents. + // + // Layouts the representation cannot express - integer-index-like keys, very wide objects - fall + // back to the ordinary property-dictionary representation silently and correctly. + return JsObject.CreateFromEntries(engine, ConvertEntries(engine, dict)); } if (raw is IList list) @@ -33,4 +35,17 @@ public static JsValue ConvertObjectToJsValue(Jint.Engine engine, object raw) return JsValue.FromObject(engine, raw); } + + /// + /// Streams the converted entries instead of materializing them into an array first: the entries are + /// enumerated exactly once while the object is being built, so a model with many properties does not + /// need a temporary buffer of its own size. + /// + private static IEnumerable> ConvertEntries(Jint.Engine engine, IDictionary dict) + { + foreach (var pair in dict) + { + yield return new KeyValuePair(pair.Key, ConvertObjectToJsValue(engine, pair.Value)); + } + } } diff --git a/src/Docfx.Build/TemplateProcessors/Preprocessors/PreprocessorLoader.cs b/src/Docfx.Build/TemplateProcessors/Preprocessors/PreprocessorLoader.cs index 9e57166cdfd..72dcc4dc980 100644 --- a/src/Docfx.Build/TemplateProcessors/Preprocessors/PreprocessorLoader.cs +++ b/src/Docfx.Build/TemplateProcessors/Preprocessors/PreprocessorLoader.cs @@ -1,9 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Concurrent; using System.Text.RegularExpressions; +using Acornima.Ast; + using Docfx.Common; +using Jint; namespace Docfx.Build.Engine; @@ -58,7 +62,10 @@ public ITemplatePreprocessor Load(ResourceInfo res, string name = null) var extension = Path.GetExtension(res.Path); if (extension.Equals(TemplateJintPreprocessor.Extension, System.StringComparison.OrdinalIgnoreCase)) { - return new PreprocessorWithResourcePool(() => new TemplateJintPreprocessor(_reader, res, _context, name), _maxParallelism); + // Every preprocessor the pool creates for this template runs the same script sources, + // so let them share one parsed copy of each. + var preparedScripts = new ConcurrentDictionary>(); + return new PreprocessorWithResourcePool(() => new TemplateJintPreprocessor(_reader, res, _context, name, preparedScripts), _maxParallelism); } else { diff --git a/src/Docfx.Build/TemplateProcessors/Preprocessors/TemplateJintPreprocessor.cs b/src/Docfx.Build/TemplateProcessors/Preprocessors/TemplateJintPreprocessor.cs index dc69af76437..d66da9022b5 100644 --- a/src/Docfx.Build/TemplateProcessors/Preprocessors/TemplateJintPreprocessor.cs +++ b/src/Docfx.Build/TemplateProcessors/Preprocessors/TemplateJintPreprocessor.cs @@ -1,6 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Concurrent; + +using Acornima.Ast; + using Docfx.Common; using Jint; using Jint.Native; @@ -57,7 +61,42 @@ public class TemplateJintPreprocessor : ITemplatePreprocessor private const string NullString = "null"; + /// + /// Wall clock budget for a single preprocessor call, that is one getOptions or + /// transform for one document. Without it an accidental infinite loop in a template script + /// hangs a build thread forever instead of failing the document. + /// + private static readonly TimeSpan DefaultExecutionTimeout = TimeSpan.FromSeconds(30); + + /// + /// Statement budget for a single preprocessor call. Deliberately far above what transforming even a + /// very large model costs, so it only ever catches a runaway script. A saturated value such as + /// would register no limit at all, so this has to be a real number. + /// + private const int MaxExecutionStatements = 50_000_000; + private object _utilityObject; + + private readonly CancellationToken _cancellationToken; + + private readonly TimeSpan _executionTimeout; + + /// + /// Every engine this preprocessor owns, keyed by resource path: the template's engine under the + /// template's own path, plus one per required module. A preprocessor instance is rented from + /// PreprocessorWithResourcePool for the duration of one call, so this is only ever touched by + /// one thread at a time. + /// + private readonly Dictionary _engineCache = []; + + /// + /// Parsed sources, keyed by resource path, shared by every preprocessor instance created for the same + /// template. A is immutable and safe to execute from several engines + /// and threads, so the template and everything it requires is parsed once instead of once per + /// engine in the preprocessor pool. + /// + private readonly ConcurrentDictionary> _preparedScripts; + private static readonly object ConsoleObject = new { log = new Action(s => Logger.Log(s ?? NullString)), @@ -72,7 +111,22 @@ public class TemplateJintPreprocessor : ITemplatePreprocessor private Func _getOptionsFunc; public TemplateJintPreprocessor(ResourceFileReader resourceCollection, ResourceInfo scriptResource, DocumentBuildContext context, string name = null) + : this(resourceCollection, scriptResource, context, name, new ConcurrentDictionary>()) + { + } + + internal TemplateJintPreprocessor( + ResourceFileReader resourceCollection, + ResourceInfo scriptResource, + DocumentBuildContext context, + string name, + ConcurrentDictionary> preparedScripts, + TimeSpan? executionTimeout = null) { + _preparedScripts = preparedScripts; + _cancellationToken = context?.CancellationToken ?? CancellationToken.None; + _executionTimeout = executionTimeout ?? DefaultExecutionTimeout; + if (!string.IsNullOrWhiteSpace(scriptResource.Content)) { SetupEngine(resourceCollection, scriptResource, context); @@ -96,6 +150,7 @@ public object GetOptions(object model) { if (_getOptionsFunc != null) { + ResetConstraints(); return _getOptionsFunc(model); } @@ -106,16 +161,38 @@ public object TransformModel(object model) { if (_transformFunc != null) { + ResetConstraints(); return _transformFunc(model); } return model; } + /// + /// Rearms the timeout and clears the statement counter on every engine this preprocessor owns, so all + /// three limits bound one document rather than the lifetime of a pooled preprocessor. + /// + /// Only the template's own engine is entered through a public Jint API - , + /// which resets that engine's constraints itself. A required module's exported function belongs to + /// the module's engine, so calling it is an ordinary function call that charges the module + /// engine's constraint instances without ever going through an entry point that resets them. Since a + /// timeout deadline is armed only on reset and a statement counter is only cleared on reset, leaving + /// them alone would give a module engine a deadline fixed at preprocessor construction and a statement + /// budget spanning the whole build. + /// + /// + private void ResetConstraints() + { + foreach (var engine in _engineCache.Values) + { + engine.Constraints.Reset(); + } + } + private Jint.Engine SetupEngine(ResourceFileReader resourceCollection, ResourceInfo scriptResource, DocumentBuildContext context) { var rootPath = (RelativePath)scriptResource.Path; - var engineCache = new Dictionary(); + var engineCache = _engineCache; var utility = new TemplateUtility(context); _utilityObject = new @@ -125,37 +202,40 @@ private Jint.Engine SetupEngine(ResourceFileReader resourceCollection, ResourceI markup = new Func(utility.Markup), }; - var engine = CreateDefaultEngine(); + // Each engine registers `require` from this delegate itself. Copying the function object out of one + // engine and into another - which is what `CreateEngine(engine, RequireFuncVariableName)` used to do - + // hands a JsValue to an engine that did not create it; a JsValue holds a hard reference to the engine + // and realm that created it and passing one across is not a supported arrangement. + object Require(string s) + { + if (!s.StartsWith(RequireRelativePathPrefix, StringComparison.Ordinal)) + { + throw new ArgumentException($"Only relative path starting with `{RequireRelativePathPrefix}` is supported in require"); + } + var relativePath = (RelativePath)s.Substring(RequireRelativePathPrefix.Length); + s = relativePath.BasedOn(rootPath); + + var script = resourceCollection?.GetResource(s); + if (string.IsNullOrWhiteSpace(script)) + { + return null; + } - var requireAction = new Func( - s => + if (!engineCache.TryGetValue(s, out Jint.Engine cachedEngine)) { - if (!s.StartsWith(RequireRelativePathPrefix, StringComparison.Ordinal)) - { - throw new ArgumentException($"Only relative path starting with `{RequireRelativePathPrefix}` is supported in require"); - } - var relativePath = (RelativePath)s.Substring(RequireRelativePathPrefix.Length); - s = relativePath.BasedOn(rootPath); - - var script = resourceCollection?.GetResource(s); - if (string.IsNullOrWhiteSpace(script)) - { - return null; - } - - if (!engineCache.TryGetValue(s, out Jint.Engine cachedEngine)) - { - cachedEngine = CreateEngine(engine, RequireFuncVariableName); - engineCache[s] = cachedEngine; - cachedEngine.Execute(script, s); - } - - return cachedEngine.GetValue(ExportsVariableName); - }); - - engine.SetValue(RequireFuncVariableName, requireAction); + cachedEngine = CreateDefaultEngine(); + cachedEngine.SetValue(RequireFuncVariableName, (Func)Require); + engineCache[s] = cachedEngine; + cachedEngine.Execute(Prepare(s, script)); + } + + return cachedEngine.GetValue(ExportsVariableName); + } + + var engine = CreateDefaultEngine(); + engine.SetValue(RequireFuncVariableName, (Func)Require); engineCache[rootPath] = engine; - engine.Execute(scriptResource.Content, scriptResource.Path); + engine.Execute(Prepare(scriptResource.Path, scriptResource.Content)); var value = engine.GetValue(ExportsVariableName); if (value.IsObject()) @@ -172,27 +252,39 @@ private Jint.Engine SetupEngine(ResourceFileReader resourceCollection, ResourceI return engine; } - private Jint.Engine CreateEngine(Jint.Engine engine, params string[] sharedVariables) + /// + /// Parses once per and reuses the result. The + /// preprocessor pool builds one instance - and therefore one engine - per parallelism slot, and they + /// all run the same sources, so without this the template and every module it requires would be + /// re-parsed for each slot. + /// + private Prepared