Binwrap slows execution down
andys8 opened this issue · 16 comments
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 0swapsSimilar results here:
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.010sIs the overhead mostly in starting node, or in something the js code is doing?
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
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 0swapsAs 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.
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.
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 thebinwrap(#31) package being slow to execute theelm-formatbinary. 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 elm/compiler@33e43e8#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-depstries to download the binary every time for people who have the--ignore-scriptsflag enabled.
So the code you are seeing there is an attempt to avoid that.
The problem is that the binary is calledelm.exeon 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 likebin/elm.cmdto 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!)
I made the change suggested by @andys8 here: #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.
Also, thanks everyone for the helpful details, and especially @andys8 for the detailed investigation.
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 :)
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-formatwhich is a bash script that explicitly callsnodewith the path of theelm-formatbin (%USERPROFILE%\AppData\Roaming\npm\node_modules\elm-format\bin\elm-format)elm-format.cmdwhich is a Windows batch script that does basically the same, namely calling theelm-formatbin withnodeexplicitly
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\npmwith the actual binary, although in my opinion we shouldn't do anything that depends upon the inner workings ofnpm
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?
We could still do the binary replacement trick for *NIX systems.
@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
0.2.2 is released (and will be used in the upcoming elm-format 0.8.2)