From cb92b16c487347a8216c6a41fc105ed11e1ff570 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Tue, 6 May 2025 16:33:06 -0700 Subject: [PATCH 1/9] Add benchmarks covering resolveAll --- bench/index.js | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/bench/index.js b/bench/index.js index 7a8a55fbe..593bd3c3b 100644 --- a/bench/index.js +++ b/bench/index.js @@ -10,7 +10,8 @@ // in between. var newSuite = require("./suite"), - payload = require("./data/bench.json"); + payload = require("./data/bench.json"), + protobuf = require(".."); var Buffer_from = Buffer.from !== Uint8Array.from && Buffer.from || function(value, encoding) { return new Buffer(value, encoding); }; @@ -88,3 +89,41 @@ newSuite("combined") jspbCls.deserializeBinary(jspbMsg.serializeBinary()); }) .run(); + +var json = require("../tests/data/test.json"); +var fromJsonRoot = protobuf.Root.fromJSON(json); +var fromJsonIndex = 1; + +newSuite("fromJSON") +.add("isolated", function() { + protobuf.Root.fromJSON(json); +}) +.add("isolated (resolveAll)", function() { + protobuf.Root.fromJSON(json).resolveAll(); +}) +.add("shared (unique)", function() { + var jsonCopy = { + options: json.options, + nested: {} + }; + Object.keys(json).forEach(key => { + jsonCopy.nested[key + fromJsonIndex] = json[key] + }); + fromJsonIndex++; + + protobuf.Root.fromJSON(jsonCopy, fromJsonRoot); +}).run(); + +var resolveAllRoot = protobuf.Root.fromJSON(json); + +newSuite("resolveAll") +.add("isolated", function() { + resolveAllRoot.resolveAll(); +}).run(); + +newSuite("load") +.add("sync", function() { + protobuf.loadSync("bench/data/bench.proto"); +}) +.run() + From 94629543a168d31863a44149c980b5dd674a00dd Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Tue, 6 May 2025 19:57:28 -0700 Subject: [PATCH 2/9] Improve shared benchmark --- bench/index.js | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/bench/index.js b/bench/index.js index 593bd3c3b..a1a4d97ec 100644 --- a/bench/index.js +++ b/bench/index.js @@ -91,9 +91,6 @@ newSuite("combined") .run(); var json = require("../tests/data/test.json"); -var fromJsonRoot = protobuf.Root.fromJSON(json); -var fromJsonIndex = 1; - newSuite("fromJSON") .add("isolated", function() { protobuf.Root.fromJSON(json); @@ -102,20 +99,21 @@ newSuite("fromJSON") protobuf.Root.fromJSON(json).resolveAll(); }) .add("shared (unique)", function() { - var jsonCopy = { - options: json.options, - nested: {} - }; - Object.keys(json).forEach(key => { - jsonCopy.nested[key + fromJsonIndex] = json[key] - }); - fromJsonIndex++; - - protobuf.Root.fromJSON(jsonCopy, fromJsonRoot); + var root = protobuf.Root.fromJSON(json); + for (var i = 0; i < 1000; ++i) { + var jsonCopy = { + options: json.options, + nested: {} + }; + Object.keys(json).forEach(key => { + jsonCopy.nested[key + i] = json[key]; + }); + + protobuf.Root.fromJSON(jsonCopy, root); + } }).run(); var resolveAllRoot = protobuf.Root.fromJSON(json); - newSuite("resolveAll") .add("isolated", function() { resolveAllRoot.resolveAll(); From d6eb4e0c3d62e053eb085d6534f9de699b12f1e5 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Tue, 6 May 2025 19:24:33 -0700 Subject: [PATCH 3/9] Rework feature resolution to not rely on resolved types --- bench/index.js | 2 +- package.json | 4 +++- src/field.js | 12 +++++++++--- src/root.js | 10 +++++----- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/bench/index.js b/bench/index.js index a1a4d97ec..2ce8982d0 100644 --- a/bench/index.js +++ b/bench/index.js @@ -123,5 +123,5 @@ newSuite("load") .add("sync", function() { protobuf.loadSync("bench/data/bench.proto"); }) -.run() +.run(); diff --git a/package.json b/package.json index 717b5f105..df40083fe 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,9 @@ "build:bundle": "gulp --gulpfile scripts/gulpfile.js", "build:types": "node cli/bin/pbts --main --global protobuf --out index.d.ts src/ lib/aspromise/index.js lib/base64/index.js lib/codegen/index.js lib/eventemitter/index.js lib/float/index.js lib/fetch/index.js lib/inquire/index.js lib/path/index.js lib/pool/index.js lib/utf8/index.js", "changelog": "node scripts/changelog -w", - "coverage": "nyc tape -r ./lib/tape-adapter tests/*.js tests/node/*.js", + "coverage": "npm run coverage:test && npm run coverage:report", + "coverage:test": "nyc --silent tape -r ./lib/tape-adapter tests/*.js tests/node/*.js", + "coverage:report": "nyc report --reporter=lcov --reporter=text", "docs": "jsdoc -c config/jsdoc.json -R README.md --verbose --pedantic", "lint": "npm run lint:sources && npm run lint:types", "lint:sources": "eslint \"**/*.js\" -c config/eslint.json", diff --git a/src/field.js b/src/field.js index b71969e2e..72d8f63e0 100644 --- a/src/field.js +++ b/src/field.js @@ -370,12 +370,18 @@ Field.prototype._inferLegacyProtoFeatures = function _inferLegacyProtoFeatures(e } var features = {}; - this.resolve(); + if (this.rule === "required") { features.field_presence = "LEGACY_REQUIRED"; } - if (this.resolvedType instanceof Type && this.resolvedType.group) { - features.message_encoding = "DELIMITED"; + if (this.parent && types.defaults[this.type] === undefined) { + // We can't use resolvedType because types may not have been resolved yet. However, + // legacy groups are always in the same scope as the field so we don't have to do a + // full scan of the tree. + var type = this.parent.get(this.type.split(".").pop()); + if (type && type instanceof Type && type.group) { + features.message_encoding = "DELIMITED"; + } } if (this.getOption("packed") === true) { features.repeated_field_encoding = "PACKED"; diff --git a/src/root.js b/src/root.js index 8e3c72c4a..a926567c0 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).resolveAll(); + return root.addJSON(json.nested)._resolveFeaturesRecursive(); }; /** @@ -99,6 +99,9 @@ Root.prototype.load = function load(filename, options, callback) { // Finishes loading by calling the callback (exactly once) function finish(err, root) { + if (root) { + root._resolveFeaturesRecursive(); + } /* istanbul ignore if */ if (!callback) { return; @@ -108,9 +111,6 @@ Root.prototype.load = function load(filename, options, callback) { } var cb = callback; callback = null; - if (root) { - root.resolveAll(); - } cb(err, root); } @@ -218,8 +218,8 @@ Root.prototype.load = function load(filename, options, callback) { for (var i = 0, resolved; i < filename.length; ++i) if (resolved = self.resolvePath("", filename[i])) fetch(resolved); - self.resolveAll(); if (sync) { + self._resolveFeaturesRecursive(); return self; } if (!queued) { From cac7293d2a3330ad3b481f35f91a5c9a058b211d Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Tue, 6 May 2025 20:16:04 -0700 Subject: [PATCH 4/9] Fix lint warning --- bench/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/bench/index.js b/bench/index.js index 2ce8982d0..dfe260b65 100644 --- a/bench/index.js +++ b/bench/index.js @@ -105,6 +105,7 @@ newSuite("fromJSON") options: json.options, nested: {} }; + // eslint-disable-next-line no-loop-func Object.keys(json).forEach(key => { jsonCopy.nested[key + i] = json[key]; }); From 9b5c845e19fb579ab5a26c0e48ea1a134f16448e Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Wed, 7 May 2025 14:18:47 -0700 Subject: [PATCH 5/9] Cache feature resolution state to avoid potential O(n^2) work. Features should never change --- src/object.js | 13 ++++++++++++- src/root.js | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/object.js b/src/object.js index bf367e267..8608810ba 100644 --- a/src/object.js +++ b/src/object.js @@ -67,6 +67,12 @@ function ReflectionObject(name, options) { */ this._features = {}; + /** + * Whether or not features have been resolved. + * @type {boolean} + * / + this._featuresResolved = false; + /** * Parent namespace. * @type {Namespace|null} @@ -173,7 +179,6 @@ ReflectionObject.prototype.resolve = function resolve() { if (this.resolved) return this; if (this instanceof Root) { - this._resolveFeaturesRecursive(this._edition); this.resolved = true; } return this; @@ -194,6 +199,10 @@ ReflectionObject.prototype._resolveFeaturesRecursive = function _resolveFeatures * @returns {undefined} */ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition) { + if (this._featuresResolved) { + return; + } + var defaults = {}; /* istanbul ignore if */ @@ -217,6 +226,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition) throw new Error("Unknown edition: " + edition); } this._features = Object.assign(defaults, protoFeatures || {}); + this._featuresResolved = true; return; } @@ -238,6 +248,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition) // Sister fields should have the same features as their extensions. this.extensionField._features = this._features; } + this._featuresResolved = true; }; /** diff --git a/src/root.js b/src/root.js index a926567c0..fa6e10be0 100644 --- a/src/root.js +++ b/src/root.js @@ -272,6 +272,7 @@ Root.prototype.resolveAll = function resolveAll() { throw Error("unresolvable extensions: " + this.deferred.map(function(field) { return "'extend " + field.extend + "' in " + field.parent.fullName; }).join(", ")); + this._resolveFeaturesRecursive(this._edition); return Namespace.prototype.resolveAll.call(this); }; From 1982e52e728826bccfedbc291bad76a0c7966e90 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Wed, 7 May 2025 19:58:21 -0700 Subject: [PATCH 6/9] Restore caching of object resolution that was accidentally removed --- src/object.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/object.js b/src/object.js index 8608810ba..a83850468 100644 --- a/src/object.js +++ b/src/object.js @@ -178,9 +178,8 @@ ReflectionObject.prototype.onRemove = function onRemove(parent) { ReflectionObject.prototype.resolve = function resolve() { if (this.resolved) return this; - if (this instanceof Root) { - this.resolved = true; - } + if (this.root instanceof Root) + this.resolved = true; // only if part of a root return this; }; From e9b488ffdef35789df413f13390185791ac3cb95 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Wed, 7 May 2025 21:33:59 -0700 Subject: [PATCH 7/9] Add caching to namespace lookup to drastically improve speed during resolveAll (30x in benchmarks) --- src/namespace.js | 69 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 15 deletions(-) diff --git a/src/namespace.js b/src/namespace.js index 51ba2bd24..0cb18d210 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -108,10 +108,27 @@ function Namespace(name, options) { * @private */ this._nestedArray = null; + + /** + * Cache lookup calls for any objects contains anywhere under this namespace. + * This drastically speeds up resolve for large cross-linked protos where the same + * types are looked up repeatedly. + * @type {Object.} + * @private + */ + this._lookupCache = {}; } function clearCache(namespace) { namespace._nestedArray = null; + namespace._lookupCache = {}; + + // Also clear parent caches, since they include nested lookups. + var parent = namespace.parent; + while(parent) { + parent._lookupCache = {}; + parent = parent.parent; + } return namespace; } @@ -341,7 +358,6 @@ Namespace.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursi * @returns {ReflectionObject|null} Looked up object or `null` if none could be found */ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChecked) { - /* istanbul ignore next */ if (typeof filterTypes === "boolean") { parentAlreadyChecked = filterTypes; @@ -360,25 +376,48 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe if (path[0] === "") return this.root.lookup(path.slice(1), filterTypes); - // Test if the first part matches any nested object, and if so, traverse if path contains more - var found = this.get(path[0]); - if (found) { - if (path.length === 1) { - if (!filterTypes || filterTypes.indexOf(found.constructor) > -1) - return found; - } else if (found instanceof Namespace && (found = found.lookup(path.slice(1), filterTypes, true))) - return found; - - // 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].lookup(path, filterTypes, true))) - return found; + var found = this._lookupImpl(path); + 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) return null; return this.parent.lookup(path, filterTypes); +} + +/** + * Internal helper for lookup that handles searching just at this namespace and below along with caching. + * @param {string[]} path Path to look up + * @returns {ReflectionObject|null} Looked up object or `null` if none could be found + * @private + */ +Namespace.prototype._lookupImpl = function lookup(path) { + var flatPath = path.join("."); + if(this._lookupCache.hasOwnProperty(flatPath)) { + return this._lookupCache[flatPath]; + } else { + // Test if the first part matches any nested object, and if so, traverse if path contains more + var found = this.get(path[0]); + var exact = null; + if (found) { + if (path.length === 1) { + exact = found; + } else if (found instanceof Namespace && (found = found._lookupImpl(path.slice(1)))) + exact = found; + + // 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))) + exact = found; + } + + // Set this even when null, so that when we walk up the tree we can quickly bail on repeated checks back down. + this._lookupCache[flatPath] = exact; + return exact; + } }; /** From db3c953f131cd30acd446d6a71d9e8365319dd84 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Wed, 7 May 2025 21:38:24 -0700 Subject: [PATCH 8/9] Fix lint issues --- index.d.ts | 3 +++ src/namespace.js | 40 ++++++++++++++++++++-------------------- src/object.js | 2 +- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/index.d.ts b/index.d.ts index 6e211631d..58b2b28c4 100644 --- a/index.d.ts +++ b/index.d.ts @@ -909,6 +909,9 @@ export abstract class ReflectionObject { /** 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/namespace.js b/src/namespace.js index 0cb18d210..b70da6759 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -385,7 +385,7 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe if (this.parent === null || parentAlreadyChecked) return null; return this.parent.lookup(path, filterTypes); -} +}; /** * Internal helper for lookup that handles searching just at this namespace and below along with caching. @@ -395,29 +395,29 @@ Namespace.prototype.lookup = function lookup(path, filterTypes, parentAlreadyChe */ Namespace.prototype._lookupImpl = function lookup(path) { var flatPath = path.join("."); - if(this._lookupCache.hasOwnProperty(flatPath)) { + if(Object.prototype.hasOwnProperty.call(this._lookupCache, flatPath)) { return this._lookupCache[flatPath]; - } else { - // Test if the first part matches any nested object, and if so, traverse if path contains more - var found = this.get(path[0]); - var exact = null; - if (found) { - if (path.length === 1) { - exact = found; - } else if (found instanceof Namespace && (found = found._lookupImpl(path.slice(1)))) - exact = found; + } - // 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))) - exact = found; - } + // Test if the first part matches any nested object, and if so, traverse if path contains more + var found = this.get(path[0]); + var exact = null; + if (found) { + if (path.length === 1) { + exact = found; + } else if (found instanceof Namespace && (found = found._lookupImpl(path.slice(1)))) + exact = found; - // Set this even when null, so that when we walk up the tree we can quickly bail on repeated checks back down. - this._lookupCache[flatPath] = exact; - return exact; + // 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))) + exact = found; } + + // Set this even when null, so that when we walk up the tree we can quickly bail on repeated checks back down. + this._lookupCache[flatPath] = exact; + return exact; }; /** diff --git a/src/object.js b/src/object.js index a83850468..0ea6a2334 100644 --- a/src/object.js +++ b/src/object.js @@ -70,7 +70,7 @@ function ReflectionObject(name, options) { /** * Whether or not features have been resolved. * @type {boolean} - * / + */ this._featuresResolved = false; /** From 0db0152f8e7927a656b5f9ed852971b9bf9d0d50 Mon Sep 17 00:00:00 2001 From: Mike Kruskal <62662355+mkruskal-google@users.noreply.github.com> Date: Thu, 8 May 2025 09:46:35 -0700 Subject: [PATCH 9/9] Cache feature resolution at the namespace level to avoid any O(n^2) scaling --- index.d.ts | 3 +++ src/namespace.js | 23 ++++++++++++++++++++--- src/service.js | 2 ++ src/type.js | 2 ++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/index.d.ts b/index.d.ts index 58b2b28c4..fb96caec1 100644 --- a/index.d.ts +++ b/index.d.ts @@ -750,6 +750,9 @@ export abstract class NamespaceBase extends ReflectionObject { /** Nested objects by name. */ public nested?: { [k: string]: ReflectionObject }; + /** Whether or not objects contained in this namespace need feature resolution. */ + protected _needsRecursiveFeatureResolution: boolean; + /** Nested objects of this namespace as an array for iteration. */ public readonly nestedArray: ReflectionObject[]; diff --git a/src/namespace.js b/src/namespace.js index b70da6759..2a79f296a 100644 --- a/src/namespace.js +++ b/src/namespace.js @@ -117,6 +117,13 @@ function Namespace(name, options) { * @private */ this._lookupCache = {}; + + /** + * Whether or not objects contained in this namespace need feature resolution. + * @type {boolean} + * @protected + */ + this._needsRecursiveFeatureResolution = true; } function clearCache(namespace) { @@ -124,10 +131,9 @@ function clearCache(namespace) { namespace._lookupCache = {}; // Also clear parent caches, since they include nested lookups. - var parent = namespace.parent; - while(parent) { + var parent = namespace; + while(parent = parent.parent) { parent._lookupCache = {}; - parent = parent.parent; } return namespace; } @@ -266,6 +272,14 @@ Namespace.prototype.add = function add(object) { } } + this._needsRecursiveFeatureResolution = true; + + // Also clear parent caches, since they need to recurse down. + var parent = this; + while(parent = parent.parent) { + parent._needsRecursiveFeatureResolution = true; + } + object.onAdd(this); return clearCache(this); }; @@ -341,6 +355,9 @@ Namespace.prototype.resolveAll = function resolveAll() { * @override */ Namespace.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursive(edition) { + if (!this._needsRecursiveFeatureResolution) return this; + this._needsRecursiveFeatureResolution = false; + edition = this._edition || edition; ReflectionObject.prototype._resolveFeaturesRecursive.call(this, edition); diff --git a/src/service.js b/src/service.js index 2c11ec9e3..a7be2f483 100644 --- a/src/service.js +++ b/src/service.js @@ -121,6 +121,8 @@ Service.prototype.resolveAll = function resolveAll() { * @override */ Service.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursive(edition) { + if (!this._needsRecursiveFeatureResolution) return this; + edition = this._edition || edition; Namespace.prototype._resolveFeaturesRecursive.call(this, edition); diff --git a/src/type.js b/src/type.js index 40c9f7868..c11fc6d65 100644 --- a/src/type.js +++ b/src/type.js @@ -317,6 +317,8 @@ Type.prototype.resolveAll = function resolveAll() { * @override */ Type.prototype._resolveFeaturesRecursive = function _resolveFeaturesRecursive(edition) { + if (!this._needsRecursiveFeatureResolution) return this; + edition = this._edition || edition; Namespace.prototype._resolveFeaturesRecursive.call(this, edition);