From 0270fb933f228f8b7bf306a8440338e8c7ddf1ad Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 16:16:41 -0700 Subject: [PATCH 1/8] run a full resolveAll eagerly now that it's optimized --- src/root.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/root.js b/src/root.js index fa6e10be0..ee5b5fd3e 100644 --- a/src/root.js +++ b/src/root.js @@ -51,7 +51,7 @@ Root.fromJSON = function fromJSON(json, root) { root = new Root(); if (json.options) root.setOptions(json.options); - return root.addJSON(json.nested)._resolveFeaturesRecursive(); + return root.addJSON(json.nested).resolveAll(); }; /** @@ -100,7 +100,7 @@ Root.prototype.load = function load(filename, options, callback) { // Finishes loading by calling the callback (exactly once) function finish(err, root) { if (root) { - root._resolveFeaturesRecursive(); + root.resolveAll(); } /* istanbul ignore if */ if (!callback) { @@ -219,7 +219,7 @@ Root.prototype.load = function load(filename, options, callback) { if (resolved = self.resolvePath("", filename[i])) fetch(resolved); if (sync) { - self._resolveFeaturesRecursive(); + self.resolveAll(); return self; } if (!queued) { From a2e0dac8f3b69b9dc0b3ff8314fa2364d3f59f4f Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 16:28:12 -0700 Subject: [PATCH 2/8] add caching to prevent excessive recursion on resolveAll --- src/namespace.js | 12 ++++++++++++ src/root.js | 2 ++ src/service.js | 2 ++ src/type.js | 2 ++ 4 files changed, 18 insertions(+) diff --git a/src/namespace.js b/src/namespace.js index 2a79f296a..57430d618 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -124,6 +124,13 @@ function Namespace(name, options) { * @protected */ this._needsRecursiveFeatureResolution = true; + + /** + * Whether or not objects contained in this namespace need a resolve. + * @type {boolean} + * @protected + */ + this._needsRecursiveResolve = true; } function clearCache(namespace) { @@ -273,11 +280,13 @@ Namespace.prototype.add = function add(object) { } this._needsRecursiveFeatureResolution = true; + this._needsRecursiveResolve = true; // Also clear parent caches, since they need to recurse down. var parent = this; while(parent = parent.parent) { parent._needsRecursiveFeatureResolution = true; + parent._needsRecursiveResolve = true; } object.onAdd(this); @@ -341,6 +350,8 @@ Namespace.prototype.define = function define(path, json) { * @returns {Namespace} `this` */ Namespace.prototype.resolveAll = function resolveAll() { + if (!this._needsRecursiveResolve) return this; + var nested = this.nestedArray, i = 0; this.resolve(); while (i < nested.length) @@ -348,6 +359,7 @@ Namespace.prototype.resolveAll = function resolveAll() { nested[i++].resolveAll(); else nested[i++].resolve(); + this._needsRecursiveResolve = false; return this; }; diff --git a/src/root.js b/src/root.js index ee5b5fd3e..d5c3285af 100644 --- a/src/root.js +++ b/src/root.js @@ -268,6 +268,8 @@ Root.prototype.loadSync = function loadSync(filename, options) { * @override */ Root.prototype.resolveAll = function resolveAll() { + if (!this._needsRecursiveResolve) return this; + if (this.deferred.length) throw Error("unresolvable extensions: " + this.deferred.map(function(field) { return "'extend " + field.extend + "' in " + field.parent.fullName; diff --git a/src/service.js b/src/service.js index a7be2f483..504674382 100644 --- a/src/service.js +++ b/src/service.js @@ -110,6 +110,8 @@ Service.prototype.get = function get(name) { * @override */ Service.prototype.resolveAll = function resolveAll() { + if (!this._needsRecursiveResolve) return this; + Namespace.prototype.resolve.call(this); var methods = this.methodsArray; for (var i = 0; i < methods.length; ++i) diff --git a/src/type.js b/src/type.js index c11fc6d65..978baf94f 100644 --- a/src/type.js +++ b/src/type.js @@ -303,6 +303,8 @@ Type.prototype.toJSON = function toJSON(toJSONOptions) { * @override */ Type.prototype.resolveAll = function resolveAll() { + if (!this._needsRecursiveResolve) return this; + Namespace.prototype.resolveAll.call(this); var oneofs = this.oneofsArray; i = 0; while (i < oneofs.length) From 9e3aec6dc5553ffaf06df7468744640f351f987c Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 16:30:50 -0700 Subject: [PATCH 3/8] build ts file --- index.d.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/index.d.ts b/index.d.ts index fb96caec1..7cbb4d5bd 100644 --- a/index.d.ts +++ b/index.d.ts @@ -753,6 +753,9 @@ export abstract class NamespaceBase extends ReflectionObject { /** Whether or not objects contained in this namespace need feature resolution. */ protected _needsRecursiveFeatureResolution: boolean; + /** Whether or not objects contained in this namespace need a resolve. */ + protected _needsRecursiveResolve: boolean; + /** Nested objects of this namespace as an array for iteration. */ public readonly nestedArray: ReflectionObject[]; From 7f1e3082d1c4d10ce3d71786c6b66ee2d235f993 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 19:06:23 -0700 Subject: [PATCH 4/8] minor 20% optimization to lookup --- src/namespace.js | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/namespace.js b/src/namespace.js index 57430d618..73d5cacc0 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -404,26 +404,36 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe // Start at root if path is absolute if (path[0] === "") return this.root.lookup(path.slice(1), filterTypes); + var flatPath = path.join("."); - var found = this._lookupImpl(path); + var found = this._lookupImpl(path, flatPath); if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) { return found; } - // If there hasn't been a match, try again at the parent - if (this.parent === null || parentAlreadyChecked) + // If there hasn't been a match, walk up the tree + if (parentAlreadyChecked) return null; - return this.parent.lookup(path, filterTypes); + + var current = this; + while (current.parent) { + found = current.parent._lookupImpl(path, flatPath); + if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) { + return found; + } + current = current.parent; + } + return null; }; /** * Internal helper for lookup that handles searching just at this namespace and below along with caching. * @param {string[]} path Path to look up + * @param {string} flatPath Flattened version of the path to use as a cache key * @returns {ReflectionObject|null} Looked up object or `null` if none could be found * @private */ -Namespace.prototype._lookupImpl = function lookup(path) { - var flatPath = path.join("."); +Namespace.prototype._lookupImpl = function lookup(path, flatPath) { if(Object.prototype.hasOwnProperty.call(this._lookupCache, flatPath)) { return this._lookupCache[flatPath]; } @@ -434,13 +444,15 @@ Namespace.prototype._lookupImpl = function lookup(path) { if (found) { if (path.length === 1) { exact = found; - } else if (found instanceof Namespace && (found = found._lookupImpl(path.slice(1)))) - exact = found; + } else if (found instanceof Namespace) { + path = path.slice(1); + exact = found._lookupImpl(path, path.join(".")); + } // Otherwise try each nested namespace } else { for (var i = 0; i < this.nestedArray.length; ++i) - if (this._nestedArray[i] instanceof Namespace && (found = this._nestedArray[i]._lookupImpl(path))) + if (this._nestedArray[i] instanceof Namespace && (found = this._nestedArray[i]._lookupImpl(path, flatPath))) exact = found; } From 7edfcab770d8360547a06c7f4064f6eda8d9b040 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 19:13:43 -0700 Subject: [PATCH 5/8] Cleanup to mark fields private --- index.d.ts | 15 --------------- src/object.js | 4 ++++ src/root.js | 6 +++++- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/index.d.ts b/index.d.ts index 7cbb4d5bd..9161111ed 100644 --- a/index.d.ts +++ b/index.d.ts @@ -903,21 +903,6 @@ export abstract class ReflectionObject { /** Unique name within its namespace. */ public name: string; - /** The edition specified for this object. Only relevant for top-level objects. */ - public _edition: string; - - /** - * The default edition to use for this object if none is specified. For legacy reasons, - * this is proto2 except in the JSON parsing case where it was proto3. - */ - public _defaultEdition: string; - - /** Resolved Features. */ - public _features: object; - - /** Whether or not features have been resolved. */ - public _featuresResolved: boolean; - /** Parent namespace. */ public parent: (Namespace|null); diff --git a/src/object.js b/src/object.js index 0ea6a2334..8eb2310b6 100644 --- a/src/object.js +++ b/src/object.js @@ -51,6 +51,7 @@ function ReflectionObject(name, options) { /** * The edition specified for this object. Only relevant for top-level objects. * @type {string} + * @private */ this._edition = null; @@ -58,18 +59,21 @@ function ReflectionObject(name, options) { * The default edition to use for this object if none is specified. For legacy reasons, * this is proto2 except in the JSON parsing case where it was proto3. * @type {string} + * @private */ this._defaultEdition = "proto2"; /** * Resolved Features. * @type {object} + * @private */ this._features = {}; /** * Whether or not features have been resolved. * @type {boolean} + * @private */ this._featuresResolved = false; diff --git a/src/root.js b/src/root.js index d5c3285af..882c958be 100644 --- a/src/root.js +++ b/src/root.js @@ -36,7 +36,11 @@ function Root(options) { */ this.files = []; - // Default to proto2 if unspecified. + /** + * Edition, defaults to proto2 if unspecified. + * @type {string} + * @private + */ this._edition = "proto2"; } From 33f5fb6f79dfbdc2398d66940f51150acce34a11 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 19:28:30 -0700 Subject: [PATCH 6/8] further 40% optimization to lookup that bails out early on fully-qualified types --- src/namespace.js | 14 +++++++++++--- src/root.js | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/namespace.js b/src/namespace.js index 73d5cacc0..b46dc54d3 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -401,20 +401,28 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe } else if (!path.length) return this; + var flatPath = path.join("."); + // Start at root if path is absolute if (path[0] === "") return this.root.lookup(path.slice(1), filterTypes); - var flatPath = path.join("."); + + // Early bailout for objects with matching absolute paths + var found = this.root._fullyQualifiedObjects["." + flatPath]; + if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) { + return found; + } - var found = this._lookupImpl(path, flatPath); + // Do a regular lookup at this namespace and below + found = this._lookupImpl(path, flatPath); if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) { return found; } - // If there hasn't been a match, walk up the tree if (parentAlreadyChecked) return null; + // If there hasn't been a match, walk up the tree and look more broadly var current = this; while (current.parent) { found = current.parent._lookupImpl(path, flatPath); diff --git a/src/root.js b/src/root.js index 882c958be..9cda0e4eb 100644 --- a/src/root.js +++ b/src/root.js @@ -42,6 +42,13 @@ function Root(options) { * @private */ this._edition = "proto2"; + + /** + * Global lookup cache of fully qualified names. + * @type {Object.} + * @private + */ + this._fullyQualifiedObjects = {}; } /** @@ -341,6 +348,11 @@ Root.prototype._handleAdd = function _handleAdd(object) { object.parent[object.name] = object; // expose namespace as property of its parent } + if (object instanceof Type || object instanceof Enum) { + // Only store types and enums for quick lookup during resolve. + this._fullyQualifiedObjects[object.fullName] = object; + } + // The above also adds uppercased (and thus conflict-free) nested types, services and enums as // properties of namespaces just like static code does. This allows using a .d.ts generated for // a static module with reflection-based solutions where the condition is met. @@ -381,6 +393,8 @@ Root.prototype._handleRemove = function _handleRemove(object) { delete object.parent[object.name]; // unexpose namespaces } + + delete this._fullyQualifiedObjects[object.fullName]; }; // Sets up cyclic dependencies (called in index-light) From f813f8ef07eb94c4bac526ed5d585ab6855b824c Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 19:29:27 -0700 Subject: [PATCH 7/8] lint fix --- src/namespace.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/namespace.js b/src/namespace.js index b46dc54d3..305e700b6 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -406,7 +406,7 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe // Start at root if path is absolute if (path[0] === "") return this.root.lookup(path.slice(1), filterTypes); - + // Early bailout for objects with matching absolute paths var found = this.root._fullyQualifiedObjects["." + flatPath]; if (found && (!filterTypes || filterTypes.indexOf(found.constructor) > -1)) { From 54fedf0457616689c5b9dd80c957976f0951dbdb Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Tue, 13 May 2025 11:14:00 -0700 Subject: [PATCH 8/8] Include fields in global fullname cache for custom options --- src/root.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/root.js b/src/root.js index 9cda0e4eb..7f0927996 100644 --- a/src/root.js +++ b/src/root.js @@ -348,7 +348,7 @@ Root.prototype._handleAdd = function _handleAdd(object) { object.parent[object.name] = object; // expose namespace as property of its parent } - if (object instanceof Type || object instanceof Enum) { + if (object instanceof Type || object instanceof Enum || object instanceof Field) { // Only store types and enums for quick lookup during resolve. this._fullyQualifiedObjects[object.fullName] = object; }