Binwrap slows execution down

andys8 commented 5 years ago

My editor is executing elm-format a lot. It looks like binwrap is slowing down execution. Same effect with npm only. I think we need more comparisons on other systems and if binwrap is the cause we should look into something like the elm "no-deps" installer, which is using npm but looks like it isn't slowing execution down on my machine.

https://github.com/elm/compiler/tree/master/installers/npm

450ms vs 80ms

which -a elm-format                                                                                                                                
/tmp/fnm-shell-3625665/bin/elm-format
/home/as/bin/elm-format


time /tmp/fnm-shell-3625665/bin/elm-format src/Main.elm --yes                                                                                      
Processing file src/Main.elm
0.48user 0.04system 0:00.45elapsed 114%CPU (0avgtext+0avgdata 52876maxresident)k
0inputs+0outputs (0major+9793minor)pagefaults 0swaps

time /home/as/bin/elm-format src/Main.elm --yes                                                                                                    
Processing file src/Main.elm
0.07user 0.00system 0:00.08elapsed 89%CPU (0avgtext+0avgdata 14856maxresident)k
0inputs+0outputs (0major+1608minor)pagefaults 0swaps

BendingBender commented 5 years ago

422ms vs 49ms

$ ls -la $(which elm-format)
lrwxr-xr-x  1 db  staff  45 Jun 21 14:26 /Users/db/.nvm/versions/node/v10.16.0/bin/elm-format -> ../lib/node_modules/elm-format/bin/elm-format

$ time /Users/db/.nvm/versions/node/v10.16.0/lib/node_modules/elm-format/bin/elm-format src/Main.elm --yes

real    0m0.422s
user    0m0.368s
sys     0m0.071s

$ time /Users/db/.nvm/versions/node/v10.16.0/lib/node_modules/elm-format/unpacked_bin/elm-format src/Main.elm --yes
Processing file src/Main.elm

real    0m0.049s
user    0m0.030s
sys     0m0.010s

avh4 commented 5 years ago

Is the overhead mostly in starting node, or in something the js code is doing?

andys8 commented 5 years ago

Added some time measurements:

Summary

Node upstart: ~60ms
Imports in node script (require): 187ms
Script running until spawn: 2ms
Spawn with elm-format: 11ms (??)

Note: Quick measurements, please verify with your own. Also this is a different machine, than my previous post.

Logs

time elm-format                                                                                                        
import: 187.111ms
total: 189.114ms
elm-format 0.8.1

Usage: elm-format [INPUT] [--output FILE] [--yes] [--validate] [--stdin]
                  [--elm-version VERSION] [--upgrade]
  Format Elm source files.

Available options:
  -h,--help                Show this help text
  --output FILE            Write output to FILE instead of overwriting the given
                           source file.
  --yes                    Reply 'yes' to all automated prompts.
  --validate               Check if files are formatted without changing them.
  --stdin                  Read from stdin, output to stdout.
  --elm-version VERSION    The Elm version of the source files being formatted.
                           Valid values: 0.18, 0.19. Default: auto
  --upgrade                Upgrade older Elm files to Elm 0.19 syntax

Examples:
  elm-format Main.elm                     # formats Main.elm
  elm-format Main.elm --output Main2.elm  # formats Main.elm as Main2.elm
  elm-format src/                         # format all *.elm files in the src directory

Full guide to using elm-format at <https://github.com/avh4/elm-format>
0.30user 0.01system 0:00.26elapsed 117%CPU (0avgtext+0avgdata 51856maxresident)k
0inputs+0outputs (0major+10958minor)pagefaults 0swaps

Node only

time node -e "console.log('does nothing')"                                                                             
does nothing
0.04user 0.01system 0:00.06elapsed 100%CPU (0avgtext+0avgdata 30180maxresident)k
0inputs+0outputs (0major+2231minor)pagefaults 0swaps

andys8 commented 5 years ago

Looks like moving those lines:

  var packageInfo = require(path.join(__dirname, "..", "package.json"));
  var package = require(path.join(__dirname, "..", packageInfo.main));

in the block where they're needed, will improve from 260ms to 80ms.

Full file

#!/usr/bin/env node
console.time("total")
console.time("import")
var path = require("path");
var spawn = require("child_process").spawn;
var fs = require("fs");
console.timeEnd("import")

var os = process.env.BINWRAP_PLATFORM || process.platform;
var arch = process.env.BINWRAP_ARCH || process.arch;

var requested = os + "-" + arch;
var current = process.platform + "-" + process.arch;
if (requested !== current ) {
  console.error("WARNING: Using binaries for the requested platform (" + requested + ") instead of for the actual platform (" + current + ").")
}

var binExt = "";
if (os == "win32") {
  binExt = ".exe";
}

var unpackedBinPath = path.join(__dirname, "..", "unpacked_bin");
var binPath = path.join(unpackedBinPath, "elm-format" + binExt);

function execBin() {
  console.timeEnd("total")
  spawn(
    binPath,
    process.argv.slice(2),
    {stdio: 'inherit'}
  ).on('exit', process.exit);
}

if (fs.existsSync(binPath)) {
  execBin();
} else {
  console.error("INFO: Running " + path.basename(__filename) + " for the first time; downloading the actual binary");
  var packageInfo = require(path.join(__dirname, "..", "package.json"));
  var package = require(path.join(__dirname, "..", packageInfo.main));
  package.install(unpackedBinPath, os, arch).then(function(result) {
    execBin();
  }, function(err) {
    console.log("ERR", err);
    process.exit(1);
  });
}

Comparison

For reference: Using the binary directly takes 10ms. So it would still make sense to eliminate the node script for execution.

time /home/user/bin/elm-format
elm-format 0.8.1

Usage: elm-format [INPUT] [--output FILE] [--yes] [--validate] [--stdin]
                  [--elm-version VERSION] [--upgrade]
  Format Elm source files.

Available options:
  -h,--help                Show this help text
  --output FILE            Write output to FILE instead of overwriting the given
                           source file.
  --yes                    Reply 'yes' to all automated prompts.
  --validate               Check if files are formatted without changing them.
  --stdin                  Read from stdin, output to stdout.
  --elm-version VERSION    The Elm version of the source files being formatted.
                           Valid values: 0.18, 0.19. Default: auto
  --upgrade                Upgrade older Elm files to Elm 0.19 syntax

Examples:
  elm-format Main.elm                     # formats Main.elm
  elm-format Main.elm --output Main2.elm  # formats Main.elm as Main2.elm
  elm-format src/                         # format all *.elm files in the src directory

Full guide to using elm-format at <https://github.com/avh4/elm-format>
0.00user 0.00system 0:00.01elapsed 57%CPU (0avgtext+0avgdata 6444maxresident)k
192inputs+0outputs (0major+356minor)pagefaults 0swaps

razzeee commented 5 years ago

So is what @andys8 tracked down something we can do or would that cause problems? Or can we create a binwrap free version like the elm compiler is now?

Unfortunatly I could not find any docs, how @evancz did that for the compiler.

andys8 commented 5 years ago

As far as I know, the trick is/was to overwrite the node file with an actual binary. But there is a recent commit, and I'm not sure, how the comment is meant. It looks like it's reverting the change.

https://github.com/elm/compiler/commit/33e43e87430b6c4abe6ef535bf294cf7d8492c72#diff-fa7bb253843eb34c43642a2324c06571

TimonVS commented 5 years ago

Same issue here, I'm pretty sure it used to work fine on my old Mac though 🤔 .

elm-format installed through npm (0.713s):

λ ~/Projects/personal/elm-in-action/PhotoGroove/ which elm-format
/Users/timon/.asdf/installs/nodejs/10.16.0/.npm/bin/elm-format
λ ~/Projects/personal/elm-in-action/PhotoGroove/ time elm-format
elm-format 0.8.1

Usage: elm-format [INPUT] [--output FILE] [--yes] [--validate] [--stdin]
                  [--elm-version VERSION] [--upgrade]
  Format Elm source files.

Available options:
  -h,--help                Show this help text
  --output FILE            Write output to FILE instead of overwriting the given
                           source file.
  --yes                    Reply 'yes' to all automated prompts.
  --validate               Check if files are formatted without changing them.
  --stdin                  Read from stdin, output to stdout.
  --elm-version VERSION    The Elm version of the source files being formatted.
                           Valid values: 0.18, 0.19. Default: auto
  --upgrade                Upgrade older Elm files to Elm 0.19 syntax

Examples:
  elm-format Main.elm                     # formats Main.elm
  elm-format Main.elm --output Main2.elm  # formats Main.elm as Main2.elm
  elm-format src/                         # format all *.elm files in the src directory

Full guide to using elm-format at <https://github.com/avh4/elm-format>
elm-format  0.42s user 0.20s system 87% cpu 0.713 total

elm-format from binary (0.020s):

λ ~/Projects/personal/elm-in-action/PhotoGroove/ time ~/bin/elm-format 
elm-format 0.8.1

Usage: elm-format [INPUT] [--output FILE] [--yes] [--validate] [--stdin]
                  [--elm-version VERSION] [--upgrade]
  Format Elm source files.

Available options:
  -h,--help                Show this help text
  --output FILE            Write output to FILE instead of overwriting the given
                           source file.
  --yes                    Reply 'yes' to all automated prompts.
  --validate               Check if files are formatted without changing them.
  --stdin                  Read from stdin, output to stdout.
  --elm-version VERSION    The Elm version of the source files being formatted.
                           Valid values: 0.18, 0.19. Default: auto
  --upgrade                Upgrade older Elm files to Elm 0.19 syntax

Examples:
  elm-format Main.elm                     # formats Main.elm
  elm-format Main.elm --output Main2.elm  # formats Main.elm as Main2.elm
  elm-format src/                         # format all *.elm files in the src directory

Full guide to using elm-format at <https://github.com/avh4/elm-format>
~/bin/elm-format  0.00s user 0.00s system 37% cpu 0.020 total

On my old Mac I used n to install Node, on my current Mac I'm using asdf with the Node.js plugin. Though this probably isn't relevant since other people are having the same problem with slowness.

BendingBender commented 5 years ago

I've just asked Evan about the revert, here's our coversation:

Me:

Hi Evan, sorry for contacting you directly, I just wasn’t sure where to post this. There is an issue on github with the binwrap (https://github.com/avh4/binwrap/issues/31) package being slow to execute the elm-format binary. There was some discussion on doing the same as you did for the node installer for the Elm compiler, namely replacing the JS file with the binary upon the first run. But we saw that you’ve reverted this change in https://github.com/elm/compiler/commit/33e43e87430b6c4abe6ef535bf294cf7d8492c72#diff-fa7bb253843eb34c43642a2324c06571. I just wanted to ask you on the reasons why you’ve reverted this, your comit message is a bit cryptic.

Evan:

It seems that on Windows the 0.19.0-no-deps tries to download the binary every time for people who have the --ignore-scripts flag enabled. So the code you are seeing there is an attempt to avoid that. The problem is that the binary is called elm.exe on Windows, so it doesn't actually overwrite the JS file as we'd prefer. I think we'd need to give the JS file a name like bin/elm.cmd to make sure it works on Windows, but then it looks pretty weird on Linux and Mac. It should work there, but it's still a bit odd! Does that make sense? (I'd like to find an ideal solution for overwriting the JS file, so I'm happy to chat about it!)

avh4 commented 5 years ago

I made the change suggested by @andys8 here: https://github.com/avh4/binwrap/pull/33

I believe a much older version of binwrap did overwrite the wrapper with the downloaded binaries, but that turned out to have issues both on Windows (as noted above), and on other systems (I think mainly some linuxes) where users did not have permissions to overwrite the files.

I'd be open to a solution that does overwrite, but it'd need to be thoroughly tested on different system environments and have a safe fallback on systems where the overwrite doesn't work or isn't safe.

avh4 commented 5 years ago

Also, thanks everyone for the helpful details, and especially @andys8 for the detailed investigation.

andys8 commented 5 years ago

Cool. What are your ideas about this topic? An additional improvement could be to replace the script with the binary only on mac and unix. This would improve the execution for a large percentage, at the cost of introduction slightly different behavior. I'd also prefer to target a single working approach and use it with elm itself, too. It's solving the same problem.

But also: Feel free to close this issue :)

BendingBender commented 5 years ago

I've just investigated a bit what a global installation of elm-format looks like on Windows. It seems that npm on Windows does the following:

it installs the package into %USERPROFILE%\AppData\Roaming\npm\node_modules
it creates 2 scripts in %USERPROFILE%\AppData\Roaming\npm:
- elm-format which is a bash script that explicitly calls node with the path of the elm-format bin (%USERPROFILE%\AppData\Roaming\npm\node_modules\elm-format\bin\elm-format)
- elm-format.cmd which is a Windows batch script that does basically the same, namely calling the elm-format bin with node explicitly

So this all means that we can't really replace the node script with the actual binary on Windows because the bin script is always being called with node explicitly. From my perspective there are 2 options here:

to leave Windows as is, the binary will be executed through node
to replace the scripts in %USERPROFILE%\AppData\Roaming\npm with the actual binary, although in my opinion we shouldn't do anything that depends upon the inner workings of npm

razzeee commented 5 years ago

Are there more things we could do to improve the speed? And do you have a plan, when the next version including this would be released?

BendingBender commented 5 years ago

We could still do the binary replacement trick for *NIX systems.

razzeee commented 5 years ago

@BendingBender I got this report which is what's confusing to me https://github.com/elm-tooling/elm-language-client-vscode/issues/18#issuecomment-517493720

avh4 commented 5 years ago

0.2.2 is released (and will be used in the upcoming elm-format 0.8.2)

avh4 / binwrap