From 23282457ad349f1cbe59e22479d8539b4ebe7e9d Mon Sep 17 00:00:00 2001 From: Nate Biggs Date: Thu, 26 Oct 2023 16:00:51 +0000 Subject: [PATCH] [dart2js] Small improvements in emitting JS. - Remove unused Map in source map emitter logic. - Buffer output writing for both JS and source maps. Previously we were only buffering the source map writes. During local compilations of a large program combined these amount to a >100MB improvement in memory usage. Change-Id: I633c2f81aa28744e30c6a706bb3927423b38a6e0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331361 Reviewed-by: Mayank Patke Commit-Queue: Nate Biggs --- .../lib/src/io/source_map_builder.dart | 12 +- .../lib/src/source_file_provider.dart | 76 +------- pkg/compiler/lib/src/util/output_util.dart | 170 ++++++++++++++---- 3 files changed, 143 insertions(+), 115 deletions(-) diff --git a/pkg/compiler/lib/src/io/source_map_builder.dart b/pkg/compiler/lib/src/io/source_map_builder.dart index 229389205ab..2cce710044d 100644 --- a/pkg/compiler/lib/src/io/source_map_builder.dart +++ b/pkg/compiler/lib/src/io/source_map_builder.dart @@ -63,22 +63,12 @@ class SourceMapBuilder { void build() { LineColumnMap lineColumnMap = LineColumnMap(); - Map> sourceLocationMap = {}; entries.forEach((SourceMapEntry sourceMapEntry) { Location kernelLocation = locationProvider.getLocation(sourceMapEntry.targetOffset); int line = kernelLocation.line - 1; int column = kernelLocation.column - 1; lineColumnMap.add(line, column, sourceMapEntry); - - SourceLocation location = sourceMapEntry.sourceLocation; - if (location.sourceUri != null) { - LineColumnMap sourceLineColumnMap = - sourceLocationMap.putIfAbsent( - location.sourceUri!, () => LineColumnMap()); - sourceLineColumnMap.add( - location.line - 1, location.column - 1, sourceMapEntry); - } }); _build(lineColumnMap); @@ -282,7 +272,7 @@ class SourceMapBuilder { extension = 'js.map.${sourceLocations.name}'; } } - final outputSink = BufferedStringOutputSink(compilerOutput + final outputSink = BufferedStringSinkWrapper(compilerOutput .createOutputSink(name, extension, api.OutputType.sourceMap)); SourceMapBuilder sourceMapBuilder = SourceMapBuilder( sourceLocations.name, diff --git a/pkg/compiler/lib/src/source_file_provider.dart b/pkg/compiler/lib/src/source_file_provider.dart index 8142c1a0b3f..3361b899871 100644 --- a/pkg/compiler/lib/src/source_file_provider.dart +++ b/pkg/compiler/lib/src/source_file_provider.dart @@ -14,6 +14,7 @@ import '../compiler_api.dart' as api; import 'colors.dart' as colors; import 'common/metrics.dart'; import 'io/source_file.dart'; +import 'util/output_util.dart'; abstract class SourceFileByteReader { List getBytes(String filename, {bool zeroTerminated = true}); @@ -396,36 +397,7 @@ class RandomAccessFileOutputProvider implements api.CompilerOutput { allOutputFiles.add(fe.relativizeUri(Uri.base, uri, Platform.isWindows)); - int charactersWritten = 0; - - void writeStringSync(String data) { - // Write the data in chunks of 8kb, otherwise we risk running OOM. - int chunkSize = 8 * 1024; - - int offset = 0; - while (offset < data.length) { - String chunk; - int cut = offset + chunkSize; - if (cut < data.length) { - // Don't break the string in the middle of a code point encoded as two - // surrogate pairs since `writeStringSync` will encode the unpaired - // surrogates as U+FFFD REPLACEMENT CHARACTER. - int lastCodeUnit = data.codeUnitAt(cut - 1); - if (_isLeadSurrogate(lastCodeUnit)) { - cut -= 1; - } - chunk = data.substring(offset, cut); - } else { - chunk = offset == 0 ? data : data.substring(offset); - } - output.writeStringSync(chunk); - offset += chunk.length; - } - charactersWritten += data.length; - } - - void onDone() { - output.closeSync(); + void onClose(int charactersWritten) { totalCharactersWritten += charactersWritten; if (isPrimaryOutput) { totalCharactersWrittenPrimary += charactersWritten; @@ -435,11 +407,10 @@ class RandomAccessFileOutputProvider implements api.CompilerOutput { } } - return _OutputSinkWrapper(writeStringSync, onDone); + return BufferedStringSinkWrapper( + FileStringOutputSink(output, onClose: onClose)); } - static bool _isLeadSurrogate(int codeUnit) => (codeUnit & 0xFC00) == 0xD800; - @override api.BinaryOutputSink createBinarySink(Uri uri) { uri = Uri.base.resolveUri(uri); @@ -460,19 +431,11 @@ class RandomAccessFileOutputProvider implements api.CompilerOutput { throw StateError('unreachable'); } - int bytesWritten = 0; - - void writeBytesSync(List data, [int start = 0, int? end]) { - output.writeFromSync(data, start, end); - bytesWritten += (end ?? data.length) - start; - } - - void onDone() { - output.closeSync(); + void onClose(int bytesWritten) { totalDataWritten += bytesWritten; } - return _BinaryOutputSinkWrapper(writeBytesSync, onDone); + return FileBinaryOutputSink(output, onClose: onClose); } } @@ -493,33 +456,6 @@ class RandomAccessBinaryOutputSink implements api.BinaryOutputSink { } } -class _OutputSinkWrapper extends api.OutputSink { - void Function(String) onAdd; - void Function() onClose; - - _OutputSinkWrapper(this.onAdd, this.onClose); - - @override - void add(String data) => onAdd(data); - - @override - void close() => onClose(); -} - -class _BinaryOutputSinkWrapper extends api.BinaryOutputSink { - void Function(List, [int, int?]) onWrite; - void Function() onClose; - - _BinaryOutputSinkWrapper(this.onWrite, this.onClose); - - @override - void add(List data, [int start = 0, int? end]) => - onWrite(data, start, end); - - @override - void close() => onClose(); -} - /// Adapter to integrate dart2js in bazel. /// /// To handle bazel's special layout: diff --git a/pkg/compiler/lib/src/util/output_util.dart b/pkg/compiler/lib/src/util/output_util.dart index f2fa2e75ed6..e77bcb47036 100644 --- a/pkg/compiler/lib/src/util/output_util.dart +++ b/pkg/compiler/lib/src/util/output_util.dart @@ -1,59 +1,161 @@ +import 'dart:io'; + import '../../compiler_api.dart' as api; +/// Implementation of [api.BinaryOutputSink] that writes to a provided file. +class FileBinaryOutputSink extends api.BinaryOutputSink { + final RandomAccessFile _fileOut; + void Function(int bytesWritten)? onClose; + int _bytesWritten = 0; + + FileBinaryOutputSink(this._fileOut, {this.onClose}); + + @override + void add(List data, [int start = 0, int? end]) { + _fileOut.writeFromSync(data, start, end); + _bytesWritten += (end ?? data.length) - start; + } + + @override + void close() { + _fileOut.closeSync(); + if (onClose != null) { + onClose!(_bytesWritten); + } + } +} + +/// Implementation of [api.OutputSink] that writes to a provided file. +class FileStringOutputSink implements api.OutputSink { + final RandomAccessFile _fileOut; + void Function(int charactersWritten)? onClose; + int _charactersWritten = 0; + + FileStringOutputSink(this._fileOut, {this.onClose}); + + @override + void add(String text) { + _fileOut.writeStringSync(text); + _charactersWritten += text.length; + } + + @override + void close() { + _fileOut.closeSync(); + if (onClose != null) { + onClose!(_charactersWritten); + } + } +} + /// [StringSink] that buffers data written to the underlying [api.OutputSink]. /// -/// Useful for string that are built slowly over many writes, avoids keeping the -/// entire string in memory. Each write operation checks if the length of the -/// buffer is above the [maxLength] threshold but a single write can push the -/// buffer above that threshold meaning more than [maxLength] bytes may be -/// forwarded at once. Avoid large writes to ensure the buffer is effective. -class BufferedStringOutputSink implements StringSink { - StringBuffer buffer = StringBuffer(); - final api.OutputSink outputSink; - final int maxLength; - static const int _defaultMaxLength = 1024; +/// Useful for strings that are built slowly over many writes, avoids keeping +/// the entire string in memory. Each write operation checks if the internal +/// buffer is longer than a maximum length and writes the contents to the +/// underlying [api.OutputSink] if so. Chunks large writes to prevent OOM +/// issues. +/// +/// When wrapping an [api.OutputSink], will check if the provided sink is itself +/// a [BufferedStringSinkWrapper]. If it is the same object will be returned, +/// otherwise returns a new [BufferedStringSinkWrapper] wrapping the provided +/// sink. +/// +/// Avoid large writes to a [BufferedStringSinkWrapper] to ensure the buffer is +/// effective. +class BufferedStringSinkWrapper implements api.OutputSink, StringSink { + static const _maxBufferSize = 1024; - BufferedStringOutputSink(this.outputSink, - {this.maxLength = _defaultMaxLength}); + /// This should be at most 8kb, otherwise we risk running OOM. If + /// [_maxBufferSize] is less than [_maxChunkSize] then this chunking will only + /// be used if a single write exceeds this size. + static const _maxChunkSize = 8 * 1024; + final _buffer = StringBuffer(); + final api.OutputSink _outputSink; + + BufferedStringSinkWrapper._(this._outputSink); + + factory BufferedStringSinkWrapper(api.OutputSink _outputSink) { + return _outputSink is BufferedStringSinkWrapper + ? _outputSink + : BufferedStringSinkWrapper._(_outputSink); + } + + static bool _isLeadSurrogate(int codeUnit) => (codeUnit & 0xFC00) == 0xD800; + + /// Write the data in chunks if it is too large for a single write. + void _flushChunked(String data) { + int offset = 0; + while (offset < data.length) { + String chunk; + int cut = offset + _maxChunkSize; + if (cut < data.length) { + // Don't break the string in the middle of a code point encoded as two + // surrogate pairs since `writeStringSync` will encode the unpaired + // surrogates as U+FFFD REPLACEMENT CHARACTER. + int lastCodeUnit = data.codeUnitAt(cut - 1); + if (_isLeadSurrogate(lastCodeUnit)) { + cut -= 1; + } + chunk = data.substring(offset, cut); + } else { + chunk = offset == 0 ? data : data.substring(offset); + } + _outputSink.add(chunk); + offset += chunk.length; + } + } + + void _flush() { + final data = _buffer.toString(); + if (data.length > _maxChunkSize) { + _flushChunked(data); + } else { + _outputSink.add(data); + } + _buffer.clear(); + } + + void _flushIfNecessary() { + if (_buffer.length >= _maxBufferSize) { + _flush(); + } + } + + @override + void add(String data) { + _buffer.write(data); + _flushIfNecessary(); + } + + @override void close() { - outputSink.add(buffer.toString()); - outputSink.close(); + _flush(); + _outputSink.close(); } @override void write(Object? object) { - buffer.write(object); - if (buffer.length > maxLength) { - outputSink.add(buffer.toString()); - buffer.clear(); - } + _buffer.write(object); + _flushIfNecessary(); } @override void writeAll(Iterable objects, [String separator = ""]) { - buffer.writeAll(objects, separator); - if (buffer.length > maxLength) { - outputSink.add(buffer.toString()); - buffer.clear(); - } + _buffer.writeAll(objects, separator); + _flushIfNecessary(); } @override void writeCharCode(int charCode) { - buffer.writeCharCode(charCode); - if (buffer.length > maxLength) { - outputSink.add(buffer.toString()); - buffer.clear(); - } + _buffer.writeCharCode(charCode); + _flushIfNecessary(); } @override void writeln([Object? object = ""]) { - buffer.writeln(object); - if (buffer.length > maxLength) { - outputSink.add(buffer.toString()); - buffer.clear(); - } + _buffer.writeln(object); + _flushIfNecessary(); } }