From 2849d34c7cec155f123a5f46df4ec3c2d8e2950f Mon Sep 17 00:00:00 2001 From: Jens Johansen Date: Fri, 13 Mar 2026 02:43:23 -0700 Subject: [PATCH] [analyzer] Replace regex in ignore_info with normal code The RegEx engine in the VM was updated in e443b89f238cf97b7d33eb54ca34fc19ef44f52f which caused the analyzer analyzing the CFE to use ~150 mio instructions more. Part of this was an increased cost in ignore comment processing which relied on regex. Using regex before the updated engine made `processPrecedingComments` have a cost of ~240 mio instructions, updating the regex engine took that to ~264 mio instructions. This CL gets rid of the regex and takes the cost of `processPrecedingComments` to ~74.4 mio instructions a saving of about 189 mio instructions (all then analyzing the CFE and looking at output from `valgrind --tool=callgrind`). Benchmarking with `perf stat` with normal GC gives: ``` task-clock:u: -2.1760% +/- 1.6063% (-265608341.80 +/- 196062087.98) (12206056036.20 -> 11940447694.40) page-faults:u: 0.2301% +/- 0.0313% (448.20 +/- 60.88) (194764.60 -> 195212.80) cycles:u: -2.2906% +/- 1.6090% (-1180119481.60 +/- 828994273.75) (51521138476.00 -> 50341018994.40) instructions:u: -0.3325% +/- 0.0032% (-196547942.60 +/- 1874215.18) (59120651337.60 -> 58924103395.00) seconds time elapsed: -2.1715% +/- 1.6011% (-0.27 +/- 0.20) (12.21 -> 11.95) seconds user: -2.2487% +/- 1.7816% (-0.27 +/- 0.21) (11.87 -> 11.60) Comparing GC data: 'No' GC change. ``` Note that it must push the GC - the savings isn't really 2% in time. And with GC disabled: ``` instructions:u: -0.4562% +/- 0.0029% (-185499444.00 +/- 1189012.77) (40663084597.80 -> 40477585153.80) ``` So here a saving of ~185 mio which fits okay with the data from valgrind. Change-Id: Ib203baeac6a93f5e37c737080fed342dbd0740a7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/487021 Commit-Queue: Jens Johansen Reviewed-by: Brian Wilkerson Reviewed-by: Johnni Winther --- .../services/correction/organize_imports.dart | 2 +- .../lib/src/correction/ignore_diagnostic.dart | 4 +- .../lib/src/ignore_comments/ignore_info.dart | 92 ++++++++++++++++--- .../src/ignore_comments/ignore_info_test.dart | 58 +++++++++++- 4 files changed, 138 insertions(+), 18 deletions(-) diff --git a/pkg/analysis_server/lib/src/services/correction/organize_imports.dart b/pkg/analysis_server/lib/src/services/correction/organize_imports.dart index b175e364307..6da8e3f6f9a 100644 --- a/pkg/analysis_server/lib/src/services/correction/organize_imports.dart +++ b/pkg/analysis_server/lib/src/services/correction/organize_imports.dart @@ -359,7 +359,7 @@ class ImportOrganizer { /// Returns whether this token is a '// ignore:' comment (but not an /// '// ignore_for_file:' comment). static bool _isIgnoreComment(Token token) => - IgnoreInfo.ignoreMatcher.matchAsPrefix(token.lexeme) != null; + IgnoreInfo.isIgnoreComment(token.lexeme); static bool _isLibraryTargetAnnotation(Annotation annotation) => annotation.elementAnnotation?.targetKinds.contains(TargetKind.library) ?? diff --git a/pkg/analysis_server_plugin/lib/src/correction/ignore_diagnostic.dart b/pkg/analysis_server_plugin/lib/src/correction/ignore_diagnostic.dart index 7f2636e7105..5d987eaf8b2 100644 --- a/pkg/analysis_server_plugin/lib/src/correction/ignore_diagnostic.dart +++ b/pkg/analysis_server_plugin/lib/src/correction/ignore_diagnostic.dart @@ -171,7 +171,7 @@ class IgnoreDiagnosticInFile extends _DartIgnoreDiagnostic { var line = source.substring(lineStart, nextLineStart); var trimmedLine = line.trim(); - if (trimmedLine.startsWith(IgnoreInfo.ignoreForFileMatcher)) { + if (IgnoreInfo.isIgnoreForFileComment(trimmedLine)) { // Found an existing ignore; insert after `// ignore_for_file: ` // before any existing codes. var insertOffset = lineStart + line.indexOf(':') + 1; @@ -243,7 +243,7 @@ class IgnoreDiagnosticOnLine extends _DartIgnoreDiagnostic { var lineStart = unitResult.lineInfo.getOffsetOfLine(lineNumber); var line = unitResult.content.substring(previousLineStart, lineStart); - if (line.trim().startsWith(IgnoreInfo.ignoreMatcher)) { + if (IgnoreInfo.isIgnoreComment(line.trim())) { // Add after the `// ignore: ` before any existing codes. var insertOffset = previousLineStart + line.indexOf(':') + 1; builder.addSimpleInsertion(insertOffset, ' $_code,'); diff --git a/pkg/analyzer/lib/src/ignore_comments/ignore_info.dart b/pkg/analyzer/lib/src/ignore_comments/ignore_info.dart index 71d1c73d1e1..b29ae7a947c 100644 --- a/pkg/analyzer/lib/src/ignore_comments/ignore_info.dart +++ b/pkg/analyzer/lib/src/ignore_comments/ignore_info.dart @@ -2,6 +2,7 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'package:_fe_analyzer_shared/src/scanner/characters.dart'; import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; import 'package:analyzer/diagnostic/diagnostic.dart'; @@ -83,16 +84,6 @@ sealed class IgnoredElement { /// Information about analysis `//ignore:` and `//ignore_for_file:` comments /// within a source file. class IgnoreInfo { - /// A regular expression for matching 'ignore' comments. - /// - /// Resulting codes may be in a list (e.g. 'error_code_1,error_code2'). - static final RegExp ignoreMatcher = RegExp(r'//+[ ]*ignore:'); - - /// A regular expression for matching 'ignore_for_file' comments. - /// - /// Resulting codes may be in a list (e.g. 'error_code_1,error_code2'). - static final RegExp ignoreForFileMatcher = RegExp(r'//[ ]*ignore_for_file:'); - /// A regular expression for matching 'ignore' comments in a .yaml file. /// /// Resulting codes may be in a list (e.g. 'error_code_1,error_code2'). @@ -219,6 +210,83 @@ class IgnoreInfo { (name) => name._matches(diagnosticCode, pluginName: pluginName), ); } + + /// If [s] starts as a 'ignore' comments. + /// + /// Resulting codes may be in a list (e.g. 'error_code_1,error_code2'). + static bool isIgnoreComment(String s) { + int end = s.length; + if (end < 9) return false; + + // Require 2 slashes. + if (s.codeUnitAt(0) != $SLASH) return false; + if (s.codeUnitAt(1) != $SLASH) return false; + + // Allow more slashes. + int from = 2; + for (; from < end; from++) { + if (s.codeUnitAt(from) != $SLASH) break; + } + + // Skip any spaces. + for (; from < end; from++) { + if (s.codeUnitAt(from) != $SPACE) break; + } + + // Does the string match 'ignore:' now? + if (end - from < 7) return false; + if (s.codeUnitAt(from++) != $i) return false; + if (s.codeUnitAt(from++) != $g) return false; + if (s.codeUnitAt(from++) != $n) return false; + if (s.codeUnitAt(from++) != $o) return false; + if (s.codeUnitAt(from++) != $r) return false; + if (s.codeUnitAt(from++) != $e) return false; + if (s.codeUnitAt(from++) != $COLON) return false; + return true; + } + + /// If [s] starts as a 'ignore_for_file' comments. + /// + /// Resulting codes may be in a list (e.g. 'error_code_1,error_code2'). + static bool isIgnoreForFileComment(String s) { + int end = s.length; + if (end < 18) return false; + + // Require 2 slashes. + if (s.codeUnitAt(0) != $SLASH) return false; + if (s.codeUnitAt(1) != $SLASH) return false; + + // We don't (currently?) allow more than 2 slashes. + int from = 2; + // for (; from < end; from++) { + // if (s.codeUnitAt(from) != $SLASH) break; + // } + + // Skip any spaces. + for (; from < end; from++) { + if (s.codeUnitAt(from) != $SPACE) break; + } + + // Does the string match 'ignore_for_file:' now? + if (end - from < 16) return false; + if (s.codeUnitAt(from++) != $i) return false; + if (s.codeUnitAt(from++) != $g) return false; + if (s.codeUnitAt(from++) != $n) return false; + if (s.codeUnitAt(from++) != $o) return false; + if (s.codeUnitAt(from++) != $r) return false; + if (s.codeUnitAt(from++) != $e) return false; + if (s.codeUnitAt(from++) != $_) return false; + if (s.codeUnitAt(from++) != $f) return false; + if (s.codeUnitAt(from++) != $o) return false; + if (s.codeUnitAt(from++) != $r) return false; + if (s.codeUnitAt(from++) != $_) return false; + if (s.codeUnitAt(from++) != $f) return false; + if (s.codeUnitAt(from++) != $i) return false; + if (s.codeUnitAt(from++) != $l) return false; + if (s.codeUnitAt(from++) != $e) return false; + if (s.codeUnitAt(from++) != $COLON) return false; + return true; + } } extension CommentTokenExtension on CommentToken { @@ -412,9 +480,9 @@ extension CompilationUnitExtension on CompilationUnit { var comment = currentToken.precedingComments; while (comment != null) { var lexeme = comment.lexeme; - if (lexeme.startsWith(IgnoreInfo.ignoreMatcher)) { + if (IgnoreInfo.isIgnoreComment(lexeme)) { result.add(comment); - } else if (lexeme.startsWith(IgnoreInfo.ignoreForFileMatcher)) { + } else if (IgnoreInfo.isIgnoreForFileComment(lexeme)) { result.add(comment); } comment = comment.next as CommentToken?; diff --git a/pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart b/pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart index 94ea20bd910..8cb9ba4dc44 100644 --- a/pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart +++ b/pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart @@ -2,7 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -import 'package:analyzer/src/dart/ast/token.dart'; import 'package:analyzer/src/ignore_comments/ignore_info.dart'; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -17,6 +16,55 @@ main() { @reflectiveTest class IgnoreInfoTest extends PubPackageResolutionTest { + test_allows_several_slashes_and_spaces() async { + var ignoredElements = await _parseIgnoredElements( + '/////// ignore: type =lint', + ); + expect(ignoredElements, hasLength(1)); + _expectIgnoredType( + ignoredElements[0], + type: 'lint', + offset: 19, + length: 10, + ); + } + + test_ignore_for_file() async { + var ignoredElements = await _parseIgnoredElements( + '// ignore_for_file: type =lint', + ); + expect(ignoredElements, hasLength(1)); + _expectIgnoredType( + ignoredElements[0], + type: 'lint', + offset: 20, + length: 10, + ); + } + + test_ignore_for_file_several_spaces() async { + var ignoredElements = await _parseIgnoredElements( + '// ignore_for_file: type =lint', + ); + expect(ignoredElements, hasLength(1)); + _expectIgnoredType( + ignoredElements[0], + type: 'lint', + offset: 23, + length: 10, + ); + } + + test_ignore_in_file_needs_exactly_two_slashes() async { + var ignoredElements = await _parseIgnoredElements( + '/// ignore_on_file: type =lint', + ); + if (ignoredElements.length == 1) { + print((ignoredElements[0] as dynamic).st); + } + expect(ignoredElements, hasLength(0)); + } + test_name_multiple() async { var ignoredElements = await _parseIgnoredElements('// ignore: foo, bar'); expect(ignoredElements, hasLength(2)); @@ -237,7 +285,11 @@ class IgnoreInfoTest extends PubPackageResolutionTest { $comment int x = 1; '''); - var commentToken = result.unit.beginToken.precedingComments as CommentToken; - return commentToken.ignoredElements.toList(); + // This corresponds roughly to what happens in `IgnoreInfo.forDart`. + List ignoreResult = []; + for (var comment in result.unit.ignoreComments) { + ignoreResult.addAll(comment.ignoredElements); + } + return ignoreResult; } }