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

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

Merged
merged 3 commits into from
Aug 1, 2024
Merged

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Jul 30, 2024

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

This allows users to run `npx sass` when they've just installed
`sass-embedded` and directly run the Dart VM. Due to npm/cmd-shim#152,
there's no way to make this work on Windows CMD/Powershell without
substantially curtailing the performance on other operating systems.
@nex3 nex3 requested a review from jathak July 30, 2024 00:13
@nex3
Copy link
Contributor Author

nex3 commented Jul 30, 2024

cc @ntkme since this seems in your wheelhouse

@ntkme
Copy link
Contributor

ntkme commented Jul 30, 2024

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
Copy link
Contributor Author

nex3 commented Jul 30, 2024

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
Copy link
Contributor Author

nex3 commented Jul 30, 2024

Filed yarnpkg/berry#6422 to track this on the Yarn end.

nex3 added a commit to sass/dart-sass that referenced this pull request Jul 30, 2024
@ntkme
Copy link
Contributor

ntkme commented Jul 30, 2024

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
Copy link
Contributor Author

nex3 commented Jul 30, 2024

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
Copy link
Contributor

ntkme commented Jul 30, 2024

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
Copy link
Contributor Author

nex3 commented Jul 30, 2024

I've filed #314 to track making this check more efficient.

@ntkme
Copy link
Contributor

ntkme commented Aug 1, 2024

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

I've put up a PR for that: #322.

nex3 added a commit to sass/dart-sass that referenced this pull request Aug 1, 2024
bin/sass Outdated
Comment on lines 11 to 15
child_process.execFileSync(
compilerCommand[0], [...compilerCommand.slice(1), ...process.argv.slice(2)], {
stdio: 'inherit',
windowsHide: true
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me realize there's not a great way to get this file covered by the linter/formatter as-is, so I converted it to TypeScript.

@nex3 nex3 merged commit 0093f68 into main Aug 1, 2024
16 checks passed
@nex3 nex3 deleted the bin-sass branch August 1, 2024 20:14
@jgerigmeyer
Copy link
Contributor

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

@nex3
Copy link
Contributor Author

nex3 commented Sep 3, 2024

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

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.

4 participants