Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

perf(cli): load only requested sub-system modules and inline require ipfs and ipfs-api #1350

Merged
merged 1 commit into from
May 12, 2018

Conversation

alanshaw
Copy link
Member

This is an alternative to #1336 inlining all the requires in every file for the CLI. This is a great idea but I wanted to see if it could be done with minimal disruption as well as other reasons.

I had a look at the good work in #1336 and have taken the best bits that provide us with the biggest gains 🚀

I've changed src/cli/bin.js so that commandDir now takes an include filter. The idea being to only include the command files for the sub-system the user asked for. i.e. if the user types jsipfs files add then we only require src/cli/commands/files.js.

I've applied the changes @victorbjelkholm made to test/cli/daemon.js verbatim as this was also really beneficial.

Inlining the require for js-ipfs and js-ipfs-api is totally worth it as these two have many many dependencies. I've added a comment next to each to make it obvious why it has been done.

As a baseline, npm run test:node:cli took 12m57.607s.

With the work in this PR, it took 6m26.499s.

I also ran #1336 and it took 5m52.966s.

So around ~30s slower than in #1336. It was never going to be faster, but it is a significantly smaller changeset, still reduces the test time for the CLI by over 50%, and addresses the concerns I raised here.

@ghost ghost assigned alanshaw May 11, 2018
@ghost ghost added the status/in-progress In progress label May 11, 2018
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@alanshaw alanshaw force-pushed the perf/fatser-tests branch from 5b78a07 to 8800909 Compare May 11, 2018 15:06
@daviddias
Copy link
Member

DNS tests are failing pretty reliably but they are not related with this PR

image

@victorb
Copy link
Member

victorb commented May 11, 2018

Seems to provide the same benefits without all of the hassle of inline requires. Seems it's just the root command which is now the same as master (while my PR made that fast as well) but I think we can live with that taking 0.8, more important to have the commands themselves faster.

Much better solution, thanks for doing this 👍

@daviddias daviddias merged commit 3820be0 into master May 12, 2018
@daviddias daviddias deleted the perf/fatser-tests branch May 12, 2018 16:57
@ghost ghost removed the status/in-progress In progress label May 12, 2018
This was referenced May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants