From 3be54f0ea00ff4d153a424f1f13d4dd8c574f0a5 Mon Sep 17 00:00:00 2001 From: Stanley Stuart Date: Thu, 13 Aug 2020 08:22:16 -0700 Subject: [PATCH] fix: cache pending/ignore config lookups for faster perf/less memory This commit improves the lookups of `ignore` and `pending` configs by reducing the amount of iterations required over those config properties. tl;dr: - cached the config lookups for `pending` and `ignore` to reduce the amount of times we have to re-iterate the `pending` or `ignore` arrays provided by the config. - cache string lookups for normalized moduleId paths since This was done in three steps: First Step The normalized `moduleId` was cached. Running ember-template-lint on a big codebase (668 template files) took 12.94 seconds to run. I used this command in the app's codebase directory to profile: ``` node --prof ../ember-template-lint/bin/ember-template-lint.js . ``` Next, I generated a graph with the [flamebearer][flamebearer] npm package: ``` node --prof-process --preprocess -j isolate*.log | flamebearer ``` Screen_Shot_2020-08-13_at_9_25_14_AM Profiling showed that ~19% of the CPU time was spent inside of the `statusForModule` function. Upon further investigation, I noticed that the [normalized moduleId based on the current working directory was being generated in a loop][3], creating a new string for potentially every item in the `pending` or `ignore` config. Our app has no `ignore` config, but does have a lot of `pending` entries. Moving the `fullPathModuleId` to a variable outside the loop took the runtime from 12.94 seconds to 11.04 seconds, about ~2 seconds of savings on its own! An important thing to keep in mind for the next two steps: `statusForModule` is called _at least once per rule_ to determine if the rule should be ignored or allowed to fail (or removed from the pending file if now passing). This means that any functions ran or strings created by `statusForModule` are, in the worst case scenario, calculated `numberOfFiles * pendingRuleConfigItems * ignoreConfigItems` times if the loop doesn't find the config for the file early, as it has to search the entire array of `pending` or `ignore` items for every file. Rather than rejoining the paths, they are instead generated once and stored in a cache object. This reduces the number of strings generated and time generating the same strings over and over. Caching the result of `process.cwd` (since it seems unlikely to change once the program is started) potentially has a nice performance side effect for users on Node 10 (supported until April 2021) as [process.cwd was not cached until Node 12.2](https://github.com/nodejs/node/pull/27224). Second Step Lookups for `ignore` and `pending` were separated out into different caches. This was done because while [`statusForModule` will check for a function to run][1] instead of a string, in practice this is only true for `ignore` rules. The reason `ignore` rules need the function check is that they are always a function due to being converted from [strings to functions via the micromatch module][2]. Caching the lookup of `ignore` rules reduced the need to run the functions for every rule/file. Third Step Last, but not least: `pending` rules always seem to be a list of `object`s or `string`s (as generated by `--print-pending`) and having a list that potentially has a function in it doesn't seem likely given that the recommendation from ember-template-lint is to copy/paste the output of `--print-pending`. After re-profiling, the runtime went from 12.94 seconds to 9.04 seconds on my machine. The `get-config` file no longer showed up in the profile. Screen Shot 2020-08-13 at 12 07 40 PM [1]: https://github.com/ember-template-lint/ember-template-lint/blob/5937b63bed30380b4bce0f96b19061658a176840/lib/get-config.js#L376-L377 [2]: https://github.com/ember-template-lint/ember-template-lint/blob/5937b63bed30380b4bce0f96b19061658a176840/lib/get-config.js#L280 [3]: https://github.com/ember-template-lint/ember-template-lint/blob/5937b63bed30380b4bce0f96b19061658a176840/lib/get-config.js#L374 [flamebearer]: https://github.com/mapbox/flamebearer --- lib/get-config.js | 84 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 23 deletions(-) diff --git a/lib/get-config.js b/lib/get-config.js index b21e775974..52c66582ec 100644 --- a/lib/get-config.js +++ b/lib/get-config.js @@ -362,47 +362,85 @@ function _determineConfigForSeverity(config) { } } -function statusForModule(type, config, options) { - let moduleId = options.moduleId; - let list = config[type]; - let configPath = options.configPath || ''; - if (!list) { - return false; +class ModuleStatusCache { + constructor(config, configPath) { + this.config = config; + this.configPath = configPath || ''; + this.cache = { + pending: {}, + ignore: {}, + }; } - for (const item of list) { - let fullPathModuleId = path.resolve(process.cwd(), moduleId); + lookupPending(moduleId) { + if (!moduleId || !this.config.pending) { + return false; + } + if (!this.cache.pendingLookup) { + this.cache.pendingLookup = this._extractPendingCache(); + } + if (!(moduleId in this.cache.pending)) { + const fullPathModuleId = path.resolve(process.cwd(), moduleId); + this.cache.pending[moduleId] = this.cache.pendingLookup[fullPathModuleId]; + } + return this.cache.pending[moduleId]; + } - if (typeof item === 'function' && item(moduleId)) { - return true; - } else if (typeof item === 'string') { - let fullPathItem = path.resolve(process.cwd(), path.dirname(configPath), item); - if (fullPathModuleId === fullPathItem) { - return true; - } - } else if (item.moduleId) { - let fullPathItem = path.resolve(process.cwd(), path.dirname(configPath), item.moduleId); - if (fullPathModuleId === fullPathItem) { - return item; - } + lookupIgnore(moduleId) { + if (!(moduleId in this.cache.ignore)) { + const ignores = this.config['ignore'] || []; + this.cache.ignore[moduleId] = ignores.find((match) => match(moduleId)); } + return Boolean(this.cache.ignore[moduleId]); } - return false; + _extractPendingCache() { + const list = this.config.pending; + const byFullModuleId = {}; + + if (!list) { + return byFullModuleId; + } + + for (const item of list) { + if (typeof item === 'string') { + const fullPath = this.resolveFullModuleId(item); + byFullModuleId[fullPath] = true; + } else if (item.moduleId) { + const fullPath = this.resolveFullModuleId(item.moduleId); + byFullModuleId[fullPath] = item; + } + } + + return byFullModuleId; + } + resolveFullModuleId(moduleId) { + if (!this._baseDirBasedOnConfigPath) { + this._baseDirBasedOnConfigPath = path.resolve(process.cwd(), path.dirname(this.configPath)); + } + return path.resolve(this._baseDirBasedOnConfigPath, moduleId); + } } + +let configModuleCacheMap = new WeakMap(); + /** * Returns the config in conjunction with overrides configuration. * @param {*} config * @param {*} filePath */ function getConfigForFile(config, options) { + if (!configModuleCacheMap.has(config)) { + configModuleCacheMap.set(config, new ModuleStatusCache(config, options.configPath)); + } + let moduleStatusCache = configModuleCacheMap.get(config); let filePath = options.filePath; let configuredRules = config.rules; let overrides = config.overrides; let fileConfig = Object.assign({}, config, { - pendingStatus: statusForModule('pending', config, options), - shouldIgnore: statusForModule('ignore', config, options), + pendingStatus: moduleStatusCache.lookupPending(options.moduleId), + shouldIgnore: moduleStatusCache.lookupIgnore(options.moduleId), }); if (filePath && overrides) {