[M] Change in dart/sdk[main]: [analyzer] Replace regex in ignore_info with normal code
0 views
Skip to first unread message
Jens Johansen (Gerrit)
Mar 11, 2026, 11:01:15 AMMar 11
to Johnni Winther, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Brian Wilkerson and Johnni Winther
Jens Johansen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Jens Johansen . resolved
Adding Brian for the changes to analysis_server.
Related details
Attention is currently required from:
- Brian Wilkerson
- Johnni Winther
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Mar 11, 2026, 12:10:55 PMMar 11
to Jens Johansen, Brian Wilkerson, Johnni Winther, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Jens Johansen and Johnni Winther
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
Patchset-level comments
Brian Wilkerson . resolved
The analysis server changes lgtm.
The API is an improvement, though I have to say that the implementation is quite inelegant. It's kind of sad that we have to write out individual offsets and character checks rather than something like `startsWith('ignore:`)` in order to get the performance improvement.
Related details
Attention is currently required from:
- Jens Johansen
- Johnni Winther
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Mar 12, 2026, 4:57:10 AMMar 12
to Jens Johansen, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Jens Johansen
Johnni Winther voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Jens Johansen
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jens Johansen (Gerrit)
Mar 13, 2026, 5:10:47 AMMar 13
to Johnni Winther, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Jens Johansen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jens Johansen
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Brian Wilkerson . resolved
The analysis server changes lgtm.
The API is an improvement, though I have to say that the implementation is quite inelegant. It's kind of sad that we have to write out individual offsets and character checks rather than something like `startsWith('ignore:`)` in order to get the performance improvement.
Jens Johansen
I did some experiments:
- With regex `isIgnoreForFileComment` (including children) cost ~264 mio instructions and the (combined) regex part was about 196 mio instructions.
- Doing it like this takes `processPrecedingComments` (including children) to ~74.4 mio instructions where the `isIgnoreComment` is ~4.7 mio and `isIgnoreForFileComment` is ~3.2 mio.
- If instead doing something like:
```
static bool isIgnoreComment(String s) {
int end = s.length;
if (end < 9) return false;
// Require 2 slashes.
if (!s.startsWith("//")) return false;
// Allow more slashes.
int from = 2;
for (; from < end; from++) {
// if (s.codeUnitAt(from) != $SLASH) break;
if (!s.startsWith("/", from)) break;
}
// Skip any spaces.
for (; from < end; from++) {
// if (s.codeUnitAt(from) != $SPACE) break;
if (!s.startsWith(" ", from)) break;
}
// Does the string match 'ignore:' now?
if (end - from < 7) return false;
return s.startsWith("ignore:", from);
}
```
(and similar for `isIgnoreForFileComment`)
we take `isIgnoreForFileComment` (including children) to ~102 mio instructions where `isIgnoreComment` is ~21.8 mio and `isIgnoreForFileComment` is ~13.3 mio.
This is still a significant speedup compared to using regex --- but quite a bit more expensive than doing the code units stuff.
If I add
```
extension on String {
bool startsWith2(String pattern, [int index = 0]) {
if (length < index + pattern.length) return false;
// ignore: prefer_is_empty
if (pattern.length == 0) return true;
int end = pattern.length;
int i = 0;
do {
if (pattern.codeUnitAt(i) != codeUnitAt(index)) return false;
i++;
index++;
} while (i < end);
return true;
}
}
```
and use `substring2` instead `isIgnoreForFileComment` lands at ~80 mio with `isIgnoreComment` at ~8 mio and `isIgnoreForFileComment` at ~5.5 mio. Still more expensive than this, but less expensive than the normal `startsWith`. Adding `@pragma("vm:never-inline")` to it brings it to ~20 and ~11 though, so much closer to the normal `startsWith`.
Anyway, I'll land this as-is.
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Mar 13, 2026, 5:43:28 AMMar 13
to Jens Johansen, Johnni Winther, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[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 <je...@google.com>
Reviewed-by: Brian Wilkerson <brianwi...@google.com>
Reviewed-by: Johnni Winther <johnni...@google.com>
Files:
- M pkg/analysis_server/lib/src/services/correction/organize_imports.dart
- M pkg/analysis_server_plugin/lib/src/correction/ignore_diagnostic.dart
- M pkg/analyzer/lib/src/ignore_comments/ignore_info.dart
- M pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart
Change size: M
Delta: 4 files changed, 138 insertions(+), 18 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Johnni Winther, +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages