Make non-copying version of BytesBuidler and make file-reads faster.

sdk/lib/io/bytes_builder.dart

Index: sdk/lib/io/bytes_builder.dart
diff --git a/sdk/lib/io/bytes_builder.dart b/sdk/lib/io/bytes_builder.dart
index e26ae89cb9b158b71f8d7e50fecd62707120e225..54329c41384631764d2d8ecaac2b467474f0659a 100644
--- a/sdk/lib/io/bytes_builder.dart
+++ b/sdk/lib/io/bytes_builder.dart
@@ -11,17 +11,20 @@ part of dart.io;
  * Used to efficiently collect bytes and lists of bytes, using an internal
  * buffer. Note that it's optimized for IO, using an initial buffer of 1K bytes.

[Søren Gjesse, 2014/04/01 08:58:40]

I am not sure the "optimized of IO" is actually true. And the mentioning of the
1K is irrelevant. If we where to talk about initial size it should be an
optional argument to the constructor.

[Anders Johnsen, 2014/04/01 09:29:07]

Removed.

  */
-class BytesBuilder {
-  // Start with 1024 bytes.
-  static const int _INIT_SIZE = 1024;
-
-  int _length = 0;
-  Uint8List _buffer;
-
+abstract class BytesBuilder {
   /**
    * Construct a new empty [BytesBuilder].
+   *
+   * If [copy] is true, the data is copied when added to the list. Default is
+   * `true`.

[Søren Gjesse, 2014/04/01 08:58:40]

I think some more documentation on what happens if the non-copied data is
changed after it is added. Maybe the message should just be that that behavior
is undefined.

[Anders Johnsen, 2014/04/01 09:29:07]

Done.

    */
-  BytesBuilder();
+  factory BytesBuilder({bool copy: true}) {

[Søren Gjesse, 2014/04/01 08:58:40]

The argument name 'copy' is not totally correct. It might will end up copying
the data, and in most cases it will.

How about adding a separate named constructor instead with a more precise name
and a separate comment.

BytesBuilder.cacheBuffers
BytesBuilder.ownBuffers

[Anders Johnsen, 2014/04/01 09:29:07]

I think cacheBuffers and ownBuffers is confusing. Also, we can not change the
default constructor.

We could add one of

- BytesBuilder.nonCopying
- BytesBuilder.lazyCopy

or something, but I'm not sure I see the gain. I think 'copy' captures the idea
very precise, even though exceptions exists. It's more of a hint than a rule,
that it may choose to not copy.

+    if (copy) {
+      return new _CopyingBytesBuilder();
+    } else {
+      return new _NonCopyingBytesBuilder();
+    }
+  }
 
   /**
    * Appends [bytes] to the current contents of the builder.
@@ -29,6 +32,59 @@ class BytesBuilder {
    * Each value of [bytes] will be bit-representation truncated to the range
    * 0 .. 255.
    */
+  void add(List<int> bytes);
+
+  /**
+   * Append [byte] to the current contents of the builder.
+   *
+   * The [byte] will be bit-representation truncated to the range 0 .. 255.
+   */
+  void addByte(int byte);
+
+  /**
+   * Returns the contents of `this` and clears `this`.
+   *
+   * The list returned is a view of the the internal buffer, limited to the
+   * [length].
+   */
+  List<int> takeBytes();
+
+  /**
+   * Returns a copy of the current contents of the builder.
+   *
+   * Leaves the contents of the builder intact.
+   */
+  List<int> toBytes();
+
+  /**
+   * The number of bytes in the builder.
+   */
+  int get length;
+
+  /**
+   * Returns `true` if the buffer is empty.
+   */
+  bool get isEmpty;
+
+  /**
+   * Returns `true` if the buffer is not empty.
+   */
+  bool get isNotEmpty;
+
+  /**
+   * Clear the contents of the builder.
+   */
+  void clear();
+}
+
+
+class _CopyingBytesBuilder implements BytesBuilder {
+  // Start with 1024 bytes.
+  static const int _INIT_SIZE = 1024;
+
+  int _length = 0;
+  Uint8List _buffer;
+
   void add(List<int> bytes) {
     int bytesLength = bytes.length;
     if (bytesLength == 0) return;
@@ -56,19 +112,8 @@ class BytesBuilder {
     _length = required;
   }
 
-  /**
-   * Append [byte] to the current contents of the builder.
-   *
-   * The [byte] will be bit-representation truncated to the range 0 .. 255.
-   */
   void addByte(int byte) => add([byte]);
 
-  /**
-   * Returns the contents of `this` and clears `this`.
-   *
-   * The list returned is a view of the the internal buffer, limited to the
-   * [length].
-   */
   List<int> takeBytes() {
     if (_buffer == null) return new Uint8List(0);
     var buffer = new Uint8List.view(_buffer.buffer, 0, _length);
@@ -76,35 +121,18 @@ class BytesBuilder {
     return buffer;
   }
 
-  /**
-   * Returns a copy of the current contents of the builder.
-   *
-   * Leaves the contents of the builder intact.
-   */
   List<int> toBytes() {
     if (_buffer == null) return new Uint8List(0);
     return new Uint8List.fromList(
         new Uint8List.view(_buffer.buffer, 0, _length));
   }
 
-  /**
-   * The number of bytes in the builder.
-   */
   int get length => _length;
 
-  /**
-   * Returns `true` if the buffer is empty.
-   */
   bool get isEmpty => _length == 0;
 
-  /**
-   * Returns `true` if the buffer is not empty.
-   */
   bool get isNotEmpty => _length != 0;
 
-  /**
-   * Clear the contents of the builder.
-   */
   void clear() {
     _length = 0;
     _buffer = null;
@@ -120,3 +148,58 @@ class BytesBuilder {
     return x + 1;
   }
 }
+
+
+class _NonCopyingBytesBuilder implements BytesBuilder {
+  int _length = 0;
+  final List _chunks = [];
+
+  void add(List<int> bytes) {
+    if (bytes is! Uint8List) {

[Søren Gjesse, 2014/04/01 08:58:40]

This might be surprising, as for the non-copying we sometimes copy anyway. If
the user think that whatever is added can be changed until takeBytes is called
this might not always be the case. We could just postpone this copy to takeBytes
- if we even need to copy.

[Anders Johnsen, 2014/04/01 09:29:07]

We do say that there is an internal list and promise that as a result. It should
not be a surprise that if you add anything else than typed-data, we'll have to
copy. I have changed it to say it will copy if needed.

+      bytes = new Uint8List.fromList(bytes);
+    }
+    _chunks.add(bytes);
+    _length += bytes.length;
+  }
+
+  void addByte(int byte) => add([byte]);
+
+  List<int> takeBytes() {
+    if (_chunks.length == 0) return new Uint8List(0);
+    if (_chunks.length == 1) {
+      var buffer = _chunks.single;
+      clear();
+      return buffer;
+    }
+    var buffer = new Uint8List(_length);
+    int offset = 0;
+    for (var chunk in _chunks) {
+      buffer.setRange(offset, offset + chunk.length, chunk);
+      offset += chunk.length;
+    }
+    clear();
+    return buffer;
+  }
+
+  List<int> toBytes() {
+    if (_chunks.length == 0) return new Uint8List(0);
+    var buffer = new Uint8List(_length);
+    int offset = 0;
+    for (var chunk in _chunks) {
+      buffer.setRange(offset, offset + chunk.length, chunk);
+      offset += chunk.length;
+    }
+    return buffer;
+  }
+
+  int get length => _length;
+
+  bool get isEmpty => _length == 0;
+
+  bool get isNotEmpty => _length != 0;
+
+  void clear() {
+    _length = 0;
+    _chunks.clear();
+  }
+}