[promises] Move CreateResolvingFunctions to c++

src/builtins/builtins-promise.cc

Index: src/builtins/builtins-promise.cc
diff --git a/src/builtins/builtins-promise.cc b/src/builtins/builtins-promise.cc
new file mode 100644
index 0000000000000000000000000000000000000000..50c04112be493e7d8024d3d48c1bb0c572559a0e
--- /dev/null
+++ b/src/builtins/builtins-promise.cc
@@ -0,0 +1,105 @@
+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#include <iostream>
+#include "src/builtins/builtins-utils.h"
+#include "src/builtins/builtins.h"
+
+namespace v8 {
+namespace internal {
+
+BUILTIN(PromiseResolveClosure) {
+  HandleScope scope(isolate);
+
+  Handle<Object> value = args.atOrUndefined(isolate, 1);
+  Context* context = isolate->context();

[adamk, 2016/11/08 18:09:55]

This should be stored into a Handle<Context> immediately.

[gsathya, 2016/11/08 20:03:18]

Done.

+  Handle<Cell> already_visited(Cell::cast(context->get(4)), isolate);

[adamk, 2016/11/08 18:09:55]

These magic numbers have to be defined as constants somewhere (maybe just in
this file, if this is the only place they're accessed?). Same goes for all other
such magic numbers in this CL.

[gsathya, 2016/11/08 20:03:18]

Done.

+
+  if (Smi::cast(already_visited->value())->value() != 0) {
+    return isolate->heap()->undefined_value();
+  }
+
+  already_visited->set_value(Smi::FromInt(1));
+  Handle<JSObject> promise(JSObject::cast(context->get(5)), isolate);
+
+  MaybeHandle<Object> maybe_result;
+  Handle<Object> argv[] = {promise, value};
+  maybe_result = Execution::Call(isolate, isolate->promise_resolve(),
+                                 isolate->factory()->undefined_value(),
+                                 arraysize(argv), argv);
+  return isolate->heap()->undefined_value();
+}
+
+BUILTIN(PromiseRejectClosure) {
+  HandleScope scope(isolate);
+
+  Handle<Object> value = args.atOrUndefined(isolate, 1);
+  Context* context = isolate->context();
+  Handle<Cell> already_visited(Cell::cast(context->get(4)), isolate);
+
+  if (Smi::cast(already_visited->value())->value() != 0) {
+    return isolate->heap()->undefined_value();
+  }
+
+  already_visited->set_value(Smi::FromInt(1));
+  Handle<JSObject> promise(JSObject::cast(context->get(5)), isolate);
+  Handle<Object> debug_event(context->get(6), isolate);
+  MaybeHandle<Object> maybe_result;
+  Handle<Object> argv[] = {promise, value, debug_event};
+  maybe_result = Execution::Call(isolate, isolate->promise_internal_reject(),
+                                 isolate->factory()->undefined_value(),
+                                 arraysize(argv), argv);
+  return isolate->heap()->undefined_value();
+}
+
+namespace {
+Handle<JSFunction> CreateClosure(Isolate* isolate,
+                                 Handle<SharedFunctionInfo> info,
+                                 Handle<Cell> already_resolved,
+                                 Handle<JSObject> promise,
+                                 Handle<Object> debug_event) {
+  Handle<JSFunction> fun =
+      isolate->factory()->NewFunctionFromSharedFunctionInfo(
+          isolate->sloppy_function_without_prototype_map(), info,
+          isolate->native_context(), TENURED);
+
+  Handle<Context> context =
+      isolate->factory()->NewPromiseResolvingFunctionContext(

[adamk, 2016/11/08 18:09:55]

Couldn't you share this context between the two functions? That'd also let you
get rid of the Cell indirection.

[gsathya, 2016/11/08 20:03:18]

Done. But now this context is completely busted (lacking closure and previous
slots).

+          fun, already_resolved, promise, debug_event);
+  fun->set_context(*context);
+  return fun;
+}
+}  // namespace
+
+BUILTIN(CreateResolvingFunctions) {
+  HandleScope scope(isolate);
+
+  Handle<Object> promise_obj = args.atOrUndefined(isolate, 1);
+  Handle<Object> debug_event = args.atOrUndefined(isolate, 2);

[adamk, 2016/11/08 18:09:55]

atOrUndefined seems wrong here, as this is only called internally so we know how
many arguments were passed. I think it'd be better to have

DCHECK_EQ(3, args.length());
Handle<Promise> promise = args.at<Promise>(1);
...

[gsathya, 2016/11/08 20:03:19]

Nice! TIL args.at

+
+  Handle<JSObject> promise = Handle<JSObject>::cast(promise_obj);
+  Handle<Cell> already_resolved =
+      isolate->factory()->NewCell(handle(Smi::kZero, isolate));
+  Handle<SharedFunctionInfo> resolve_shared_fun(
+      isolate->native_context()->promise_resolve_shared_fun(), isolate);
+  Handle<SharedFunctionInfo> reject_shared_fun(
+      isolate->native_context()->promise_reject_shared_fun(), isolate);
+
+  Handle<JSFunction> resolve = CreateClosure(
+      isolate, resolve_shared_fun, already_resolved, promise, debug_event);
+  Handle<JSFunction> reject = CreateClosure(
+      isolate, reject_shared_fun, already_resolved, promise, debug_event);
+
+  Handle<JSObject> result =
+      isolate->factory()->NewJSObject(isolate->object_function(), NOT_TENURED);

[adamk, 2016/11/08 18:09:55]

What might be even faster would be to change this calling convention to return
an array [resolve, reject]. You'd create a FixedArray of length 2 for the
backing store, then call Factory::NewJSArrayWIthElements().

[gsathya, 2016/11/08 20:03:18]

Done.

+  JSObject::AddProperty(result, isolate->factory()->resolve_string(), resolve,
+                        NONE);
+
+  JSObject::AddProperty(result, isolate->factory()->reject_string(), reject,
+                        NONE);
+
+  return *result;
+}
+
+}  // namespace internal
+}  // namespace v8