[scanner] Use 'vm:unsafe:no-bounds-checks' and add explicit bounds checks

The (utf8) scanner currently has this thing where you give it a
0-terminated byte-array (i.e. you read the file, then allocate
something that's 1 bigger, copy the data, then give it to the scanner)
to 'avoid bounds checks'.
Dart still has bounds checks though - they're just implicit.

As for the string scanner ut gets a string, then creates a new string
like `string + '\x00'` - so basically the same thing.

This CL uses the 'vm:unsafe:no-bounds-checks' pragma, removing the
implicit bounds checks, adding explicit bounds checks,
saving ~73.6 mio instructions when compiling the CFE in the process:

```
Comparing snapshot #1 with snapshot #2
cycles:u: -0.9983% +/- 0.6563% (-174026333.30 +/- 114410028.98)
instructions:u: -0.3416% +/- 0.0005% (-73659267.00 +/- 108567.20)
branch-misses:u: -4.8952% +/- 2.2612% (-3172939.50 +/- 1465641.18)
```

With the scanner-benchmark with `--bytes` I get this:

```
msec task-clock:u: -1.2251% +/- 0.6355% (-50.64 +/- 26.27)
cycles:u: -1.2376% +/- 0.6385% (-223642830.80 +/- 115393789.68)
instructions:u: -2.8155% +/- 0.0000% (-1153243856.00 +/- 428.11)
seconds time elapsed: -1.2165% +/- 0.6408% (-0.05 +/- 0.03)
seconds user: -1.1539% +/- 0.6495% (-0.05 +/- 0.03)
```

With the scanner-benchmark with `--string` I get this:

```
msec task-clock:u: -7.6439% +/- 0.6628% (-366.08 +/- 31.74)
page-faults:u: -95.0034% +/- 0.0014% (-228023.50 +/- 3.41)
instructions:u: 2.1041% +/- 0.0000% (897941907.60 +/- 2082.79)
branch-misses:u: 3.2994% +/- 1.4675% (3239735.30 +/- 1440940.88)
seconds time elapsed: -7.6595% +/- 0.6610% (-0.37 +/- 0.03)
seconds user: -0.8801% +/- 0.7676% (-0.04 +/- 0.03)
seconds sys: -92.0140% +/- 2.8075% (-0.33 +/- 0.01)
MarkSweep(   old space) goes from 6 to 0
Notice combined GC time goes from 112 ms to 41 ms (notice only 1 run each).
```

Where I'll note that the 'vm:unsafe:no-bounds-checks' pragma doesn't
(yet?) work for `String.codeUnitAt`.
See https://dart-review.googlesource.com/c/sdk/+/384540
(and https://dart-review.googlesource.com/c/sdk/+/385201) for details.
I assume the relatively  big change here is caused by not allocating
a new string with a 0-byte in the end each time.

Note that the read-allocate-copy dance is still performed for the utf8
scanner in this CL as it requires changing all call-sites instead.
It will be done in a follow-up CL where the "end-of-file" int will
likely also be changed to `-1` to (I assume) allow for having the
0-byte in the middle of a file (see also the 10+ year old bug at
#18090)

Note: The pragma (currently?) only has effect in AOT and this change
will (for the utf8 scanner) make the JIT version slower
(probably by the same ~73.6 mio instructions as - at least in AOT -
the implicit check is 6 instructions and the explicit one is 3
instructions). As the pragma doesn't work in the StringScanner anyway
I expect the change to be somewhat equivalent there. Once the
read-allocate-copy dance is also removed from the utf8 scanner I expect
the combined result to be positive all around.

Update: With https://dart-review.googlesource.com/c/sdk/+/385201 landed
I get these changes:

Compiling the CFE:
```
instructions:u: -0.4520% +/- 0.0002% (-98470955.29 +/- 42253.40)
```

Scanner benchmark with `--bytes`:

```
msec task-clock:u: -2.1758% +/- 0.2316% (-92.07 +/- 9.80)
cycles:u: -2.1941% +/- 0.2283% (-405224983.11 +/- 42160655.88)
instructions:u: -3.1049% +/- 0.0000% (-1272360052.95 +/- 706.54)
branch-misses:u: 2.4718% +/- 0.5142% (2371345.23 +/- 493257.76)
seconds time elapsed: -2.1761% +/- 0.2317% (-0.09 +/- 0.01)
seconds user: -2.2071% +/- 0.2308% (-0.09 +/- 0.01)
```

Scanner benchmark with `--string`:

```
msec task-clock:u: -15.0073% +/- 0.2175% (-745.93 +/- 10.81)
page-faults:u: -95.0035% +/- 0.0003% (-228024.25 +/- 0.81)
cycles:u: -7.7986% +/- 0.2329% (-1558985588.99 +/- 46560962.79)
instructions:u: -3.7054% +/- 0.0000% (-1581977447.66 +/- 481.68)
branch-misses:u: -0.6880% +/- 0.5818% (-689453.22 +/- 583101.50)
seconds time elapsed: -15.0198% +/- 0.2170% (-0.75 +/- 0.01)
seconds user: -8.8149% +/- 0.2648% (-0.41 +/- 0.01)
seconds sys: -94.1247% +/- 1.6444% (-0.34 +/- 0.01)
MarkSweep(   old space) goes from 6 to 0
```

Change-Id: I524a21f488da7df5dc9d2cdf40112b84896ad3e0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/383324
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/_fe_analyzer_shared/lib/src/scanner/string_scanner.dart

@@ -35,22 +35,24 @@ import 'error_token.dart' show ErrorToken;
  */
 class StringScanner extends AbstractScanner {
   /** The file content. */
-  final String string;
+  final String _string;
+  final int _stringLengthMinusOne;
 
-  /** The current offset in [string]. */
+  /** The current offset in [_string]. */
   @override
   int scanOffset = -1;
 
-  StringScanner(String string,
+  StringScanner(this._string,
       {ScannerConfiguration? configuration,
       bool includeComments = false,
       LanguageVersionChanged? languageVersionChanged})
-      : string = ensureZeroTermination(string),
+      : _stringLengthMinusOne = _string.length - 1,
         super(configuration, includeComments, languageVersionChanged,
-            numberOfBytesHint: string.length);
+            numberOfBytesHint: _string.length);
 
   StringScanner.recoveryOptionScanner(StringScanner super.copyFrom)
-      : string = copyFrom.string,
+      : _string = copyFrom._string,
+        _stringLengthMinusOne = copyFrom._stringLengthMinusOne,
         scanOffset = copyFrom.scanOffset,
         super.recoveryOptionScanner();
 
@@ -59,23 +61,28 @@ class StringScanner extends AbstractScanner {
     return new StringScanner.recoveryOptionScanner(this);
   }
 
-  static String ensureZeroTermination(String string) {
-    return (string.isEmpty || string.codeUnitAt(string.length - 1) != 0)
-        // TODO(lry): abort instead of copying the array, or warn?
-        ? string + '\x00'
-        : string;
-  }
-
   static bool isLegalIdentifier(String identifier) {
     StringScanner scanner = new StringScanner(identifier);
     Token startToken = scanner.tokenize();
     return startToken is! ErrorToken && startToken.next!.isEof;
   }
 
   @override
-  int advance() => string.codeUnitAt(++scanOffset);
+  @pragma('vm:unsafe:no-bounds-checks')
+  int advance() {
+    // Always increment so scanOffset goes past the end.
+    ++scanOffset;
+    if (scanOffset > _stringLengthMinusOne) return 0;
+    return _string.codeUnitAt(scanOffset);
+  }
+
   @override
-  int peek() => string.codeUnitAt(scanOffset + 1);
+  @pragma('vm:unsafe:no-bounds-checks')
+  int peek() {
+    int next = scanOffset + 1;
+    if (next > _stringLengthMinusOne) return 0;
+    return _string.codeUnitAt(next);
+  }
 
   @override
   int get stringOffset => scanOffset;
@@ -90,7 +97,7 @@ class StringScanner extends AbstractScanner {
   analyzer.StringToken createSubstringToken(TokenType type, int start,
       bool asciiOnly, int extraOffset, bool allowLazy) {
     return new StringTokenImpl.fromSubstring(
-        type, string, start, scanOffset + extraOffset, tokenStart,
+        type, _string, start, scanOffset + extraOffset, tokenStart,
         canonicalize: true,
         precedingComments: comments,
         allowLazyFoo: allowLazy);
@@ -100,9 +107,9 @@ class StringScanner extends AbstractScanner {
   analyzer.StringToken createSyntheticSubstringToken(
       TokenType type, int start, bool asciiOnly, String syntheticChars) {
     String value = syntheticChars.length == 0
-        ? canonicalizeSubString(string, start, scanOffset)
+        ? canonicalizeSubString(_string, start, scanOffset)
         : canonicalizeString(
-            string.substring(start, scanOffset) + syntheticChars);
+            _string.substring(start, scanOffset) + syntheticChars);
     return new SyntheticStringToken(
         type, value, tokenStart, value.length - syntheticChars.length);
   }
@@ -111,26 +118,29 @@ class StringScanner extends AbstractScanner {
   CommentToken createCommentToken(TokenType type, int start, bool asciiOnly,
       [int extraOffset = 0]) {
     return new CommentTokenImpl.fromSubstring(
-        type, string, start, scanOffset + extraOffset, tokenStart,
+        type, _string, start, scanOffset + extraOffset, tokenStart,
         canonicalize: true);
   }
 
   @override
   DartDocToken createDartDocToken(TokenType type, int start, bool asciiOnly,
       [int extraOffset = 0]) {
     return new DartDocToken.fromSubstring(
-        type, string, start, scanOffset + extraOffset, tokenStart,
+        type, _string, start, scanOffset + extraOffset, tokenStart,
         canonicalize: true);
   }
 
   @override
   LanguageVersionToken createLanguageVersionToken(
       int start, int major, int minor) {
     return new LanguageVersionTokenImpl.fromSubstring(
-        string, start, scanOffset, tokenStart, major, minor,
+        _string, start, scanOffset, tokenStart, major, minor,
         canonicalize: true);
   }
 
   @override
-  bool atEndOfFile() => scanOffset >= string.length - 1;
+  // To preserve old behavior we only return true once advance has been out of
+  // bounds. This should probably change. It's at least used in tests
+  // (where the eof token has its offset reduced by one to 'fix' this.)
+  bool atEndOfFile() => scanOffset > _stringLengthMinusOne;
 }

pkg/_fe_analyzer_shared/lib/src/scanner/utf8_bytes_scanner.dart

@@ -38,7 +38,8 @@ class Utf8BytesScanner extends AbstractScanner {
    *
    * The content is zero-terminated.
    */
-  final Uint8List bytes;
+  final Uint8List _bytes;
+  final int _bytesLengthMinusOne;
 
   /**
    * Points to the offset of the last byte returned by [advance].
@@ -94,15 +95,16 @@ class Utf8BytesScanner extends AbstractScanner {
    * array whose last element is '0' to signal the end of the file. If this
    * is not the case, the entire array is copied before scanning.
    */
-  Utf8BytesScanner(this.bytes,
+  Utf8BytesScanner(this._bytes,
       {ScannerConfiguration? configuration,
       bool includeComments = false,
       LanguageVersionChanged? languageVersionChanged,
       bool allowLazyStrings = true})
-      : super(configuration, includeComments, languageVersionChanged,
-            numberOfBytesHint: bytes.length,
+      : _bytesLengthMinusOne = _bytes.length - 1,
+        super(configuration, includeComments, languageVersionChanged,
+            numberOfBytesHint: _bytes.length,
             allowLazyStrings: allowLazyStrings) {
-    assert(bytes.last == 0);
+    assert(_bytes.last == 0);
     // Skip a leading BOM.
     if (containsBomAt(/* offset = */ 0)) {
       byteOffset += 3;
@@ -111,7 +113,8 @@ class Utf8BytesScanner extends AbstractScanner {
   }
 
   Utf8BytesScanner.createRecoveryOptionScanner(Utf8BytesScanner copyFrom)
-      : bytes = copyFrom.bytes,
+      : _bytes = copyFrom._bytes,
+        _bytesLengthMinusOne = copyFrom._bytesLengthMinusOne,
         super.recoveryOptionScanner(copyFrom) {
     this.byteOffset = copyFrom.byteOffset;
     this.scanSlack = copyFrom.scanSlack;
@@ -127,17 +130,28 @@ class Utf8BytesScanner extends AbstractScanner {
   bool containsBomAt(int offset) {
     const List<int> BOM_UTF8 = const [0xEF, 0xBB, 0xBF];
 
-    return offset + 3 < bytes.length &&
-        bytes[offset] == BOM_UTF8[0] &&
-        bytes[offset + 1] == BOM_UTF8[1] &&
-        bytes[offset + 2] == BOM_UTF8[2];
+    return offset + 3 < _bytes.length &&
+        _bytes[offset] == BOM_UTF8[0] &&
+        _bytes[offset + 1] == BOM_UTF8[1] &&
+        _bytes[offset + 2] == BOM_UTF8[2];
   }
 
   @override
-  int advance() => bytes[++byteOffset];
+  @pragma('vm:unsafe:no-bounds-checks')
+  int advance() {
+    // Always increment so byteOffset goes past the end.
+    ++byteOffset;
+    if (byteOffset > _bytesLengthMinusOne) return 0;
+    return _bytes[byteOffset];
+  }
 
   @override
-  int peek() => bytes[byteOffset + 1];
+  @pragma('vm:unsafe:no-bounds-checks')
+  int peek() {
+    int next = byteOffset + 1;
+    if (next > _bytesLengthMinusOne) return 0;
+    return _bytes[next];
+  }
 
   /// Returns the unicode code point starting at the byte offset [startOffset]
   /// with the byte [nextByte].
@@ -154,9 +168,10 @@ class Utf8BytesScanner extends AbstractScanner {
     } else {
       expectedHighBytes = 1; // Bad code unit.
     }
+    // TODO(jensj): Don't we need a bounds check here? Can't I crash this?
     int numBytes = 0;
     for (int i = 0; i < expectedHighBytes; i++) {
-      if (bytes[byteOffset + i] < 0x80) {
+      if (_bytes[byteOffset + i] < 0x80) {
         break;
       }
       numBytes++;
@@ -169,7 +184,7 @@ class Utf8BytesScanner extends AbstractScanner {
     // TODO(lry): measurably slow, decode creates first a Utf8Decoder and a
     // _Utf8Decoder instance. Also the sublist is eagerly allocated.
     String codePoint =
-        utf8.decode(bytes.sublist(startOffset, end), allowMalformed: true);
+        utf8.decode(_bytes.sublist(startOffset, end), allowMalformed: true);
     if (codePoint.length == 0) {
       // The UTF-8 decoder discards leading BOM characters.
       // TODO(floitsch): don't just assume that removed characters were the
@@ -214,7 +229,7 @@ class Utf8BytesScanner extends AbstractScanner {
     int end = byteOffset;
     // TODO(lry): this measurably slows down the scanner for files with unicode.
     String s =
-        utf8.decode(bytes.sublist(startScanOffset, end), allowMalformed: true);
+        utf8.decode(_bytes.sublist(startScanOffset, end), allowMalformed: true);
     utf8Slack += (end - startScanOffset) - s.length;
   }
 
@@ -246,17 +261,18 @@ class Utf8BytesScanner extends AbstractScanner {
   analyzer.StringToken createSubstringToken(TokenType type, int start,
       bool asciiOnly, int extraOffset, bool allowLazy) {
     return new StringTokenImpl.fromUtf8Bytes(
-        type, bytes, start, byteOffset + extraOffset, asciiOnly, tokenStart,
+        type, _bytes, start, byteOffset + extraOffset, asciiOnly, tokenStart,
         precedingComments: comments, allowLazyFoo: allowLazy);
   }
 
   @override
   analyzer.StringToken createSyntheticSubstringToken(
       TokenType type, int start, bool asciiOnly, String syntheticChars) {
     String value = syntheticChars.length == 0
-        ? canonicalizeUtf8SubString(bytes, start, byteOffset, asciiOnly)
+        ? canonicalizeUtf8SubString(_bytes, start, byteOffset, asciiOnly)
         : canonicalizeString(
-            decodeString(bytes, start, byteOffset, asciiOnly) + syntheticChars);
+            decodeString(_bytes, start, byteOffset, asciiOnly) +
+                syntheticChars);
     return new SyntheticStringToken(
         type, value, tokenStart, value.length - syntheticChars.length);
   }
@@ -266,23 +282,23 @@ class Utf8BytesScanner extends AbstractScanner {
       TokenType type, int start, bool asciiOnly,
       [int extraOffset = 0]) {
     return new CommentTokenImpl.fromUtf8Bytes(
-        type, bytes, start, byteOffset + extraOffset, asciiOnly, tokenStart);
+        type, _bytes, start, byteOffset + extraOffset, asciiOnly, tokenStart);
   }
 
   @override
   DartDocToken createDartDocToken(TokenType type, int start, bool asciiOnly,
       [int extraOffset = 0]) {
     return new DartDocToken.fromUtf8Bytes(
-        type, bytes, start, byteOffset + extraOffset, asciiOnly, tokenStart);
+        type, _bytes, start, byteOffset + extraOffset, asciiOnly, tokenStart);
   }
 
   @override
   LanguageVersionToken createLanguageVersionToken(
       int start, int major, int minor) {
     return new LanguageVersionTokenImpl.fromUtf8Bytes(
-        bytes, start, byteOffset, tokenStart, major, minor);
+        _bytes, start, byteOffset, tokenStart, major, minor);
   }
 
   @override
-  bool atEndOfFile() => byteOffset >= bytes.length - 1;
+  bool atEndOfFile() => byteOffset >= _bytesLengthMinusOne;
 }