sass / embedded-host-node

A Node.js library that will communicate with Embedded Dart Sass using the Embedded Sass protocol
MIT License
163 stars 28 forks source link

Add a Sass executable wrapper that forwards to the correct exe #313

Closed nex3 closed 2 months ago

nex3 commented 3 months ago

This allows users to run npx sass when they've just installed sass-embedded and directly run the Dart VM.

nex3 commented 3 months ago

cc @ntkme since this seems in your wheelhouse

ntkme commented 3 months ago

Or maybe we just take a small bite in performance and create the wrapper as node JS script to use child_process.execFileSync(). Given that user probably won't call this extremely frequently, the extra node startup cost and memory cost might be not be too bad, and we can import and reuse the platform detect logic in https://github.com/sass/embedded-host-node/blob/main/lib/src/compiler-path.ts that way.

nex3 commented 3 months ago

Apparently Yarn doesn't support non-JS executables at all, so I guess it's really swimming upstream to try to do it this way. The performance hit is non-negligible—it looks like about 450ms of overhead on my laptop, which will definitely add up if anyone tries running this in a tight loop. But it does seem like the infrastructure is just not there for doing this the faster way.

nex3 commented 3 months ago

Filed https://github.com/yarnpkg/berry/issues/6422 to track this on the Yarn end.

ntkme commented 3 months ago

it looks like about 450ms of overhead on my laptop,

Is that measuring npm exec sass or just running the script wrapper via node as node sass?

Either way, that is really slow. For reference, the overhead of invoking ruby wrapper directly without using bundle exec is about 40ms on my M1 MBP. Running the ruby wrapper script under bundle as bundle exec sass has an additional 80ms overhead from bundle.

I haven't got a chance to measure it myself, but my guess is that even npm exec's overhead is pretty significant.

nex3 commented 3 months ago

This is installing it via npm install -g and then just running sass. That doesn't invoke npm at all, just the symlinked script (which now runs /usr/bin/env/node). Poking around, it looks like some of that time is coming from loading dependencies through utils.ts that we don't actually use, but most of it is the isLinuxMusl() function.

ntkme commented 3 months ago

The current implementation of isLinuxMusl is very brute force that it reads the whole node binary into a buffer and then search for a substring which is unfortunately slow. A faster and more reliable way is to implement an ELF parser (only need bare minimum capability of parsing elf headers and program headers), extract the interpreter string, and check if -musl is in the basename of the interpreter string.

For reference, this is a Ruby implementation to extract the ELF interpreter string: https://github.com/sass-contrib/sass-embedded-host-ruby/blob/main/lib/sass/elf.rb. I tested it on Linux and it only took less than 1ms on my M1 MBP to get the interpreter string.

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'sass-embedded', require: false
  # https://github.com/protocolbuffers/protobuf/issues/16853
  gem 'google-protobuf', force_ruby_platform: true if Gem::Platform.local.to_s == 'aarch64-linux-musl'
end

module Sass
  starting = Process.clock_gettime(Process::CLOCK_MONOTONIC)
  require 'sass/elf'
  ending = Process.clock_gettime(Process::CLOCK_MONOTONIC)
  puts "interpreter: #{ELF::INTERPRETER}"
  puts "is-musl: #{File.basename(ELF::INTERPRETER).start_with?('ld-musl-')}"
  elapsed = ending - starting
  puts elapsed
end

Sample outputs:

interpreter: /lib/ld-linux-aarch64.so.1
is-musl: false
0.000662623999232892
interpreter: /lib/ld-musl-aarch64.so.1
is-musl: true
0.0005600410004262812
nex3 commented 3 months ago

I've filed https://github.com/sass/embedded-host-node/issues/314 to track making this check more efficient.

ntkme commented 2 months ago

Shall we remove the bin section in the optional dependencies' package.json work?

I've put up a PR for that: https://github.com/sass/embedded-host-node/pull/322.

jgerigmeyer commented 1 month ago

@nex3 Is there a timeline for making a new release of sass-embedded that includes this fix?

nex3 commented 1 month ago

@jgerigmeyer We're planning to cut a release as soon as @jathak's work on making deprecations available to the legacy API lands.