Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the interactive part of the CLI to a separate file #60

Merged
merged 10 commits into from Mar 25, 2019
Merged

Move the interactive part of the CLI to a separate file #60

merged 10 commits into from Mar 25, 2019

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Mar 2, 2019

Moved all the code and requires handling interactive sessions to cli-interactive.js, leaving only arguments parsing and invoking fkill if possible in cli.js.

Timed requires with node --require time-require ./cli.js :8080 with process-to-kill on port 8080 on my dev machine(high speed nvme ssd, Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz).

All values are rough averages of 5 runs, fs primed by running command another 5 times prior to test runs(and restarting process-to-kill), time between runs: roughly 5 seconds.
Didn't find any framework for such kind of tests, so just did it by hand.

before:

  • total time: 321ms
  • require time: 235ms
  • time spent on time-require module: 24ms
  • calculated total time without time-require: 297ms
  • calculated require time without time-require: 211ms

after:

  • total time: 194ms
  • require time: 108ms
  • time spent on time-require module: 24ms
  • calculated total time without time-require: 170ms
  • calculated require time without time-require: 84ms

effective require speedup: -60%, x2.5 times
effective total speedup: -43%, x1.75 times

Another possible speedup can be achieved by removing meow and using minimist directly:
-33ms to all metrics, bringing require speedup to -76%, x4.1 times, total speedup -54%, x2.1times


Fixes #58

@sindresorhus
Copy link
Owner

Another possible speedup can be achieved by removing meow and using minimist directly

I plan to fix the meow slowness in a different way: sindresorhus/meow#107

cli.js Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title moved interactive part to another file Move the interactive part of the CLI to a separate file Mar 3, 2019
@stroncium
Copy link
Contributor Author

@sindresorhus Pushed wrong branch by accident. Tried to make a test against this, but Travis execution speeds are too unpredictable to make it work good, plus there was some unknown issue with pty not working correctly at all on some runs. If you have any pointers about writing such tests for node/travis, I could write one, but it seems like no one is testing such functionality.

Anyhow, fixed the problems.

@sindresorhus sindresorhus merged commit 4a5bf79 into sindresorhus:master Mar 25, 2019
@sindresorhus
Copy link
Owner

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants