[js_runtime] Try to avoid String.prototype.replace with replacement patterns
Change-Id: I4b063fe77592bbb0ad9f6ffb628c3bdd6323b20c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/96220
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Jenny Messerly <jmesserly@google.com>
Commit-Queue: Stephen Adams <sra@google.com>
diff --git a/pkg/dev_compiler/tool/input_sdk/private/string_helper.dart b/pkg/dev_compiler/tool/input_sdk/private/string_helper.dart
index 41a51d6..aeb4e2f 100644
--- a/pkg/dev_compiler/tool/input_sdk/private/string_helper.dart
+++ b/pkg/dev_compiler/tool/input_sdk/private/string_helper.dart
@@ -162,9 +162,7 @@
return result.toString();
}
} else {
- var quoted = quoteStringForRegExp(pattern);
- var replacer = JS('', "new RegExp(#, 'g')", quoted);
- return stringReplaceJS(receiver, replacer, replacement);
+ return JS('String', '#.split(#).join(#)', receiver, pattern, replacement);
}
} else if (pattern is JSSyntaxRegExp) {
var re = regExpGetGlobalNative(pattern);
diff --git a/sdk/lib/_internal/js_runtime/lib/js_string.dart b/sdk/lib/_internal/js_runtime/lib/js_string.dart
index b5be305..39370d7 100644
--- a/sdk/lib/_internal/js_runtime/lib/js_string.dart
+++ b/sdk/lib/_internal/js_runtime/lib/js_string.dart
@@ -59,8 +59,7 @@
}
String replaceAll(Pattern from, String to) {
- checkString(to);
- return stringReplaceAllUnchecked(this, from, to);
+ return stringReplaceAllUnchecked(this, from, checkString(to));
}
String replaceAllMapped(Pattern from, String convert(Match match)) {
diff --git a/sdk/lib/_internal/js_runtime/lib/string_helper.dart b/sdk/lib/_internal/js_runtime/lib/string_helper.dart
index 0180369..ea5374e 100644
--- a/sdk/lib/_internal/js_runtime/lib/string_helper.dart
+++ b/sdk/lib/_internal/js_runtime/lib/string_helper.dart
@@ -117,12 +117,23 @@
}
}
-stringReplaceJS(receiver, replacer, replacement) {
- // The JavaScript String.replace method recognizes replacement
- // patterns in the replacement string. Dart does not have that
- // behavior.
- replacement = JS('String', r'#.replace(/\$/g, "$$$$")', replacement);
- return JS('String', r'#.replace(#, #)', receiver, replacer, replacement);
+String stringReplaceJS(String receiver, jsRegExp, String replacement) {
+ return JS('String', r'#.replace(#, #)', receiver, jsRegExp,
+ escapeReplacement(replacement));
+}
+
+String escapeReplacement(String replacement) {
+ // The JavaScript `String.prototype.replace` method recognizes replacement
+ // patterns in the replacement string. Dart does not have that behavior, so
+ // the replacement patterns need to be escaped.
+
+ // `String.prototype.replace` tends to be slower when there are replacement
+ // patterns, and the escaping itself uses replacement patterns, so it is
+ // worthwhile checking for `$` first.
+ if (stringContainsStringUnchecked(replacement, r'$', 0)) {
+ return JS('String', r'#.replace(/\$/g, "$$$$")', replacement);
+ }
+ return replacement;
}
stringReplaceFirstRE(receiver, regexp, replacement, startIndex) {
@@ -136,38 +147,73 @@
/// Returns a string for a RegExp pattern that matches [string]. This is done by
/// escaping all RegExp metacharacters.
quoteStringForRegExp(string) {
- return JS('String', r'#.replace(/[[\]{}()*+?.\\^$|]/g, "\\$&")', string);
+ // We test and replace essentially the same RegExp because replacement when
+ // there are replacement patterns is slow enough to be worth avoiding.
+ if (JS('bool', r'/[[\]{}()*+?.\\^$|]/.test(#)', string)) {
+ return JS('String', r'#.replace(/[[\]{}()*+?.\\^$|]/g, "\\$&")', string);
+ }
+ return string;
}
stringReplaceAllUnchecked(receiver, pattern, replacement) {
checkString(replacement);
if (pattern is String) {
- if (pattern == "") {
- if (receiver == "") {
- return JS('String', '#', replacement); // help type inference.
- } else {
- StringBuffer result = new StringBuffer('');
- int length = receiver.length;
- result.write(replacement);
- for (int i = 0; i < length; i++) {
- result.write(receiver[i]);
- result.write(replacement);
- }
- return result.toString();
- }
- } else {
- var quoted = quoteStringForRegExp(pattern);
- var replacer = JS('', "new RegExp(#, 'g')", quoted);
- return stringReplaceJS(receiver, replacer, replacement);
- }
- } else if (pattern is JSSyntaxRegExp) {
+ return stringReplaceAllUncheckedString(receiver, pattern, replacement);
+ }
+
+ if (pattern is JSSyntaxRegExp) {
var re = regExpGetGlobalNative(pattern);
return stringReplaceJS(receiver, re, replacement);
- } else {
- checkNull(pattern);
- // TODO(floitsch): implement generic String.replace (with patterns).
- throw "String.replaceAll(Pattern) UNIMPLEMENTED";
}
+
+ checkNull(pattern);
+ // TODO(floitsch): implement generic String.replace (with patterns).
+ throw "String.replaceAll(Pattern) UNIMPLEMENTED";
+}
+
+/// Replaces all non-overlapping occurences of [pattern] in [receiver] with
+/// [replacement]. This should be replace with
+/// (String.prototype.replaceAll)[https://github.com/tc39/proposal-string-replace-all]
+/// when available.
+String stringReplaceAllUncheckedString(
+ String receiver, String pattern, String replacement) {
+ if (pattern == "") {
+ if (receiver == "") {
+ return JS('String', '#', replacement); // help type inference.
+ }
+ StringBuffer result = new StringBuffer('');
+ int length = receiver.length;
+ result.write(replacement);
+ for (int i = 0; i < length; i++) {
+ result.write(receiver[i]);
+ result.write(replacement);
+ }
+ return result.toString();
+ }
+
+ if (!const bool.fromEnvironment(
+ 'dart2js.testing.String.replaceAll.force.regexp')) {
+ // First check for no match.
+ int index = stringIndexOfStringUnchecked(receiver, pattern, 0);
+ if (index < 0) return receiver;
+
+ // The fastest approach in general is to replace with a global RegExp, but
+ // this requires the receiver string to be long enough to amortize the cost
+ // of creating the RegExp, and the replacement to have no '$' patterns,
+ // which tend to make `String.prototype.replace` much slower. In these
+ // cases, using split-join usually wins.
+ if (receiver.length < 500 ||
+ stringContainsStringUnchecked(replacement, r'$', 0)) {
+ return stringReplaceAllUsingSplitJoin(receiver, pattern, replacement);
+ }
+ }
+ var quoted = quoteStringForRegExp(pattern);
+ var replacer = JS('', "new RegExp(#, 'g')", quoted);
+ return stringReplaceJS(receiver, replacer, replacement);
+}
+
+String stringReplaceAllUsingSplitJoin(receiver, pattern, replacement) {
+ return JS('String', '#.split(#).join(#)', receiver, pattern, replacement);
}
String _matchString(Match match) => match[0];
diff --git a/tests/compiler/dart2js/analyses/api_allowed.json b/tests/compiler/dart2js/analyses/api_allowed.json
index 3139ef5..a7a08a3 100644
--- a/tests/compiler/dart2js/analyses/api_allowed.json
+++ b/tests/compiler/dart2js/analyses/api_allowed.json
@@ -28,8 +28,8 @@
"Dynamic invocation of '_js_helper::_execGlobal'.": 1,
"Dynamic access of 'start'.": 1,
"Dynamic access of 'end'.": 1,
- "Dynamic access of 'length'.": 4,
- "Dynamic invocation of '[]'.": 2,
+ "Dynamic access of 'length'.": 3,
+ "Dynamic invocation of '[]'.": 1,
"Dynamic invocation of 'call'.": 13,
"Dynamic invocation of 'codeUnitAt'.": 2,
"Dynamic access of 'iterator'.": 2,
@@ -244,4 +244,4 @@
"Dynamic access of 'port'.": 1,
"Dynamic invocation of 'dart._http::_toJSON'.": 1
}
-}
\ No newline at end of file
+}
diff --git a/tests/corelib_2/string_replace_all_2_test.dart b/tests/corelib_2/string_replace_all_2_test.dart
new file mode 100644
index 0000000..39d196a
--- /dev/null
+++ b/tests/corelib_2/string_replace_all_2_test.dart
@@ -0,0 +1,11 @@
+// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file
+// 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.
+//
+// dart2jsOptions=-Ddart2js.testing.String.replaceAll.force.regexp=true
+
+import "string_replace_all_test.dart" as base;
+
+main() {
+ base.main();
+}
