Skip to content

Commit

Permalink
[perf] Improve ExportMap.for performance on larger codebases
Browse files Browse the repository at this point in the history
While looking at a larger code base https://gitlab.com/gitlab-org/gitlab,
`ExportMap.for` seems to take a significant amount of time. More than
half of it is spend on calculating hashes with `hashObject`.

Digging a little deeper, it seems like we are calling it around 500
thousand times. Each iteration calculates the hash of a context object.
This context object is created inside of the `childContext` function and
consists of four parts:
- `settings` -> an Object itself
- `parserOptions` -> an Object itself
- `parserPath` -> a String
- `path` -> a String.

Interestingly `settings`, `parserOptions` and `parserPath` rarely do
change for us, so calculating their hashes on every iteration seems
unnecessary. `hashObject` recursively calculates the hashes of each
key / value pair.

So instead of doing:

```js
cacheKey = hashObject({settings, parserOptions, parserPath, path})
```
We could also do:
```js
cacheKey = parserPath +
  hashObject(parserOptions) +
  hashObject(settings) +
  path
```

This would be just as stable as before, although resulting in longer
cache keys. `parserPath` and `path` would not need to be hashed,
because they are strings and a single character change in them would
result in a different cache key.

Furthermore we can memoize the hashes of `parserOptions` and
`settings`, in case they didn't change compared. The equality is
checked with the excellent `fast-deep-equal`.

We move this `cacheKey` calculation to `childContext`, adding the
cache key to the `context` object. This way, we can fall back to the
old calculation inside of `ExportMap.for`, as it is a public
interface which consumers might be using.

In our code base the results speak for itself:
- 16.28s spent in `ExportMap.for`, 0ms spent in `childContext`.
    - More than half is spent in `hashObject`
    - 5.82s is spent in node:crypto/hash `update` (overall)
- 7.21s spent in `ExportMap.for, 284ms spent in `childContext`.
    - Almost no time spent in `hashObject`, actually all calls in
      our flame graph come from other code paths
    - 23.88ms is spent in node:crypto/hash `update` (overall)
    - 257ms is spent in the newly introduced `fast-deep-equal`

So on this machine, and project, we are cutting the execution time
of `ExportMap.for` in half. On machines, which are hashing slower,
the effect might be more pronounced. Similarly machines with less
memory, as the `hashObject` function creates a lot of tiny strings.

One side-effect here could be, that the memoization is in-efficient
if the `settings` or `parserOptions` change often. (I cannot think
of such a scenario, but I am not that versed in the use cases of
this plugin.) But even then, the overhead should mainly be the
equality checks with `fast-deep-equal`.
  • Loading branch information
leipert committed Apr 8, 2023
1 parent 6be042b commit b3beaef
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 1 deletion.
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -107,6 +107,7 @@
"doctrine": "^2.1.0",
"eslint-import-resolver-node": "^0.3.7",
"eslint-module-utils": "^2.7.4",
"fast-deep-equal": "^3.1.3",
"has": "^1.0.3",
"is-core-module": "^2.11.0",
"is-glob": "^4.0.3",
Expand Down
20 changes: 19 additions & 1 deletion src/ExportMap.js
Expand Up @@ -13,6 +13,7 @@ import resolve from 'eslint-module-utils/resolve';
import isIgnored, { hasValidExtension } from 'eslint-module-utils/ignore';

import { hashObject } from 'eslint-module-utils/hash';
import deepEqual from 'fast-deep-equal';
import * as unambiguous from 'eslint-module-utils/unambiguous';

import { tsConfigLoader } from 'tsconfig-paths/lib/tsconfig-loader';
Expand Down Expand Up @@ -305,7 +306,7 @@ ExportMap.get = function (source, context) {
ExportMap.for = function (context) {
const { path } = context;

const cacheKey = hashObject(context).digest('hex');
const cacheKey = context.cacheKey || hashObject(context).digest('hex');
let exportMap = exportCache.get(cacheKey);

// return cached ignore
Expand Down Expand Up @@ -781,12 +782,29 @@ export function recursivePatternCapture(pattern, callback) {
}
}

let parserOptionsHash = '';
let prevParserOptions = null;
let settingsHash = '';
let prevSettings = null;
/**
* don't hold full context object in memory, just grab what we need.
* also calculate a cashKey, where parts of the cacheKey hash are memoized
*/
function childContext(path, context) {
const { settings, parserOptions, parserPath } = context;

if (!deepEqual(settings, prevSettings)){
settingsHash = hashObject({ settings }).digest('hex');
prevSettings = settings;
}

if (!deepEqual(parserOptions, prevParserOptions)){
parserOptionsHash = hashObject({ parserOptions }).digest('hex');
prevParserOptions = parserOptions;
}

return {
cacheKey: String(parserPath) + parserOptionsHash + settingsHash + String(path),
settings,
parserOptions,
parserPath,
Expand Down
5 changes: 5 additions & 0 deletions utils/CHANGELOG.md
Expand Up @@ -5,6 +5,9 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange

## Unreleased

### Fixed
- [Perf] Improve `ExportMap.for` performance on larger codebases ([#2756], thanks [@leipert])

## v2.7.4 - 2022-08-11

### Fixed
Expand Down Expand Up @@ -123,6 +126,7 @@ Yanked due to critical issue with cache key resulting from #839.
### Fixed
- `unambiguous.test()` regex is now properly in multiline mode

[#2756]: https://github.com/import-js/eslint-plugin-import/pull/2756
[#2523]: https://github.com/import-js/eslint-plugin-import/pull/2523
[#2431]: https://github.com/import-js/eslint-plugin-import/pull/2431
[#2350]: https://github.com/import-js/eslint-plugin-import/issues/2350
Expand Down Expand Up @@ -160,6 +164,7 @@ Yanked due to critical issue with cache key resulting from #839.
[@iamnapo]: https://github.com/iamnapo
[@JounQin]: https://github.com/JounQin
[@kaiyoma]: https://github.com/kaiyoma
[@leipert]: https://github.com/leipert
[@manuth]: https://github.com/manuth
[@maxkomarychev]: https://github.com/maxkomarychev
[@mgwalker]: https://github.com/mgwalker
Expand Down

0 comments on commit b3beaef

Please sign in to comment.