Make RegExpFlagsFromString faster

The new ObjectPtr design makes non-inlined helper functions a little
more expensive because "this" is always a pointer where pass-by-value
would be more efficient, which is an issue for functions whose size puts
them right at the threshold of getting inlined or not. String::Get falls
into this category when called from RegExpFlagsFromString. In this case,
we can do even better than restoring inlineability by fine-tuning
the control flow a bit.

This should repair the regression in crbug.com/910573

Bug: chromium:910573
Change-Id: Ie6b68ef01cd978ec502d8d6c1da788c77422dce7
Reviewed-on: https://chromium-review.googlesource.com/c/1369087
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58234}

src/objects.cc

@@ -16626,40 +16626,75 @@ Handle<Object> JSPromise::TriggerPromiseReactions(Isolate* isolate,
 
 namespace {
 
-JSRegExp::Flags RegExpFlagsFromString(Handle<String> flags, bool* success) {
-  JSRegExp::Flags value = JSRegExp::kNone;
+constexpr JSRegExp::Flag kCharFlagValues[] = {
+    JSRegExp::kGlobal,      // g
+    JSRegExp::kInvalid,     // h
+    JSRegExp::kIgnoreCase,  // i
+    JSRegExp::kInvalid,     // j
+    JSRegExp::kInvalid,     // k
+    JSRegExp::kInvalid,     // l
+    JSRegExp::kMultiline,   // m
+    JSRegExp::kInvalid,     // n
+    JSRegExp::kInvalid,     // o
+    JSRegExp::kInvalid,     // p
+    JSRegExp::kInvalid,     // q
+    JSRegExp::kInvalid,     // r
+    JSRegExp::kDotAll,      // s
+    JSRegExp::kInvalid,     // t
+    JSRegExp::kUnicode,     // u
+    JSRegExp::kInvalid,     // v
+    JSRegExp::kInvalid,     // w
+    JSRegExp::kInvalid,     // x
+    JSRegExp::kSticky,      // y
+};
+
+constexpr JSRegExp::Flag CharToFlag(uc16 flag_char) {
+  return (flag_char < 'g' || flag_char > 'y')
+             ? JSRegExp::kInvalid
+             : kCharFlagValues[flag_char - 'g'];
+}
+
+JSRegExp::Flags RegExpFlagsFromString(Isolate* isolate, Handle<String> flags,
+                                      bool* success) {
+  STATIC_ASSERT(CharToFlag('g') == JSRegExp::kGlobal);
+  STATIC_ASSERT(CharToFlag('i') == JSRegExp::kIgnoreCase);
+  STATIC_ASSERT(CharToFlag('m') == JSRegExp::kMultiline);
+  STATIC_ASSERT(CharToFlag('s') == JSRegExp::kDotAll);
+  STATIC_ASSERT(CharToFlag('u') == JSRegExp::kUnicode);
+  STATIC_ASSERT(CharToFlag('y') == JSRegExp::kSticky);
+
   int length = flags->length();
+  if (length == 0) {
+    *success = true;
+    return JSRegExp::kNone;
+  }
   // A longer flags string cannot be valid.
   if (length > JSRegExp::FlagCount()) return JSRegExp::Flags(0);
-  for (int i = 0; i < length; i++) {
-    JSRegExp::Flag flag = JSRegExp::kNone;
-    switch (flags->Get(i)) {
-      case 'g':
-        flag = JSRegExp::kGlobal;
-        break;
-      case 'i':
-        flag = JSRegExp::kIgnoreCase;
-        break;
-      case 'm':
-        flag = JSRegExp::kMultiline;
-        break;
-      case 's':
-        flag = JSRegExp::kDotAll;
-        break;
-      case 'u':
-        flag = JSRegExp::kUnicode;
-        break;
-      case 'y':
-        flag = JSRegExp::kSticky;
-        break;
-      default:
-        return JSRegExp::Flags(0);
+  // Initialize {value} to {kInvalid} to allow 2-in-1 duplicate/invalid check.
+  JSRegExp::Flags value = JSRegExp::kInvalid;
+  if (flags->IsSeqOneByteString()) {
+    DisallowHeapAllocation no_gc;
+    SeqOneByteString seq_flags = SeqOneByteString::cast(*flags);
+    for (int i = 0; i < length; i++) {
+      JSRegExp::Flag flag = CharToFlag(seq_flags.SeqOneByteStringGet(i));
+      // Duplicate or invalid flag.
+      if (value & flag) return JSRegExp::Flags(0);
+      value |= flag;
+    }
+  } else {
+    flags = String::Flatten(isolate, flags);
+    DisallowHeapAllocation no_gc;
+    String::FlatContent flags_content = flags->GetFlatContent(no_gc);
+    for (int i = 0; i < length; i++) {
+      JSRegExp::Flag flag = CharToFlag(flags_content.Get(i));
+      // Duplicate or invalid flag.
+      if (value & flag) return JSRegExp::Flags(0);
+      value |= flag;
     }
-    // Duplicate flag.
-    if (value & flag) return JSRegExp::Flags(0);
-    value |= flag;
   }
   *success = true;
+  // Drop the initially set {kInvalid} bit.
+  value ^= JSRegExp::kInvalid;
   return value;
 }
 
@@ -16757,7 +16792,7 @@ MaybeHandle<JSRegExp> JSRegExp::Initialize(Handle<JSRegExp> regexp,
                                            Handle<String> flags_string) {
   Isolate* isolate = regexp->GetIsolate();
   bool success = false;
-  Flags flags = RegExpFlagsFromString(flags_string, &success);
+  Flags flags = RegExpFlagsFromString(isolate, flags_string, &success);
   if (!success) {
     THROW_NEW_ERROR(
         isolate,

src/objects/js-regexp.h

@@ -36,40 +36,43 @@ class JSRegExp : public JSObject {
   // ATOM: A simple string to match against using an indexOf operation.
   // IRREGEXP: Compiled with Irregexp.
   enum Type { NOT_COMPILED, ATOM, IRREGEXP };
-  enum Flag {
+  struct FlagShiftBit {
+    static const int kGlobal = 0;
+    static const int kIgnoreCase = 1;
+    static const int kMultiline = 2;
+    static const int kSticky = 3;
+    static const int kUnicode = 4;
+    static const int kDotAll = 5;
+    static const int kInvalid = 7;
+  };
+  enum Flag : uint8_t {
     kNone = 0,
-    kGlobal = 1 << 0,
-    kIgnoreCase = 1 << 1,
-    kMultiline = 1 << 2,
-    kSticky = 1 << 3,
-    kUnicode = 1 << 4,
-    kDotAll = 1 << 5,
+    kGlobal = 1 << FlagShiftBit::kGlobal,
+    kIgnoreCase = 1 << FlagShiftBit::kIgnoreCase,
+    kMultiline = 1 << FlagShiftBit::kMultiline,
+    kSticky = 1 << FlagShiftBit::kSticky,
+    kUnicode = 1 << FlagShiftBit::kUnicode,
+    kDotAll = 1 << FlagShiftBit::kDotAll,
     // Update FlagCount when adding new flags.
+    kInvalid = 1 << FlagShiftBit::kInvalid,  // Not included in FlagCount.
   };
   typedef base::Flags<Flag> Flags;
-
   static constexpr int FlagCount() { return 6; }
 
   static int FlagShiftBits(Flag flag) {
     switch (flag) {
       case kGlobal:
-        STATIC_ASSERT(kGlobal == (1 << 0));
-        return 0;
+        return FlagShiftBit::kGlobal;
       case kIgnoreCase:
-        STATIC_ASSERT(kIgnoreCase == (1 << 1));
-        return 1;
+        return FlagShiftBit::kIgnoreCase;
       case kMultiline:
-        STATIC_ASSERT(kMultiline == (1 << 2));
-        return 2;
+        return FlagShiftBit::kMultiline;
       case kSticky:
-        STATIC_ASSERT(kSticky == (1 << 3));
-        return 3;
+        return FlagShiftBit::kSticky;
       case kUnicode:
-        STATIC_ASSERT(kUnicode == (1 << 4));
-        return 4;
+        return FlagShiftBit::kUnicode;
       case kDotAll:
-        STATIC_ASSERT(kDotAll == (1 << 5));
-        return 5;
+        return FlagShiftBit::kDotAll;
       default:
         STATIC_ASSERT(FlagCount() == 6);
         UNREACHABLE();

test/js-perf-test/RegExp/base.js

@@ -5,6 +5,7 @@
 function benchName(bench, setup) {
   var name = bench.name;
   if (setup) name += "/" + setup.name;
+  return name;
 }
 
 function slowBenchName(bench, setup) {
@@ -30,7 +31,7 @@ function createBenchmarkSuite(name) {
   return new BenchmarkSuite(
     name, [1000],
     benchmarks.map(([bench, setup]) =>
-        new Benchmark(benchName(bench, setup), false, false, 0, bench,
+        new Benchmark(benchName(bench, setup), false, false, 100000, bench,
                       setup)));
 }