[regexp] Move RegExp.prototype[@@search] to TF

src/builtins/builtins-regexp.cc

Index: src/builtins/builtins-regexp.cc
diff --git a/src/builtins/builtins-regexp.cc b/src/builtins/builtins-regexp.cc
index d993e82f2eae7ee0a2545f712ae43f22b0810db6..57ff578f51559feae7de67a8d8ab1e31d0332b7d 100644
--- a/src/builtins/builtins-regexp.cc
+++ b/src/builtins/builtins-regexp.cc
@@ -1144,57 +1144,106 @@ BUILTIN(RegExpPrototypeMatch) {
 
 // ES#sec-regexp.prototype-@@search
 // RegExp.prototype [ @@search ] ( string )
-BUILTIN(RegExpPrototypeSearch) {
-  HandleScope scope(isolate);
-  CHECK_RECEIVER(JSReceiver, recv, "RegExp.prototype.@@search");
+void Builtins::Generate_RegExpPrototypeSearch(CodeStubAssembler* a) {
+  typedef CodeStubAssembler::Label Label;
+  typedef compiler::Node Node;
 
-  Handle<Object> string_obj = args.atOrUndefined(isolate, 1);
+  Isolate* const isolate = a->isolate();
 
-  Handle<String> string;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, string,
-                                     Object::ToString(isolate, string_obj));
+  Node* const maybe_receiver = a->Parameter(0);
+  Node* const maybe_string = a->Parameter(1);
+  Node* const context = a->Parameter(4);
+
+  Node* const smi_zero = a->SmiConstant(Smi::kZero);
+
+  // TODO(jgruber): If we need to optimize this further, we could add an entire
+  // separate fast-path, skipping all follow-up map checks since we can
+  // guarantee they won't change. The downside is more code to maintain.
+
+  // Ensure {maybe_receiver} is a JSReceiver.
+  Node* const map =
+      ThrowIfNotJSReceiver(a, isolate, context, maybe_receiver,
+                           MessageTemplate::kIncompatibleMethodReceiver,
+                           "RegExp.prototype.@@search");
+  Node* const receiver = maybe_receiver;
+
+  // Convert {maybe_string} to a String.
+  Node* const string = a->ToString(context, maybe_string);
+
+  // Grab the initial value of last index.
+  Node* has_initialmap = IsInitialRegExpMap(a, context, map);
+  Node* const previous_last_index =
+      LoadLastIndex(a, context, has_initialmap, receiver);
+
+  // Ensure last index is 0.
+  {
+    Label next(a);
+    a->GotoIf(a->WordEqual(previous_last_index, smi_zero), &next);
 
-  Handle<Object> previous_last_index_obj;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, previous_last_index_obj,
-                                     RegExpUtils::GetLastIndex(isolate, recv));
+    StoreLastIndex(a, context, has_initialmap, receiver, smi_zero);

[Yang, 2016/10/25 07:30:12]

The spec actually suggests to always set the last index to 0, regardless of what
the previous value was. This is an observable difference.

Also, below we check for IsInitialRegExpMap because it might have changed. This
could happen here too, if the regexp does not have the initial map and
LoadLastIndex triggered side effects to change the map.

I suggest having one fast path with has_initial_map, because then we can be
certain that nothing observable can happen. We would not need to recheck
IsInitialRegExpMap along this fast path. And we would not need LoadLastIndex,
since it's going to be overwritten and not-observable anyways, then always
StoreLastIndex to 0.

The slow path would not need to do the IsInitialRegExpMap along the way either,
since we would just know that it would be false.

[jgruber, 2016/10/25 08:40:57]

The spec has changed fairly recently to avoid unnecessary writes to lastIndex:
https://github.com/tc39/ecma262/pull/627

This CL implements those semantics.

Agreed on the separate fast/slow path implementations (also had this as a TODO
above). Good catch on the missing map checks. Will go with that solution for the
next patch set.

+    a->Goto(&next);
 
-  if (!previous_last_index_obj->IsSmi() ||
-      Smi::cast(*previous_last_index_obj)->value() != 0) {
-    RETURN_FAILURE_ON_EXCEPTION(isolate,
-                                RegExpUtils::SetLastIndex(isolate, recv, 0));
+    a->Bind(&next);
   }
 
-  Handle<Object> result;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-      isolate, result,
-      RegExpUtils::RegExpExec(isolate, recv, string,
-                              isolate->factory()->undefined_value()));
-
-  Handle<Object> current_last_index_obj;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, current_last_index_obj,
-                                     RegExpUtils::GetLastIndex(isolate, recv));
-
-  Maybe<bool> is_last_index_unchanged =
-      Object::Equals(current_last_index_obj, previous_last_index_obj);
-  if (is_last_index_unchanged.IsNothing()) return isolate->pending_exception();

[Yang, 2016/10/25 07:30:13]

This does not seem correct. To throw an exception we do not return the pending
exception object, but the isolate->heap()->exception() sentinel. I know this is
being removed, but having this suggests that we don't have a test case for this.

[jgruber, 2016/10/25 08:40:57]

Great catch. Looking at this further, we actually need to do more work here -
the spec uses SameValue(x, y) so a simple identity comparison is not enough
unfortunately. Will also add a test case.

-  if (!is_last_index_unchanged.FromJust()) {
-    if (previous_last_index_obj->IsSmi()) {
-      RETURN_FAILURE_ON_EXCEPTION(
-          isolate,
-          RegExpUtils::SetLastIndex(
-              isolate, recv, Smi::cast(*previous_last_index_obj)->value()));
-    } else {
-      RETURN_FAILURE_ON_EXCEPTION(
-          isolate,
-          Object::SetProperty(recv, isolate->factory()->lastIndex_string(),
-                              previous_last_index_obj, STRICT));
-    }
+  // Call exec.
+  Node* const match_indices = RegExpExec(a, context, receiver, string);
+
+  // Reset last index if necessary.
+  {
+    Label next(a);
+
+    // Recheck the map since it might have changed.
+    has_initialmap = IsInitialRegExpMap(a, context, map);
+
+    Node* const current_last_index =
+        LoadLastIndex(a, context, has_initialmap, receiver);

[Yang, 2016/10/25 07:30:12]

Again, the spec suggests to always overwrite the last index, and not load at
all.

[jgruber, 2016/10/25 08:40:57]

The spec has changed recently (see above).

+
+    a->GotoIf(a->WordEqual(current_last_index, previous_last_index), &next);
+
+    StoreLastIndex(a, context, has_initialmap, receiver, previous_last_index);
+    a->Goto(&next);
+
+    a->Bind(&next);
   }
 
-  if (result->IsNull(isolate)) return Smi::FromInt(-1);
+  // Return -1 if no match was found.
+  {
+    Label next(a);
+    a->GotoUnless(a->WordEqual(match_indices, a->NullConstant()), &next);
+    a->Return(a->SmiConstant(Smi::FromInt(-1)));
+    a->Bind(&next);
+  }
+
+  // Return the index of the match.
+  {
+    Label fast_path(a), slow_path(a, Label::kDeferred);
+
+    Node* const native_context = a->LoadNativeContext(context);
+    Node* const initial_regexp_result_map =
+        a->LoadContextElement(native_context, Context::REGEXP_RESULT_MAP_INDEX);
+    Node* const match_indices_map = a->LoadMap(match_indices);
+
+    a->Branch(a->WordEqual(match_indices_map, initial_regexp_result_map),
+              &fast_path, &slow_path);
 
-  RETURN_RESULT_OR_FAILURE(
-      isolate, Object::GetProperty(result, isolate->factory()->index_string()));
+    a->Bind(&fast_path);
+    {
+      Node* const index =
+          a->LoadObjectField(match_indices, JSRegExpResult::kIndexOffset,
+                             MachineType::AnyTagged());
+      a->Return(index);
+    }
+
+    a->Bind(&slow_path);
+    {
+      Node* const name = a->HeapConstant(isolate->factory()->index_string());
+      Callable getproperty_callable = CodeFactory::GetProperty(a->isolate());
+      Node* const index =
+          a->CallStub(getproperty_callable, context, match_indices, name);
+      a->Return(index);
+    }
+  }
 }
 
 namespace {