From 77e208c2044dc91ab5aa75c6a96531922592b049 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Fri, 8 Sep 2023 18:29:09 -0700 Subject: [PATCH] Run cli compilations in parallel dart isolates (#2078) Co-authored-by: Natalie Weizenbaum --- bin/sass.dart | 77 ++----------- lib/src/executable/compile_stylesheet.dart | 64 ++++++++++- lib/src/executable/concurrent.dart | 66 ++++++++++++ lib/src/executable/concurrent/js.dart | 10 ++ lib/src/executable/concurrent/vm.dart | 33 ++++++ lib/src/executable/watch.dart | 119 +++++++-------------- lib/src/io/interface.dart | 8 ++ lib/src/io/js.dart | 8 ++ lib/src/io/vm.dart | 15 ++- lib/src/parse/parser.dart | 5 +- test/cli/shared/colon_args.dart | 1 - test/cli/shared/update.dart | 23 +++- test/cli/shared/watch.dart | 4 +- 13 files changed, 273 insertions(+), 160 deletions(-) create mode 100644 lib/src/executable/concurrent.dart create mode 100644 lib/src/executable/concurrent/js.dart create mode 100644 lib/src/executable/concurrent/vm.dart diff --git a/bin/sass.dart b/bin/sass.dart index 986ff9bcb..ad23649d4 100644 --- a/bin/sass.dart +++ b/bin/sass.dart @@ -8,17 +8,14 @@ import 'package:path/path.dart' as p; import 'package:stack_trace/stack_trace.dart'; import 'package:term_glyph/term_glyph.dart' as term_glyph; -import 'package:sass/src/exception.dart'; -import 'package:sass/src/executable/compile_stylesheet.dart'; +import 'package:sass/src/executable/concurrent.dart'; import 'package:sass/src/executable/options.dart'; import 'package:sass/src/executable/repl.dart'; import 'package:sass/src/executable/watch.dart'; import 'package:sass/src/import_cache.dart'; import 'package:sass/src/io.dart'; -import 'package:sass/src/io.dart' as io; import 'package:sass/src/logger/deprecation_handling.dart'; import 'package:sass/src/stylesheet_graph.dart'; -import 'package:sass/src/util/map.dart'; import 'package:sass/src/utils.dart'; import 'package:sass/src/embedded/executable.dart' // Never load the embedded protocol when compiling to JS. @@ -26,27 +23,6 @@ import 'package:sass/src/embedded/executable.dart' as embedded; Future main(List args) async { - var printedError = false; - - // Prints [error] to stderr, along with a preceding newline if anything else - // has been printed to stderr. - // - // If [trace] is passed, its terse representation is printed after the error. - void printError(String error, StackTrace? stackTrace) { - var buffer = StringBuffer(); - if (printedError) buffer.writeln(); - printedError = true; - buffer.write(error); - - if (stackTrace != null) { - buffer.writeln(); - buffer.writeln(); - buffer.write(Trace.from(stackTrace).terse.toString().trimRight()); - } - - io.printError(buffer); - } - if (args case ['--embedded', ...var rest]) { embedded.main(rest); return; @@ -84,37 +60,8 @@ Future main(List args) async { return; } - for (var (source, destination) in options.sourcesToDestinations.pairs) { - try { - await compileStylesheet(options, graph, source, destination, - ifModified: options.update); - } on SassException catch (error, stackTrace) { - if (destination != null && !options.emitErrorCss) { - _tryDelete(destination); - } - - printError(error.toString(color: options.color), - options.trace ? getTrace(error) ?? stackTrace : null); - - // Exit code 65 indicates invalid data per - // https://www.freebsd.org/cgi/man.cgi?query=sysexits. - // - // We let exitCode 66 take precedence for deterministic behavior. - if (exitCode != 66) exitCode = 65; - if (options.stopOnError) return; - } on FileSystemException catch (error, stackTrace) { - var path = error.path; - printError( - path == null - ? error.message - : "Error reading ${p.relative(path)}: ${error.message}.", - options.trace ? getTrace(error) ?? stackTrace : null); - - // Error 66 indicates no input. - exitCode = 66; - if (options.stopOnError) return; - } - } + await compileStylesheets(options, graph, options.sourcesToDestinations, + ifModified: options.update); } on UsageException catch (error) { print("${error.message}\n"); print("Usage: sass [output.css]\n" @@ -128,8 +75,11 @@ Future main(List args) async { if (options?.color ?? false) buffer.write('\u001b[0m'); buffer.writeln(); buffer.writeln(error); - - printError(buffer.toString(), getTrace(error) ?? stackTrace); + buffer.writeln(); + buffer.writeln(); + buffer.write( + Trace.from(getTrace(error) ?? stackTrace).terse.toString().trimRight()); + printError(buffer); exitCode = 255; } } @@ -154,14 +104,3 @@ Future _loadVersion() async { .split(" ") .last; } - -/// Delete [path] if it exists and do nothing otherwise. -/// -/// This is a separate function to work around dart-lang/sdk#53082. -void _tryDelete(String path) { - try { - deleteFile(path); - } on FileSystemException { - // If the file doesn't exist, that's fine. - } -} diff --git a/lib/src/executable/compile_stylesheet.dart b/lib/src/executable/compile_stylesheet.dart index 8c24ee494..70b52ba10 100644 --- a/lib/src/executable/compile_stylesheet.dart +++ b/lib/src/executable/compile_stylesheet.dart @@ -6,6 +6,7 @@ import 'dart:convert'; import 'package:path/path.dart' as p; import 'package:source_maps/source_maps.dart'; +import 'package:stack_trace/stack_trace.dart'; import '../async_import_cache.dart'; import '../compile.dart'; @@ -30,8 +31,42 @@ import 'options.dart'; /// If [ifModified] is `true`, only recompiles if [source]'s modification time /// or that of a file it imports is more recent than [destination]'s /// modification time. Note that these modification times are cached by [graph]. -Future compileStylesheet(ExecutableOptions options, StylesheetGraph graph, - String? source, String? destination, +/// +/// Returns `(exitCode, error, stackTrace)` when an error occurs. +Future<(int, String, String?)?> compileStylesheet(ExecutableOptions options, + StylesheetGraph graph, String? source, String? destination, + {bool ifModified = false}) async { + try { + await _compileStylesheetWithoutErrorHandling( + options, graph, source, destination, + ifModified: ifModified); + } on SassException catch (error, stackTrace) { + if (destination != null && !options.emitErrorCss) { + _tryDelete(destination); + } + var message = error.toString(color: options.color); + + // Exit code 65 indicates invalid data per + // https://www.freebsd.org/cgi/man.cgi?query=sysexits. + return _getErrorWithStackTrace( + 65, message, options.trace ? getTrace(error) ?? stackTrace : null); + } on FileSystemException catch (error, stackTrace) { + var path = error.path; + var message = path == null + ? error.message + : "Error reading ${p.relative(path)}: ${error.message}."; + + // Exit code 66 indicates no input. + return _getErrorWithStackTrace( + 66, message, options.trace ? getTrace(error) ?? stackTrace : null); + } + return null; +} + +/// Like [compileStylesheet], but throws errors instead of handling them +/// internally. +Future _compileStylesheetWithoutErrorHandling(ExecutableOptions options, + StylesheetGraph graph, String? source, String? destination, {bool ifModified = false}) async { var importer = FilesystemImporter('.'); if (ifModified) { @@ -150,7 +185,7 @@ Future compileStylesheet(ExecutableOptions options, StylesheetGraph graph, buffer.write('Compiled $sourceName to $destinationName.'); if (options.color) buffer.write('\u001b[0m'); - print(buffer); + safePrint(buffer); } /// Writes the source map given by [mapping] to disk (if necessary) according to @@ -195,3 +230,26 @@ String _writeSourceMap( return (options.style == OutputStyle.compressed ? '' : '\n\n') + '/*# sourceMappingURL=$escapedUrl */'; } + +/// Delete [path] if it exists and do nothing otherwise. +/// +/// This is a separate function to work around dart-lang/sdk#53082. +void _tryDelete(String path) { + try { + deleteFile(path); + } on FileSystemException { + // If the file doesn't exist, that's fine. + } +} + +/// Return a Record of `(exitCode, error, stackTrace)` for the given error. +(int, String, String?) _getErrorWithStackTrace( + int exitCode, String error, StackTrace? stackTrace) { + return ( + exitCode, + error, + stackTrace != null + ? Trace.from(stackTrace).terse.toString().trimRight() + : null + ); +} diff --git a/lib/src/executable/concurrent.dart b/lib/src/executable/concurrent.dart new file mode 100644 index 000000000..f3020f679 --- /dev/null +++ b/lib/src/executable/concurrent.dart @@ -0,0 +1,66 @@ +// Copyright 2023 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:math' as math; + +import '../io.dart'; +import '../stylesheet_graph.dart'; +import '../util/map.dart'; +import 'compile_stylesheet.dart'; +import 'concurrent/vm.dart' + // Never load the isolate library when compiling to JS. + if (dart.library.js) 'concurrent/js.dart'; +import 'options.dart'; + +/// Compiles the stylesheets concurrently and returns whether all stylesheets are compiled +/// successfully. +Future compileStylesheets(ExecutableOptions options, + StylesheetGraph graph, Map sourcesToDestinations, + {bool ifModified = false}) async { + var errorsWithStackTraces = switch ([...sourcesToDestinations.pairs]) { + // Concurrency does add some overhead, so avoid it in the common case of + // compiling a single stylesheet. + [(var source, var destination)] => [ + await compileStylesheet(options, graph, source, destination, + ifModified: ifModified) + ], + var pairs => await Future.wait([ + for (var (source, destination) in pairs) + compileStylesheetConcurrently(options, graph, source, destination, + ifModified: ifModified) + ], eagerError: options.stopOnError) + }; + + var printedError = false; + + // Print all errors in deterministic order. + for (var errorWithStackTrace in errorsWithStackTraces) { + if (errorWithStackTrace == null) continue; + var (code, error, stackTrace) = errorWithStackTrace; + + // We let the highest exitCode take precedence for deterministic behavior. + exitCode = math.max(exitCode, code); + + _printError(error, stackTrace, printedError); + printedError = true; + } + + return !printedError; +} + +// Prints [error] to stderr, along with a preceding newline if anything else +// has been printed to stderr. +// +// If [stackTrace] is passed, it is printed after the error. +void _printError(String error, String? stackTrace, bool printedError) { + var buffer = StringBuffer(); + if (printedError) buffer.writeln(); + buffer.write(error); + if (stackTrace != null) { + buffer.writeln(); + buffer.writeln(); + buffer.write(stackTrace); + } + printError(buffer); +} diff --git a/lib/src/executable/concurrent/js.dart b/lib/src/executable/concurrent/js.dart new file mode 100644 index 000000000..11cdcb1c1 --- /dev/null +++ b/lib/src/executable/concurrent/js.dart @@ -0,0 +1,10 @@ +// Copyright 2023 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import '../compile_stylesheet.dart'; + +/// We don't currently support concurrent compilation in JS. +/// +/// In the future, we could add support using web workers. +final compileStylesheetConcurrently = compileStylesheet; diff --git a/lib/src/executable/concurrent/vm.dart b/lib/src/executable/concurrent/vm.dart new file mode 100644 index 000000000..b063b52f0 --- /dev/null +++ b/lib/src/executable/concurrent/vm.dart @@ -0,0 +1,33 @@ +// Copyright 2023 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:isolate'; + +import 'package:term_glyph/term_glyph.dart' as term_glyph; + +import '../options.dart'; +import '../../stylesheet_graph.dart'; +import '../compile_stylesheet.dart'; + +/// Compiles the stylesheet at [source] to [destination]. +/// +/// Runs in a new Dart Isolate, unless [source] is `null`. +Future<(int, String, String?)?> compileStylesheetConcurrently( + ExecutableOptions options, + StylesheetGraph graph, + String? source, + String? destination, + {bool ifModified = false}) { + // Reading from stdin does not work properly in dart isolate. + if (source == null) { + return compileStylesheet(options, graph, source, destination, + ifModified: ifModified); + } + + return Isolate.run(() { + term_glyph.ascii = !options.unicode; + return compileStylesheet(options, graph, source, destination, + ifModified: ifModified); + }); +} diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart index 1fc8d9f37..c8a222b0b 100644 --- a/lib/src/executable/watch.dart +++ b/lib/src/executable/watch.dart @@ -2,21 +2,16 @@ // MIT-style license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'dart:collection'; - import 'package:path/path.dart' as p; -import 'package:stack_trace/stack_trace.dart'; import 'package:stream_transform/stream_transform.dart'; import 'package:watcher/watcher.dart'; -import '../exception.dart'; import '../importer/filesystem.dart'; import '../io.dart'; import '../stylesheet_graph.dart'; import '../util/map.dart'; import '../util/multi_dir_watcher.dart'; -import '../utils.dart'; -import 'compile_stylesheet.dart'; +import 'concurrent.dart'; import 'options.dart'; /// Watches all the files in [graph] for changes and updates them as necessary. @@ -41,15 +36,17 @@ Future watch(ExecutableOptions options, StylesheetGraph graph) async { // they currently exist. This ensures that changes that come in update a // known-good state. var watcher = _Watcher(options, graph); - for (var (source, destination) in _sourcesToDestinations(options).pairs) { + var sourcesToDestinations = _sourcesToDestinations(options); + for (var source in sourcesToDestinations.keys) { graph.addCanonical( FilesystemImporter('.'), p.toUri(canonicalize(source)), p.toUri(source), recanonicalize: false); - var success = await watcher.compile(source, destination, ifModified: true); - if (!success && options.stopOnError) { - dirWatcher.events.listen(null).cancel(); - return; - } + } + var success = await compileStylesheets(options, graph, sourcesToDestinations, + ifModified: true); + if (!success && options.stopOnError) { + dirWatcher.events.listen(null).cancel(); + return; } print("Sass is watching for changes. Press Ctrl-C to stop.\n"); @@ -67,34 +64,6 @@ final class _Watcher { _Watcher(this._options, this._graph); - /// Compiles the stylesheet at [source] to [destination], and prints any - /// errors that occur. - /// - /// Returns whether or not compilation succeeded. - Future compile(String source, String destination, - {bool ifModified = false}) async { - try { - await compileStylesheet(_options, _graph, source, destination, - ifModified: ifModified); - return true; - } on SassException catch (error, stackTrace) { - if (!_options.emitErrorCss) _delete(destination); - _printError( - error.toString(color: _options.color), getTrace(error) ?? stackTrace); - exitCode = 65; - return false; - } on FileSystemException catch (error, stackTrace) { - var path = error.path; - _printError( - path == null - ? error.message - : "Error reading ${p.relative(path)}: ${error.message}.", - getTrace(error) ?? stackTrace); - exitCode = 66; - return false; - } - } - /// Deletes the file at [path] and prints a message about it. void _delete(String path) { try { @@ -109,21 +78,6 @@ final class _Watcher { } } - /// Prints [message] to standard error, with [stackTrace] if [_options.trace] - /// is set. - void _printError(String message, StackTrace stackTrace) { - var buffer = StringBuffer(message); - - if (_options.trace) { - buffer.writeln(); - buffer.writeln(); - buffer.write(Trace.from(stackTrace).terse.toString().trimRight()); - } - - if (!_options.stopOnError) buffer.writeln(); - printError(buffer); - } - /// Listens to `watcher.events` and updates the filesystem accordingly. /// /// Returns a future that will only complete if an unexpected error occurs. @@ -172,8 +126,9 @@ final class _Watcher { /// Returns whether all necessary recompilations succeeded. Future _handleAdd(String path) async { var destination = _destinationFor(path); - - var success = destination == null || await compile(path, destination); + var success = destination == null || + await compileStylesheets(_options, _graph, {path: destination}, + ifModified: true); var downstream = _graph.addCanonical( FilesystemImporter('.'), _canonicalize(path), p.toUri(path)); return await _recompileDownstream(downstream) && success; @@ -226,34 +181,42 @@ final class _Watcher { /// Returns whether all recompilations succeeded. Future _recompileDownstream(Iterable nodes) async { var seen = {}; - var toRecompile = Queue.of(nodes); - var allSucceeded = true; - while (toRecompile.isNotEmpty) { - var node = toRecompile.removeFirst(); - if (!seen.add(node)) continue; + while (nodes.isNotEmpty) { + nodes = [ + for (var node in nodes) + if (seen.add(node)) node + ]; - var success = await _compileIfEntrypoint(node.canonicalUrl); - allSucceeded = allSucceeded && success; - if (!success && _options.stopOnError) return false; + var sourcesToDestinations = _sourceEntrypointsToDestinations(nodes); + if (sourcesToDestinations.isNotEmpty) { + var success = await compileStylesheets( + _options, _graph, sourcesToDestinations, + ifModified: true); + if (!success && _options.stopOnError) return false; - toRecompile.addAll(node.downstream); + allSucceeded = allSucceeded && success; + } + + nodes = [for (var node in nodes) ...node.downstream]; } return allSucceeded; } - /// Compiles the stylesheet at [url] to CSS if it's an entrypoint that's being - /// watched. - /// - /// Returns `false` if compilation failed, `true` otherwise. - Future _compileIfEntrypoint(Uri url) async { - if (url.scheme != 'file') return true; - - var source = p.fromUri(url); - return switch (_destinationFor(source)) { - var destination? => await compile(source, destination), - _ => true - }; + /// Returns a sourcesToDestinations mapping for nodes that are entrypoints. + Map _sourceEntrypointsToDestinations( + Iterable nodes) { + var entrypoints = {}; + for (var node in nodes) { + var url = node.canonicalUrl; + if (url.scheme != 'file') continue; + + var source = p.fromUri(url); + if (_destinationFor(source) case var destination?) { + entrypoints[source] = destination; + } + } + return entrypoints; } /// If a Sass file at [source] should be compiled to CSS, returns the path to diff --git a/lib/src/io/interface.dart b/lib/src/io/interface.dart index 618c3ff6e..be0ec574c 100644 --- a/lib/src/io/interface.dart +++ b/lib/src/io/interface.dart @@ -32,8 +32,16 @@ bool get isBrowser => throw ''; /// sequences. bool get supportsAnsiEscapes => throw ''; +/// Prints [message] (followed by a newline) to standard output or the +/// equivalent. +/// +/// This method is thread-safe. +void safePrint(Object? message) => throw ''; + /// Prints [message] (followed by a newline) to standard error or the /// equivalent. +/// +/// This method is thread-safe. void printError(Object? message) => throw ''; /// Reads the file at [path] as a UTF-8 encoded string. diff --git a/lib/src/io/js.dart b/lib/src/io/js.dart index efc1955e4..c806dc181 100644 --- a/lib/src/io/js.dart +++ b/lib/src/io/js.dart @@ -28,6 +28,14 @@ class FileSystemException { String toString() => "${p.prettyUri(p.toUri(path))}: $message"; } +void safePrint(Object? message) { + if (process case var process?) { + process.stdout.write("${message ?? ''}\n"); + } else { + console.log(message ?? ''); + } +} + void printError(Object? message) { if (process case var process?) { process.stderr.write("${message ?? ''}\n"); diff --git a/lib/src/io/vm.dart b/lib/src/io/vm.dart index 0174e09b4..cba88e40f 100644 --- a/lib/src/io/vm.dart +++ b/lib/src/io/vm.dart @@ -37,8 +37,21 @@ bool get supportsAnsiEscapes { return io.stdout.supportsAnsiEscapes; } +void safePrint(Object? message) { + _threadSafeWriteLn(io.stdout, message); +} + void printError(Object? message) { - io.stderr.writeln(message); + _threadSafeWriteLn(io.stderr, message); +} + +void _threadSafeWriteLn(io.IOSink sink, Object? message) { + // This does have performance penality of copying buffer + // if message is already a StringBuffer. + // https://github.com/dart-lang/sdk/issues/53471. + var buffer = StringBuffer(message.toString()); + buffer.writeln(); + sink.write(buffer); } String readFile(String path) { diff --git a/lib/src/parse/parser.dart b/lib/src/parse/parser.dart index dba0bef99..6dd17c80d 100644 --- a/lib/src/parse/parser.dart +++ b/lib/src/parse/parser.dart @@ -9,6 +9,7 @@ import 'package:string_scanner/string_scanner.dart'; import '../exception.dart'; import '../interpolation_map.dart'; +import '../io.dart'; import '../logger.dart'; import '../util/character.dart'; import '../util/lazy_file_span.dart'; @@ -696,9 +697,9 @@ class Parser { @protected void debug([Object? message]) { if (message == null) { - print(scanner.emptySpan.highlight(color: true)); + safePrint(scanner.emptySpan.highlight(color: true)); } else { - print(scanner.emptySpan.message(message.toString(), color: true)); + safePrint(scanner.emptySpan.message(message.toString(), color: true)); } } diff --git a/test/cli/shared/colon_args.dart b/test/cli/shared/colon_args.dart index 88ca8c642..a5ccf314f 100644 --- a/test/cli/shared/colon_args.dart +++ b/test/cli/shared/colon_args.dart @@ -97,7 +97,6 @@ void sharedTests(Future runSass(Iterable arguments)) { await sass.shouldExit(65); await d.file("out1.css", contains(message)).validate(); - await d.nothing("out2.css").validate(); }); group("with a directory argument", () { diff --git a/test/cli/shared/update.dart b/test/cli/shared/update.dart index 01aa73949..4fc7bfbc1 100644 --- a/test/cli/shared/update.dart +++ b/test/cli/shared/update.dart @@ -66,16 +66,31 @@ void sharedTests(Future runSass(Iterable arguments)) { await d.file("test2.scss", r"$var: 2; @import 'other'").create(); var sass = await update(["test1.scss:out1.css", "test2.scss:out2.css"]); - expect(sass.stdout, emits(endsWith('Compiled test1.scss to out1.css.'))); - expect(sass.stdout, emits(endsWith('Compiled test2.scss to out2.css.'))); + expect( + sass.stdout, + emitsInAnyOrder([ + endsWith('Compiled test1.scss to out1.css.'), + endsWith('Compiled test2.scss to out2.css.') + ])); await sass.shouldExit(0); + await d + .file("out1.css", equalsIgnoringWhitespace("a { b: 1; }")) + .validate(); + await d + .file("out2.css", equalsIgnoringWhitespace("a { b: 2; }")) + .validate(); + await tick; await d.file("other.scss", r"x {y: $var}").create(); sass = await update(["test1.scss:out1.css", "test2.scss:out2.css"]); - expect(sass.stdout, emits(endsWith('Compiled test1.scss to out1.css.'))); - expect(sass.stdout, emits(endsWith('Compiled test2.scss to out2.css.'))); + expect( + sass.stdout, + emitsInAnyOrder([ + endsWith('Compiled test1.scss to out1.css.'), + endsWith('Compiled test2.scss to out2.css.') + ])); await sass.shouldExit(0); await d diff --git a/test/cli/shared/watch.dart b/test/cli/shared/watch.dart index b46fd97a0..b80b33e64 100644 --- a/test/cli/shared/watch.dart +++ b/test/cli/shared/watch.dart @@ -3,6 +3,7 @@ // https://opensource.org/licenses/MIT. import 'package:path/path.dart' as p; +import 'package:sass/src/io.dart'; import 'package:test/test.dart'; import 'package:test_descriptor/test_descriptor.dart' as d; import 'package:test_process/test_process.dart'; @@ -29,7 +30,7 @@ void sharedTests(Future runSass(Iterable arguments)) { /// Modifying a file very quickly after it was processed can go /// unrecognized, especially on Windows where filesystem operations can have /// very high delays. - Future tickIfPoll() => poll ? tick : Future.value(); + Future tickIfPoll() => poll || isWindows ? tick : Future.value(); group("${poll ? 'with' : 'without'} --poll", () { group("when started", () { @@ -128,7 +129,6 @@ void sharedTests(Future runSass(Iterable arguments)) { await sass.shouldExit(65); await d.file("out1.css", contains(message)).validate(); - await d.nothing("out2.css").validate(); }); });