sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.94k stars 359 forks source link

Concurrent requests block each other and Stack Overflow under high concurrency #1959

Closed ntkme closed 1 year ago

ntkme commented 2 years ago

The dart-sass-embedded compiler would stack overflow with lots of concurrent requests.

This issue cannot be reproduced with the sass-embedded npm package because the node host does not re-use the compiler, but on the ruby host I can reliably reproduce it:

require 'sass-embedded'

200.times do
  Thread.new do
    begin
      puts Sass.compile_string('a { b: foo() }', functions: {
        'foo()' => lambda do |_args|
          sleep 10
          Sass::Value::String.new('bar')
        end
      }).css
    rescue
    end
  end
end

Thread.list.each do |t|
  t.join unless t == Thread.current
end
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: Unhandled exception:
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: Stack Overflow
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #0      _RootZone.handleUncaughtError (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#1      _RootZone.runUnaryGuarded (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#2      _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#3      _BufferingStreamSubscription._add (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#4      _SinkTransformerStreamSubscription._add (dart:async/stream_transformers.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#5      _EventSinkWrapper.add (dart:async/stream_transformers.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#6      lengthDelimitedDecoder.<anonymous closure>.<anonymous closure> (package:sass_embedded/src/util/length_delimited_transformer.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#7      _HandlerEventSink.add (dart:async/stream_transformers.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#8      _SinkTransformerStreamSubscription._handleData (dart:async/stream_transformers.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#9      _RootZone.runUnaryGuarded (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#10     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#11     _BufferingStreamSubscription._add (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#12     _StreamController._add (dart:async/stream_controller.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#13     _StreamController.add (dart:async/stream_controller.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#14     _RootZone.runUnaryGuarded (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#15     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#16     _BufferingStreamSubscription._add (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#17     _StreamController._add (dart:async/stream_controller.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#18     _StreamController.add (dart:async/stream_controller.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#19     _Socket._onData (dart:io-patch/socket_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#20     _rootRunUnary (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#21     _RootZone.runUnaryGuarded (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#22     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#23     _BufferingStreamSubscription._add (dart:async/stream_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#24     _StreamController._add (dart:async/stream_controller.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#25     _StreamController.add (dart:async/stream_controller.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#26     new _RawSocket.<anonymous closure> (dart:io-patch/socket_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#27     _NativeSocket.issueReadEvent.issue (dart:io-patch/socket_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#28     _rootRun (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#29     _CustomZone.run (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#30     _CustomZone.bindCallback.<anonymous closure> (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#31     _microtaskLoop (dart:async/schedule_microtask.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#32     _startMicrotaskLoop (dart:async/schedule_microtask.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#33     _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#34     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#35     _waitForEvent (dart:cli-patch/cli_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: sass/dart-sass-embedded#36     waitFor (dart:cli/wait_for.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: ...
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: ...
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8717   _EvaluateVisitor._runFunctionCallable (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8718   _EvaluateVisitor.visitFunctionExpression.<anonymous closure> (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8719   _EvaluateVisitor._addErrorSpan (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8720   _EvaluateVisitor.visitFunctionExpression (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8721   FunctionExpression.accept (package:sass/src/ast/sass/expression/function.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8722   _EvaluateVisitor.visitDeclaration.<anonymous closure> (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8723   NullableExtension.andThen (package:sass/src/util/nullable.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8724   _EvaluateVisitor.visitDeclaration (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8725   Declaration.accept (package:sass/src/ast/sass/statement/declaration.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8726   _EvaluateVisitor.visitStyleRule.<anonymous closure>.<anonymous closure> (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8727   _EvaluateVisitor._withStyleRule (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8728   _EvaluateVisitor.visitStyleRule.<anonymous closure> (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8729   Environment.scope (package:sass/src/environment.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8730   _EvaluateVisitor._withParent (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8731   _EvaluateVisitor.visitStyleRule (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8732   StyleRule.accept (package:sass/src/ast/sass/statement/style_rule.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8733   _EvaluateVisitor.visitStylesheet (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8734   _EvaluateVisitor._execute.<anonymous closure> (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8735   _EvaluateVisitor._withEnvironment (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8736   _EvaluateVisitor._execute (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8737   _EvaluateVisitor.run.<anonymous closure> (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8738   _rootRun (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8739   _CustomZone.run (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8740   _runZoned (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8741   runZoned (dart:async/zone.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8742   withEvaluationContext (package:sass/src/evaluation_context.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8743   _EvaluateVisitor.run (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8744   evaluate (package:sass/src/visitor/evaluate.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8745   _compileStylesheet (package:sass/src/compile.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8746   compileString (package:sass/src/compile.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8747   compileStringToResult (package:sass/sass.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8748   main.<anonymous closure> (file:///home/runner/work/dart-sass-embedded/dart-sass-embedded/bin/dart_sass_embedded.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8749   Dispatcher.listen.<anonymous closure> (package:sass_embedded/src/dispatcher.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8750   _FutureListener.handleValue (dart:async/future_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8751   Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8752   Future._propagateToListeners (dart:async/future_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8753   Future._completeWithValue (dart:async/future_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8754   Future._asyncCompleteWithValue.<anonymous closure> (dart:async/future_impl.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8755   _microtaskLoop (dart:async/schedule_microtask.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8756   _startMicrotaskLoop (dart:async/schedule_microtask.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8757   _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart)
/usr/local/bundle/gems/sass-embedded-1.54.9/lib/sass/embedded/compiler.rb:19: warning: #8758   _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart)
nex3 commented 2 years ago

This is an unfortunate side-effect of the way Dart handles the intersection of synchrony and asynchrony. In order to call an asynchronous custom function from a synchronous compilation process, we need to use Dart's waitFor(Future) function which runs another event loop on top of the current stack. This means that if you pile up too many asynchronous operations all at once, you can end up with a stack overflow.

Unfortunately, Dart doesn't have any other mechanism for forcing synchrony. The only alternatives I see are:

ntkme commented 2 years ago

Using waitFor is not good for performance (at least with current implementation), and running fully async may perform better even with all the event loop overhead in some real world use cases.

For example, in the following code I'm submitting a compile request every second, and each of them will take about 2 seconds to complete due to another sleep in the function call. With an event loop implementation, we should be able to yield results for earlier requests almost as soon as they complete, but you can see that the responses for eariler requests are actually blocked until the last one is finished.

The launch of new process is of course a huge overhead, but this can be even worse depending on the usage pattern:

require 'sass-embedded'

threads = []
10.times do |i|
  sleep 1
  threads << Thread.new do
    begin
      puts "start[#{i}]: #{Time.now}"
      puts Sass.compile_string('a { b: foo() }', functions: {
        'foo()' => lambda do |_args|
          sleep 2
          Sass::Value::String.new(i.to_s)
        end
      }).css
      puts "end[#{i}]: #{Time.now}"
    rescue
    end
  end
end

threads.each do |t|
  t.join
end
start[0]: 2022-09-19 21:45:57 +0000
start[1]: 2022-09-19 21:45:58 +0000
start[2]: 2022-09-19 21:45:59 +0000
start[3]: 2022-09-19 21:46:00 +0000
start[4]: 2022-09-19 21:46:01 +0000
start[5]: 2022-09-19 21:46:02 +0000
start[6]: 2022-09-19 21:46:03 +0000
start[7]: 2022-09-19 21:46:04 +0000
start[8]: 2022-09-19 21:46:05 +0000
start[9]: 2022-09-19 21:46:06 +0000
a {
  b: "6";
}
end[6]: 2022-09-19 21:46:08 +0000
a {
  b: "9";
}
end[9]: 2022-09-19 21:46:08 +0000
a {
  b: "8";
}
end[8]: 2022-09-19 21:46:08 +0000
a {
  b: "7";
}
end[7]: 2022-09-19 21:46:08 +0000
a {
  b: "3";
}
end[3]: 2022-09-19 21:46:08 +0000
a {
  b: "4";
}
end[4]: 2022-09-19 21:46:08 +0000
a {
  b: "2";
}
end[2]: 2022-09-19 21:46:08 +0000
a {
  b: "5";
}
end[5]: 2022-09-19 21:46:08 +0000
a {
  b: "1";
}
end[1]: 2022-09-19 21:46:08 +0000
a {
  b: "0";
}
end[0]: 2022-09-19 21:46:08 +0000
ntkme commented 2 years ago

Maybe we should use sync compilation if no host call is needed (e.g. no host function nor host importer), and use async compilation if any host function or host importer is provided.

nex3 commented 2 years ago

Maybe we should use sync compilation if no host call is needed (e.g. no host function nor host importer), and use async compilation if any host function or host importer is provided.

The primary goal of the embedded host is to support plugins efficiently—having a huge performance cliff when you pass one in isn't viable.

That said, @jathak pointed out that Dart has made its isolate support more "lightweight", so it's probably worth testing to see if spawning an isolate per compilation would work. However, it should be noted that this won't be able to work with https://github.com/sass/sass/issues/2745, so if you ever start using that you'll be back to square one.

ntkme commented 2 years ago

Dart has made its isolate support more "lightweight", so it's probably worth testing to see if spawning an isolate per compilation would work.

In my opinion, running in isolate in parallel would will increase CPU-time for individual compilation a little bit, but will reduce the overall wall-clock time if a project can parallel compilation requests. It would be interesting to benckmark and see how much difference we see.

However, it should be noted that this won't be able to work with sass/sass#2745, so if you ever start using that you'll be back to square one.

It's not really that this would make it impossible, but more about the message passing between isolates would be slow and inefficient, right?

I saw some discussions in dart sdk: https://github.com/dart-lang/language/issues/1788, https://github.com/dart-lang/language/issues/333, looks like they aren't going to provide anything soon but it is possible to share data accross isolates by putting the data outside of the dart heap using ffi. Edit: looks like shared heap is available in the recent version of Dart.

nex3 commented 2 years ago

I've mocked up a version of this that can run in parallel using isolates (you can see the code here, although be aware that it's not production-ready) so I could do some benchmarking. My main question was whether and by how much it regressed the performance of sequential compilations that start a separate process each time, since that's the way the JS host currently operates. The answer is that it's about 15% slower than the status quo, which would represent a pretty serious regression.

If you'd like to take a crack at benchmarking this with the Ruby host, I'm curious how it performs when you make multiple concurrent requests—both with and without a delayed function call. My guess is that it will be substantially faster for all the reasons outlined here, but it would be valuable to get some real numbers.

I think for now the high-level takeaway is that this is once again blocked on https://github.com/sass/sass/issues/3296. However, Dart's isolates are now lightweight enough that we probably can get away with using one per compilation once we provide a way to mitigate the startup cost in the JS API.

It's not really that this would make it impossible, but more about the message passing between isolates would be slow and inefficient, right?

I saw some discussions in dart sdk: https://github.com/dart-lang/language/issues/1788, https://github.com/dart-lang/language/issues/333, looks like they aren't going to provide anything soon but it is possible to share data accross isolates by putting the data outside of the dart heap using ffi. Edit: looks like shared heap is available in the recent version of Dart.

It's not strictly impossible, but there are a few major roadblocks:

nex3 commented 2 years ago

I've pushed a couple fixes. Give it another try.

nex3 commented 2 years ago

Okay, I updated the POC branch to use a plain counter for isolate IDs

ntkme commented 2 years ago

There is still a concurrency issue causing some outbound packets on the compiler side to be dropped under high concurrency, I added logging on the host side for all packets:

Update: Turns out the real issue is multiple waitFor in multiple isolates all got stuck at same time for some reason.

require 'sass-embedded'

threads = []

20.times do
  threads << Thread.new do
    Sass.compile_string('a { b: foo() }', style: :compressed, functions: {
      'foo()' => lambda do |_args|
        sleep 3
        Sass::Value::String.new('bar')
      end
    }).css
  end
end

threads.each do |t|
  t.join
end
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 0, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 1, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 2, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 3, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 4, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 5, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 6, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 7, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 8, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 9, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 10, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 11, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 12, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 13, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 14, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 15, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 16, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 17, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 18, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::InboundMessage: compile_request: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest: id: 19, style: :COMPRESSED, source_map: false, importers: [], global_functions: ["foo()"], alert_color: true, alert_ascii: false, verbose: false, quiet_deps: false, source_map_include_sources: false, charset: true, string: <Sass::EmbeddedProtocol::InboundMessage::CompileRequest::StringInput: source: "a { b: foo() }", url: "", syntax: :SCSS>>>
<Sass::EmbeddedProtocol::OutboundMessage: function_call_request: <Sass::EmbeddedProtocol::OutboundMessage::FunctionCallRequest: id: 1, compilation_id: 1, arguments: [], name: "foo">>
<Sass::EmbeddedProtocol::OutboundMessage: function_call_request: <Sass::EmbeddedProtocol::OutboundMessage::FunctionCallRequest: id: 2, compilation_id: 2, arguments: [], name: "foo">>
<Sass::EmbeddedProtocol::OutboundMessage: function_call_request: <Sass::EmbeddedProtocol::OutboundMessage::FunctionCallRequest: id: 3, compilation_id: 3, arguments: [], name: "foo">>
<Sass::EmbeddedProtocol::OutboundMessage: function_call_request: <Sass::EmbeddedProtocol::OutboundMessage::FunctionCallRequest: id: 4, compilation_id: 4, arguments: [], name: "foo">>
<Sass::EmbeddedProtocol::OutboundMessage: function_call_request: <Sass::EmbeddedProtocol::OutboundMessage::FunctionCallRequest: id: 0, compilation_id: 0, arguments: [], name: "foo">>
<Sass::EmbeddedProtocol::InboundMessage: function_call_response: <Sass::EmbeddedProtocol::InboundMessage::FunctionCallResponse: id: 1, accessed_argument_lists: [], success: <Sass::EmbeddedProtocol::Value: string: <Sass::EmbeddedProtocol::Value::String: text: "bar", quoted: true>>>>
<Sass::EmbeddedProtocol::InboundMessage: function_call_response: <Sass::EmbeddedProtocol::InboundMessage::FunctionCallResponse: id: 2, accessed_argument_lists: [], success: <Sass::EmbeddedProtocol::Value: string: <Sass::EmbeddedProtocol::Value::String: text: "bar", quoted: true>>>>
<Sass::EmbeddedProtocol::InboundMessage: function_call_response: <Sass::EmbeddedProtocol::InboundMessage::FunctionCallResponse: id: 3, accessed_argument_lists: [], success: <Sass::EmbeddedProtocol::Value: string: <Sass::EmbeddedProtocol::Value::String: text: "bar", quoted: true>>>>
<Sass::EmbeddedProtocol::InboundMessage: function_call_response: <Sass::EmbeddedProtocol::InboundMessage::FunctionCallResponse: id: 4, accessed_argument_lists: [], success: <Sass::EmbeddedProtocol::Value: string: <Sass::EmbeddedProtocol::Value::String: text: "bar", quoted: true>>>>
<Sass::EmbeddedProtocol::InboundMessage: function_call_response: <Sass::EmbeddedProtocol::InboundMessage::FunctionCallResponse: id: 0, accessed_argument_lists: [], success: <Sass::EmbeddedProtocol::Value: string: <Sass::EmbeddedProtocol::Value::String: text: "bar", quoted: true>>>>
ntkme commented 2 years ago

If you want to test it, you can install the gem with a custom build using the following commands:

dart pub get
dart run grinder pkg-standalone-linux-x64
gem cleanup sass-embedded
gem uninstall sass-embedded
gem install --verbose sass-embedded -- SASS_EMBEDDED=$PWD/build/sass_embedded-1.55.0-linux-x64.tar.gz
ntkme commented 1 year ago

@nex3 I finally got some time to take a look at this and got it fully working based on POC: https://github.com/ntkme/dart-sass-embedded/tree/poc.isolates

Here are the changes from the current POC:

In my test, this is about 20% faster when compiling a small number of large input (compile bootstrap, concurrency of 10), due to being able to taking advantage of multi cores. Unfortunately, it is also more than 4x slower when compiling tons of tiny input (compile 1 line input, concurrency of 10), likely due to the cost of spawning isolates.

Next thing to try is to see if re-use isolates would improve the tiny input case.

nex3 commented 1 year ago

Yes, unfortunately the async compilation functions have such a high intrinsic overhead that they're effectively unusable for performance-sensitive applications. I think we'll need to stick with synchronous compilations in separate isolates. I'm not sure why you were seeing deadlocks, but one possible short-term workaround would just be to add a hard limit (something like ceil(cores * 1.5) concurrent compilations) and just queue up any jobs beyond that.

nex3 commented 1 year ago

I had some time to circle back to this today. I tried running your repro from above against the poc.isolates branch, but I couldn't reproduce a deadlock. In fact, I was able to run 100 compilations in 3.167s (which is to say, 167ms overhead on top of the 3s sleep). Once I started pushing the concurrency higher, I did eventually force a stack overflow—but it took 166 concurrent requests. I'll see if I can use that to find a solution, although once the concurrency gets that high a queuing solution might just be simpler.

ntkme commented 1 year ago

a queuing solution might just be simpler.

It might also be more performant for given the limitation we're facing with Dart SDK.


I actually did a local POC where:

  1. In main thread I starts a RawServerSocket.
  2. In each isolate worker starts a RawSynchronousSocket connecting to socket. - This allows the isolate to exchange message with main thread synchronously, and completely eliminate the use of waitFor.

It does work, but the use of an extra TCP socket for cross-isolate communications make things way slower. If we can get Dart team to implement https://github.com/dart-lang/sdk/issues/44804, it can then be a feasible and likely a desirable solution. Without proper blocking isolate communication, I don't think we an have a "proper" solution to this issue, so queuing might be a good idea.

nex3 commented 1 year ago

I think I actually screwed up the initial install, and I wasn't actually running against the poc.isolates branch. Manually injecting that branch into the gem, I'm seeing it deadlock around 16x concurrency (4x the number of cores my laptop has, although that may be a coincidence).

nex3 commented 1 year ago

I've pushed a new version which uses a pool of up to 15 isolates, and enqueues compile requests after the 15th. In my testing with your package, it seems to work for arbitrarily-sized workloads. However, it's no longer terminating—I'm assuming this is somehow because the isolates are still alive, but I'd expect your package to close out the underlying process at some point. I could add a timeout to close unused isolates if there's no request for a certain amount of time, but this might also require some API tweaks to allow end users to directly specify the process lifetime.

Other than that, my only remaining concern is whether the 16-waitFor() boundary is inherent to the Dart VM or computer-specific somehow. Having a flexible number of isolates would be a lot more complex so I'd like to avoid it, but I don't want to end up in a situation where the host deadlocks on single-core systems or whatever.