cache-require-paths

[ayumi]

Add cache-require-paths.

BL version	1st start	Subsequent starts
master 0147755	770 ms	770 ms
feature/require-faster 9ffc7ee	790 ms	700 ms

The first start is slightly slower (maybe due to writing the cache) but subsequent starts are faster.

Test plan:

1. Try packaged build on MacOS, Linux and Windows.
2. Try dev on MacOS, Linux and Windows.

Measuring perf

1. Log run time of app/index.js from 0 to perWindowStateLoaded ready = true
   a. To L10 add:

const tStart = process.hrtime()

b. To L361 after ready = true add:

const tTime = process.hrtime(tStart)
console.log(`index.js window ready: ${tTime[0] * 1e3 + tTime[1] * 1e-6} ms`)

2. Start brave and record the run time.
3. Repeat

Note: when switching branches delete node_modules and reinstall to be sure the npm dedupe changes got applied

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

• Adequate test coverage exists to prevent regressions
• Tests should be independent and work correctly when run individually or as a suite ref
• New files have MPL2 license header

[bsclifton]

@ayumi can you please rebase this? I promise to review after that 😄

[ayumi]

@bsclifton thanks for the reminder! I rebased and tried it out again on Linux (dev and built package)

[cezaraugusto]

++ confirmed the perf gains

has this dep passed through sec audit? if so lgtm

[bsclifton]

++

[bsclifton]

Security review created https://github.com/brave/internal/issues/161

Will wait for results before merging

[diracdeltas]

i wonder if we will run into cache invalidation issues? bahmutov/cache-require-paths#6

[diracdeltas]

sorry, accidental close

[bsclifton]

@ayumi do you know if the cache is invalidated at any point during each run? If not, we could encounter the problem @diracdeltas linked to when running from source (we wouldn't run into it for packaged builds though)

The issue linked does (for that use-case) propose a work-around using an npm hook

[ayumi]

@diracdeltas @bsclifton That's a good catch. The only cache invalidation happens when the requiring a cached path fails, in that case it resolves from scratch: https://github.com/bahmutov/cache-require-paths/blob/master/index.js#L40

So it sounds like this may affect dev builds, however wouldn't affect packaged builds.
Would it be sufficient to add a post npm install hook to delete the cache file?

[bsclifton]

@ayumi I think that would be perfect actually 😄 👍

[bsclifton]

Let's go with the invalidating using the install npm hook 😄

[bsclifton]

pushing back a few releases - @ayumi we can re-review once the hook is done (per notes above) 😄

[bsclifton]

Closing PR for now- let's reopen if we have the time to revisit 😄