From 407e80e7ca352ae7cfc48778ce658b6eaa6ad137 Mon Sep 17 00:00:00 2001 From: zodern Date: Mon, 29 Mar 2021 09:47:52 -0500 Subject: [PATCH 1/6] Check for changes once per name --- lib/runtime/entry.js | 141 +++++++++++++++++++++++++++---------------- 1 file changed, 90 insertions(+), 51 deletions(-) diff --git a/lib/runtime/entry.js b/lib/runtime/entry.js index 272052a0..2591dbf1 100644 --- a/lib/runtime/entry.js +++ b/lib/runtime/entry.js @@ -26,6 +26,9 @@ function Entry(id) { // The normalized namespace object that importers receive when they use // `import * as namespace from "..."` syntax. this.namespace = utils.createNamespace(); + + this._lastValues = Object.create(null); + this._uninitializedSetters = []; } var Ep = utils.setPrototypeOf(Entry.prototype, null); @@ -98,6 +101,12 @@ Ep.addSetters = function (parent, setters, key) { entry.setters[name] = Object.create(null); } entry.setters[name][key] = setter; + + this._uninitializedSetters.push({ + name: name, + key: key, + setter: setter + }); } } @@ -214,7 +223,8 @@ Ep.runSetters = function (names) { // module objects whose setters we might need to run. var parents; - forEachSetter(this, names, function (setter, name, value) { + + function runSetter(setter, name, value) { if (parents === void 0) { parents = Object.create(null); } @@ -223,7 +233,23 @@ Ep.runSetters = function (names) { // The param order for setters is `value` then `name` because the `name` // param is only used by namespace exports. setter(value, name); - }); + } + + forEachSetter(this, names, undefined, runSetter); + + if (this._uninitializedSetters.length > 0) { + var initNames = []; + var initKeys = []; + this._uninitializedSetters.forEach(function (config) { + if (config.setter.initialized) { + return; + } + initNames.push(config.name); + initKeys.push(config.key); + }); + this._uninitializedSetters.length = 0; + forEachSetter(this, initNames, initKeys, runSetter); + } if (! parents) { return; @@ -254,64 +280,32 @@ function callSetterIfNecessary(setter, name, value, callback) { return; } - var shouldCall = false; + setter.initialized = true; + return callback(setter, name, value); +} - if (setter.last === void 0) { - setter.last = Object.create(null); - // Always call the setter if it has never been called before. - shouldCall = true; +function changed(last, prop, value) { + var valueToCompare = value; + if (valueToCompare !== valueToCompare) { + valueToCompare = NAN; + } else if (valueToCompare === void 0) { + valueToCompare = UNDEFINED; } - function changed(name, value) { - var valueToCompare = value; - if (valueToCompare !== valueToCompare) { - valueToCompare = NAN; - } else if (valueToCompare === void 0) { - valueToCompare = UNDEFINED; - } - - if (setter.last[name] === valueToCompare) { - return false; - } - - setter.last[name] = valueToCompare; - return true; + if (last[prop] === valueToCompare) { + return false; } - if (name === "*") { - var keys = safeKeys(value); - var keyCount = keys.length; - for (var i = 0; i < keyCount; ++i) { - var key = keys[i]; - // Evaluating value[key] is risky because the property might be - // defined by a getter function that logs a deprecation warning (or - // worse) when evaluated. For example, Node uses this trick to - // display a deprecation warning whenever crypto.createCredentials - // is accessed. Fortunately, when value[key] is defined by a getter - // function, it's enough to check whether the getter function itself - // has changed, since we are careful elsewhere to preserve getters - // rather than prematurely evaluating them. - if (changed(key, utils.valueOrGetter(value, key))) { - shouldCall = true; - } - } - } else if (changed(name, value)) { - shouldCall = true; - } - - if (shouldCall) { - // Only invoke the callback if we have not called this setter - // (with a value of this name) before, or the current value is - // different from the last value we passed to this setter. - return callback(setter, name, value); - } + last[prop] = valueToCompare; + return true; } // Invoke the given callback once for every (setter, name, value) that needs to // be called. Note that forEachSetter does not call any setters itself, only the // given callback. -function forEachSetter(entry, names, callback) { +function forEachSetter(entry, names, keys, callback) { var needToCheckNames = true; + var keysProvided = keys !== void 0; if (names === void 0) { names = Object.keys(entry.setters); @@ -328,13 +322,58 @@ function forEachSetter(entry, names, callback) { continue; } + var value = getExportByName(entry, name); + var shouldCall = false; + + if (!hasOwn.call(entry._lastValues, name)) { + entry._lastValues[name] = Object.create(null); + } + + var last = entry._lastValues[name]; + + if (!shouldCall && name === "*") { + var keysToCheck = safeKeys(value); + var keyCount = keysToCheck.length; + for (var x = 0; x < keyCount; ++x) { + var key = keysToCheck[x]; + // Evaluating value[key] is risky because the property might be + // defined by a getter function that logs a deprecation warning (or + // worse) when evaluated. For example, Node uses this trick to + // display a deprecation warning whenever crypto.createCredentials + // is accessed. Fortunately, when value[key] is defined by a getter + // function, it's enough to check whether the getter function itself + // has changed, since we are careful elsewhere to preserve getters + // rather than prematurely evaluating them. + if (changed(last, key, utils.valueOrGetter(value, key))) { + shouldCall = true; + } + } + } else if (!shouldCall && changed(last, name, value)) { + shouldCall = true; + } + + // Only invoke the callback if the current value is + // different from the last value we passed to this setter. + // If we need to run setters for the first time even if the value didn't change, + // their keys should be provided to force them to be called + if (!shouldCall && !keysProvided) { + continue; + } + var setters = entry.setters[name]; - var keys = Object.keys(setters); + if (!keysProvided) { + keys = Object.keys(setters); + } + var keyCount = keys.length; for (var j = 0; j < keyCount; ++j) { var key = keys[j]; - var value = getExportByName(entry, name); + + if (keysProvided && + !hasOwn.call(setters, key)) { + continue; + } callSetterIfNecessary(setters[key], name, value, callback); From 5f4844d657b2d10795566bd6747e3c44fbb3f5ba Mon Sep 17 00:00:00 2001 From: zodern Date: Mon, 29 Mar 2021 10:04:50 -0500 Subject: [PATCH 2/6] Add fast path for re-exporting values When re-exporting values, the values can not be used within the module. This allows us to skip running all getters since we know exactly which exports were modified. --- lib/runtime/entry.js | 35 +++++++++++++++++++++++++++++++---- lib/runtime/index.js | 8 +++++++- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lib/runtime/entry.js b/lib/runtime/entry.js index 2591dbf1..8e99a78a 100644 --- a/lib/runtime/entry.js +++ b/lib/runtime/entry.js @@ -214,21 +214,45 @@ function syncExportsToNamespace(entry, names) { // Called whenever module.exports might have changed, to trigger any // setters associated with the newly exported values. The names parameter // is optional; without it, all getters and setters will run. -Ep.runSetters = function (names) { +// If the '*' setter needs to be run, but not the '*' getter (names includes +// all exports/getters that changed), the runNsSetter option can be enabled. +Ep.runSetters = function (names, runNsSetter) { // Make sure entry.namespace and module.exports are up to date before we // call getExportByName(entry, name). this.runGetters(names); + if (runNsSetter && names !== void 0) { + names.push('*'); + } + // Lazily-initialized object mapping parent module identifiers to parent // module objects whose setters we might need to run. var parents; - + var parentNames; function runSetter(setter, name, value) { if (parents === void 0) { parents = Object.create(null); } - parents[setter.parent.id] = setter.parent; + if (parentNames === void 0) { + parentNames = Object.create(null); + } + + var parentId = setter.parent.id; + + // When setters use the shorthand for re-exporting values, we know + // which exports in the parent module were modified, and can do less work + // when running the parent setters. + // parentNames[parentId] is set to false if there are any setters that we do + // not know which exports they modify + if (setter.exportAs !== void 0 && parentNames[parentId] !== false) { + parentNames[parentId] = parentNames[parentId] || []; + parentNames[parentId].push(setter.exportAs); + } else if (parentNames[parentId] !== false) { + parentNames[parentId] = false; + } + + parents[parentId] = setter.parent; // The param order for setters is `value` then `name` because the `name` // param is only used by namespace exports. @@ -269,7 +293,10 @@ Ep.runSetters = function (names) { var parent = parents[parentIDs[i]]; var parentEntry = entryMap[parent.id]; if (parentEntry) { - parentEntry.runSetters(); + parentEntry.runSetters( + parentNames[parentIDs[i]] || void 0, + !!parentNames[parentIDs[i]] + ); } } }; diff --git a/lib/runtime/index.js b/lib/runtime/index.js index fe8bc453..76efab54 100644 --- a/lib/runtime/index.js +++ b/lib/runtime/index.js @@ -94,7 +94,7 @@ function moduleExportDefault(value) { function moduleExportAs(name) { var entry = this; var includeDefault = name === "*+"; - return function (value) { + var setter = function (value) { if (name === "*" || name === "*+") { Object.keys(value).forEach(function (key) { if (includeDefault || key !== "default") { @@ -105,6 +105,12 @@ function moduleExportAs(name) { entry.exports[name] = value; } }; + + if (name !== '*+' && name !== "*") { + setter.exportAs = name; + } + + return setter; } // Platform-specific code should find a way to call this method whenever From d3710a3941e5d5d93f444732e07e197e49207017 Mon Sep 17 00:00:00 2001 From: zodern Date: Mon, 29 Mar 2021 11:46:23 -0500 Subject: [PATCH 3/6] Use void instead of undefined --- lib/runtime/entry.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/runtime/entry.js b/lib/runtime/entry.js index 8e99a78a..7537b8a8 100644 --- a/lib/runtime/entry.js +++ b/lib/runtime/entry.js @@ -259,7 +259,7 @@ Ep.runSetters = function (names, runNsSetter) { setter(value, name); } - forEachSetter(this, names, undefined, runSetter); + forEachSetter(this, names, void 0, runSetter); if (this._uninitializedSetters.length > 0) { var initNames = []; From 78b2fa94ebe86663e3af5638dcae613963d11f1c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 12 Apr 2021 16:40:09 -0400 Subject: [PATCH 4/6] Record both setter.snapshot and entry.snapshots[name]. To make things a bit clearer, I renamed entry._lastValues to entry.snapshots and setter.last to setter.snapshot, and I factored out the snapshotting logic into a helper function called updateSnapshot, which replaces the changed function from before. Recording entry._lastValues[name] was a good idea (in addition to recording setter.last for each setter function), because it allows us to determine if the value has changed before looking at any of the setter functions. However, I think we still need to record setter.snapshot, in order to be sure we pass any new results to every applicable setter. If the value has changed, then we need to pass it to every setter function and update each setter.snapshot property to the current snapshot. If the value has not changed, we still have to loop through the setter functions to make sure they've all received an initial value, which requires recording/checking setter.snapshot for each setter function. We could potentially skip this work if we know no new setter functions have been added since we last broadcast a value, but (unless this becomes a performance problem) I prefer the safety of storing setter.snapshot on each setter function. Either way, the new snapshot becomes the setter.snapshot object for every setter registered for the given name, which is much more efficient than recomputing a separate setter.last snapshot for each setter, as we were doing before PR #246. --- lib/runtime/entry.js | 194 ++++++++++++++++++++----------------------- 1 file changed, 88 insertions(+), 106 deletions(-) diff --git a/lib/runtime/entry.js b/lib/runtime/entry.js index 7537b8a8..27f915cb 100644 --- a/lib/runtime/entry.js +++ b/lib/runtime/entry.js @@ -26,8 +26,9 @@ function Entry(id) { // The normalized namespace object that importers receive when they use // `import * as namespace from "..."` syntax. this.namespace = utils.createNamespace(); - - this._lastValues = Object.create(null); + // Map from local names to snapshots of the corresponding local values, used + // to determine when local values have changed and need to be re-broadcast. + this.snapshots = Object.create(null); this._uninitializedSetters = []; } @@ -259,20 +260,22 @@ Ep.runSetters = function (names, runNsSetter) { setter(value, name); } - forEachSetter(this, names, void 0, runSetter); + forEachSetter(this, names, runSetter); if (this._uninitializedSetters.length > 0) { var initNames = []; var initKeys = []; this._uninitializedSetters.forEach(function (config) { - if (config.setter.initialized) { + // The presence of setter.snapshot implies this setter has already + // received at least one value, and is thus no longer uninitialized. + if (config.setter.snapshot) { return; } initNames.push(config.name); initKeys.push(config.key); }); this._uninitializedSetters.length = 0; - forEachSetter(this, initNames, initKeys, runSetter); + forEachSetter(this, initNames, runSetter); } if (! parents) { @@ -301,127 +304,106 @@ Ep.runSetters = function (names, runNsSetter) { } }; -function callSetterIfNecessary(setter, name, value, callback) { - if (name === "__esModule") { - // Ignore setters asking for module.exports.__esModule. - return; - } +function updateSnapshot(entry, name, newValue) { + var newSnapshot = Object.create(null); + var newKeys = []; - setter.initialized = true; - return callback(setter, name, value); -} - -function changed(last, prop, value) { - var valueToCompare = value; - if (valueToCompare !== valueToCompare) { - valueToCompare = NAN; - } else if (valueToCompare === void 0) { - valueToCompare = UNDEFINED; + if (name === "*") { + safeKeys(newValue).forEach(keyOfValue => { + // Evaluating value[key] is risky because the property might be + // defined by a getter function that logs a deprecation warning (or + // worse) when evaluated. For example, Node uses this trick to display + // a deprecation warning whenever crypto.createCredentials is + // accessed. Fortunately, when value[key] is defined by a getter + // function, it's enough to check whether the getter function itself + // has changed, since we are careful elsewhere to preserve getters + // rather than prematurely evaluating them. + newKeys.push(keyOfValue); + newSnapshot[keyOfValue] = normalizeSnapshotValue( + utils.valueOrGetter(newValue, keyOfValue) + ); + }); + } else { + newKeys.push(name); + newSnapshot[name] = normalizeSnapshotValue(newValue); } - if (last[prop] === valueToCompare) { - return false; + var oldSnapshot = entry.snapshots[name]; + if ( + oldSnapshot && + newKeys.every(key => oldSnapshot[key] === newSnapshot[key]) && + newKeys.length === Object.keys(oldSnapshot).length + ) { + return oldSnapshot; } - last[prop] = valueToCompare; - return true; + return entry.snapshots[name] = newSnapshot; +} + +function normalizeSnapshotValue(value) { + if (value === void 0) return UNDEFINED; + if (value !== value && isNaN(value)) return NAN; + return value; } // Invoke the given callback once for every (setter, name, value) that needs to // be called. Note that forEachSetter does not call any setters itself, only the // given callback. -function forEachSetter(entry, names, keys, callback) { - var needToCheckNames = true; - var keysProvided = keys !== void 0; - +function forEachSetter(entry, names, callback) { if (names === void 0) { names = Object.keys(entry.setters); - needToCheckNames = false; } - var nameCount = names.length; - - for (var i = 0; i < nameCount; ++i) { - var name = names[i]; + names.forEach(name => { + // Ignore setters asking for module.exports.__esModule. + if (name === "__esModule") return; - if (needToCheckNames && - ! hasOwn.call(entry.setters, name)) { - continue; - } + var settersByKey = entry.setters[name]; + if (!settersByKey) return; var value = getExportByName(entry, name); - var shouldCall = false; - if (!hasOwn.call(entry._lastValues, name)) { - entry._lastValues[name] = Object.create(null); - } - - var last = entry._lastValues[name]; - - if (!shouldCall && name === "*") { - var keysToCheck = safeKeys(value); - var keyCount = keysToCheck.length; - for (var x = 0; x < keyCount; ++x) { - var key = keysToCheck[x]; - // Evaluating value[key] is risky because the property might be - // defined by a getter function that logs a deprecation warning (or - // worse) when evaluated. For example, Node uses this trick to - // display a deprecation warning whenever crypto.createCredentials - // is accessed. Fortunately, when value[key] is defined by a getter - // function, it's enough to check whether the getter function itself - // has changed, since we are careful elsewhere to preserve getters - // rather than prematurely evaluating them. - if (changed(last, key, utils.valueOrGetter(value, key))) { - shouldCall = true; + // Although we may have multiple setter functions with different keys in + // settersByKey, we can compute a snapshot of value and check it against + // entry.snapshots[name] before iterating over the individual setter + // functions, which is convenient because then all the setter.snapshot + // properties will end up referring to the same snapshot object. + var snapshot = updateSnapshot(entry, name, value); + + Object.keys(settersByKey).forEach(key => { + var setter = settersByKey[key]; + + // If value has not changed since the last time we broadcast it, then + // snapshot === entry.snapshots[name], so there's a good chance we can + // skip most/all of the setters that already have setter.snapshot === + // snapshot. If value has changed, snapshot !== entry.snapshots[name], and + // we need to broadcast the new value to every setter. + if (setter.snapshot !== snapshot) { + setter.snapshot = snapshot; + + // Invoke the setter function with the updated value. + callback(setter, name, value); + + // TODO Refactor this out of the settersByKey loop, if possible. + var getter = entry.getters[name]; + if (typeof getter === "function" && + // Sometimes a getter function will throw because it's called + // before the variable it's supposed to return has been + // initialized, so we need to know that the getter function has + // run to completion at least once. + getter.runCount > 0 && + getter.constant) { + // If we happen to know that this getter function has run + // successfully, and will never return a different value, then we + // can forget the corresponding setter, because we've already + // reported that constant value. Note that we can't forget the + // getter, because we need to remember the original value in case + // anyone tampers with entry.module.exports[name]. + delete settersByKey[key]; } } - } else if (!shouldCall && changed(last, name, value)) { - shouldCall = true; - } - - // Only invoke the callback if the current value is - // different from the last value we passed to this setter. - // If we need to run setters for the first time even if the value didn't change, - // their keys should be provided to force them to be called - if (!shouldCall && !keysProvided) { - continue; - } - - var setters = entry.setters[name]; - if (!keysProvided) { - keys = Object.keys(setters); - } - - var keyCount = keys.length; - - for (var j = 0; j < keyCount; ++j) { - var key = keys[j]; - - if (keysProvided && - !hasOwn.call(setters, key)) { - continue; - } - - callSetterIfNecessary(setters[key], name, value, callback); - - var getter = entry.getters[name]; - if (typeof getter === "function" && - // Sometimes a getter function will throw because it's called - // before the variable it's supposed to return has been - // initialized, so we need to know that the getter function has - // run to completion at least once. - getter.runCount > 0 && - getter.constant) { - // If we happen to know that this getter function has run - // successfully, and will never return a different value, then we - // can forget the corresponding setter, because we've already - // reported that constant value. Note that we can't forget the - // getter, because we need to remember the original value in case - // anyone tampers with entry.module.exports[name]. - delete setters[key]; - } - } - } + }); + }); } function getExportByName(entry, name) { From 8021e62d5f3b669a35459ff28db07c9baf1a6e6a Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 12 Apr 2021 16:40:09 -0400 Subject: [PATCH 5/6] Stop tracking entry._uninitializedSetters. This logic was slightly off, since it combined all the config.name strings in one initNames array and all the config.key strings in one initKeys array, then passed those arrays to runSetters, even though not all keys are applicable for all names. Fortunately, tracking uninitialized setters should no longer be necessary, since we make sure every setter receives the latest value (using the setter.snapshot property to prevent duplicates). --- lib/runtime/entry.js | 30 +++--------------------------- 1 file changed, 3 insertions(+), 27 deletions(-) diff --git a/lib/runtime/entry.js b/lib/runtime/entry.js index 27f915cb..36e7983a 100644 --- a/lib/runtime/entry.js +++ b/lib/runtime/entry.js @@ -29,7 +29,6 @@ function Entry(id) { // Map from local names to snapshots of the corresponding local values, used // to determine when local values have changed and need to be re-broadcast. this.snapshots = Object.create(null); - this._uninitializedSetters = []; } var Ep = utils.setPrototypeOf(Entry.prototype, null); @@ -102,12 +101,6 @@ Ep.addSetters = function (parent, setters, key) { entry.setters[name] = Object.create(null); } entry.setters[name][key] = setter; - - this._uninitializedSetters.push({ - name: name, - key: key, - setter: setter - }); } } @@ -231,10 +224,11 @@ Ep.runSetters = function (names, runNsSetter) { var parents; var parentNames; - function runSetter(setter, name, value) { + forEachSetter(this, names, function (setter, name, value) { if (parents === void 0) { parents = Object.create(null); } + if (parentNames === void 0) { parentNames = Object.create(null); } @@ -258,25 +252,7 @@ Ep.runSetters = function (names, runNsSetter) { // The param order for setters is `value` then `name` because the `name` // param is only used by namespace exports. setter(value, name); - } - - forEachSetter(this, names, runSetter); - - if (this._uninitializedSetters.length > 0) { - var initNames = []; - var initKeys = []; - this._uninitializedSetters.forEach(function (config) { - // The presence of setter.snapshot implies this setter has already - // received at least one value, and is thus no longer uninitialized. - if (config.setter.snapshot) { - return; - } - initNames.push(config.name); - initKeys.push(config.key); - }); - this._uninitializedSetters.length = 0; - forEachSetter(this, initNames, runSetter); - } + }); if (! parents) { return; From 23a6c729ece002d4b5f5de66345f79c8b2aa5f11 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 12 Apr 2021 19:14:24 -0400 Subject: [PATCH 6/6] Extract alreadyCalledConstantGetter from inner loop. As promised by my TODO comment. --- lib/runtime/entry.js | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/lib/runtime/entry.js b/lib/runtime/entry.js index 36e7983a..f1b6a78e 100644 --- a/lib/runtime/entry.js +++ b/lib/runtime/entry.js @@ -337,6 +337,16 @@ function forEachSetter(entry, names, callback) { var settersByKey = entry.setters[name]; if (!settersByKey) return; + var getter = entry.getters[name]; + var alreadyCalledConstantGetter = + typeof getter === "function" && + // Sometimes a getter function will throw because it's called + // before the variable it's supposed to return has been + // initialized, so we need to know that the getter function has + // run to completion at least once. + getter.runCount > 0 && + getter.constant; + var value = getExportByName(entry, name); // Although we may have multiple setter functions with different keys in @@ -360,21 +370,13 @@ function forEachSetter(entry, names, callback) { // Invoke the setter function with the updated value. callback(setter, name, value); - // TODO Refactor this out of the settersByKey loop, if possible. - var getter = entry.getters[name]; - if (typeof getter === "function" && - // Sometimes a getter function will throw because it's called - // before the variable it's supposed to return has been - // initialized, so we need to know that the getter function has - // run to completion at least once. - getter.runCount > 0 && - getter.constant) { - // If we happen to know that this getter function has run - // successfully, and will never return a different value, then we - // can forget the corresponding setter, because we've already - // reported that constant value. Note that we can't forget the - // getter, because we need to remember the original value in case - // anyone tampers with entry.module.exports[name]. + if (alreadyCalledConstantGetter) { + // If we happen to know this getter function has run successfully + // (getter.runCount > 0), and will never return a different value + // (getter.constant), then we can forget the corresponding setter, + // because we've already reported that constant value. Note that we + // can't forget the getter, because we need to remember the original + // value in case anyone tampers with entry.module.exports[name]. delete settersByKey[key]; } }