[esnext] only load .next() once for JSAsyncFromSyncIterator

A version of the spec change from tc39/ecma262#988, but applied to the Async-from-Sync iterator type. This change does not modify generated bytecode (but maybe it should to take advantage of load IC feedback for loading "next"). Doing this grows bytecode by quite a bit, since it's necessary to throw-if-not-an-object before loading "next" (which currently gets to live in a code stub instead). BUG=v8:5855 Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng Change-Id: I0d2affef664d1069b24c54a553d62e17b49e5a16 Reviewed-on: https://chromium-review.googlesource.com/723136 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Ross McIlroy <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#51078}
paul99 · Feb 2, 2018 · 6b1586e · 6b1586e
1 parent 9bffe96
commit 6b1586e
Show file tree

Hide file tree

Showing 9 changed files with 49 additions and 22 deletions.
diff --git a/src/builtins/builtins-async-iterator-gen.cc b/src/builtins/builtins-async-iterator-gen.cc
@@ -28,13 +28,29 @@ class AsyncFromSyncBuiltinsAssembler : public AsyncBuiltinsAssembler {
   typedef std::function<void(Node* const context, Node* const promise,
                              Label* if_exception)>
       UndefinedMethodHandler;
+  typedef std::function<Node*(Node*)> SyncIteratorNodeGenerator;
   void Generate_AsyncFromSyncIteratorMethod(
       Node* const context, Node* const iterator, Node* const sent_value,
-      Handle<Name> method_name, UndefinedMethodHandler&& if_method_undefined,
+      const SyncIteratorNodeGenerator& get_method,
+      const UndefinedMethodHandler& if_method_undefined,
       const char* operation_name,
       Label::Type reject_label_type = Label::kDeferred,
       Node* const initial_exception_value = nullptr);
 
+  void Generate_AsyncFromSyncIteratorMethod(
+      Node* const context, Node* const iterator, Node* const sent_value,
+      Handle<String> name, const UndefinedMethodHandler& if_method_undefined,
+      const char* operation_name,
+      Label::Type reject_label_type = Label::kDeferred,
+      Node* const initial_exception_value = nullptr) {
+    auto get_method = [=](Node* const sync_iterator) {
+      return GetProperty(context, sync_iterator, name);
+    };
+    return Generate_AsyncFromSyncIteratorMethod(
+        context, iterator, sent_value, get_method, if_method_undefined,
+        operation_name, reject_label_type, initial_exception_value);
+  }
+
   // Load "value" and "done" from an iterator result object. If an exception
   // is thrown at any point, jumps to te `if_exception` label with exception
   // stored in `var_exception`.
@@ -79,7 +95,8 @@ void AsyncFromSyncBuiltinsAssembler::ThrowIfNotAsyncFromSyncIterator(
 
 void AsyncFromSyncBuiltinsAssembler::Generate_AsyncFromSyncIteratorMethod(
     Node* const context, Node* const iterator, Node* const sent_value,
-    Handle<Name> method_name, UndefinedMethodHandler&& if_method_undefined,
+    const SyncIteratorNodeGenerator& get_method,
+    const UndefinedMethodHandler& if_method_undefined,
     const char* operation_name, Label::Type reject_label_type,
     Node* const initial_exception_value) {
   Node* const native_context = LoadNativeContext(context);
@@ -96,7 +113,7 @@ void AsyncFromSyncBuiltinsAssembler::Generate_AsyncFromSyncIteratorMethod(
   Node* const sync_iterator =
       LoadObjectField(iterator, JSAsyncFromSyncIterator::kSyncIteratorOffset);
 
-  Node* const method = GetProperty(context, sync_iterator, method_name);
+  Node* const method = get_method(sync_iterator);
 
   if (if_method_undefined) {
     Label if_isnotundefined(this);
@@ -220,9 +237,12 @@ TF_BUILTIN(AsyncFromSyncIteratorPrototypeNext, AsyncFromSyncBuiltinsAssembler) {
   Node* const value = Parameter(Descriptor::kValue);
   Node* const context = Parameter(Descriptor::kContext);
 
+  auto get_method = [=](Node* const unused) {
+    return LoadObjectField(iterator, JSAsyncFromSyncIterator::kNextOffset);
+  };
   Generate_AsyncFromSyncIteratorMethod(
-      context, iterator, value, factory()->next_string(),
-      UndefinedMethodHandler(), "[Async-from-Sync Iterator].prototype.next");
+      context, iterator, value, get_method, UndefinedMethodHandler(),
+      "[Async-from-Sync Iterator].prototype.next");
 }
 
 // https://tc39.github.io/proposal-async-iteration/

diff --git a/src/factory.cc b/src/factory.cc
@@ -2097,12 +2097,13 @@ Handle<JSIteratorResult> Factory::NewJSIteratorResult(Handle<Object> value,
 }
 
 Handle<JSAsyncFromSyncIterator> Factory::NewJSAsyncFromSyncIterator(
-    Handle<JSReceiver> sync_iterator) {
+    Handle<JSReceiver> sync_iterator, Handle<Object> next) {
   Handle<Map> map(isolate()->native_context()->async_from_sync_iterator_map());
   Handle<JSAsyncFromSyncIterator> iterator =
       Handle<JSAsyncFromSyncIterator>::cast(NewJSObjectFromMap(map));
 
   iterator->set_sync_iterator(*sync_iterator);
+  iterator->set_next(*next);
   return iterator;
 }
 

diff --git a/src/factory.h b/src/factory.h
@@ -600,7 +600,7 @@ class V8_EXPORT_PRIVATE Factory final {
 
   Handle<JSIteratorResult> NewJSIteratorResult(Handle<Object> value, bool done);
   Handle<JSAsyncFromSyncIterator> NewJSAsyncFromSyncIterator(
-      Handle<JSReceiver> sync_iterator);
+      Handle<JSReceiver> sync_iterator, Handle<Object> next);
 
   Handle<JSMap> NewJSMap();
   Handle<JSSet> NewJSSet();

diff --git a/src/interpreter/interpreter-intrinsics-generator.cc b/src/interpreter/interpreter-intrinsics-generator.cc
@@ -7,6 +7,7 @@
 #include "src/allocation.h"
 #include "src/builtins/builtins.h"
 #include "src/code-factory.h"
+#include "src/factory-inl.h"
 #include "src/frames.h"
 #include "src/interpreter/bytecodes.h"
 #include "src/interpreter/interpreter-assembler.h"
@@ -52,6 +53,7 @@ class IntrinsicsGenerator {
 
   Isolate* isolate() { return isolate_; }
   Zone* zone() { return zone_; }
+  Factory* factory() { return isolate()->factory(); }
 
   Isolate* isolate_;
   Zone* zone_;
@@ -368,13 +370,18 @@ Node* IntrinsicsGenerator::CreateAsyncFromSyncIterator(
   __ GotoIf(__ TaggedIsSmi(sync_iterator), &not_receiver);
   __ GotoIfNot(__ IsJSReceiver(sync_iterator), &not_receiver);
 
+  Node* const next =
+      __ GetProperty(context, sync_iterator, factory()->next_string());
+
   Node* const native_context = __ LoadNativeContext(context);
   Node* const map = __ LoadContextElement(
       native_context, Context::ASYNC_FROM_SYNC_ITERATOR_MAP_INDEX);
   Node* const iterator = __ AllocateJSObjectFromMap(map);
 
   __ StoreObjectFieldNoWriteBarrier(
       iterator, JSAsyncFromSyncIterator::kSyncIteratorOffset, sync_iterator);
+  __ StoreObjectFieldNoWriteBarrier(iterator,
+                                    JSAsyncFromSyncIterator::kNextOffset, next);
 
   return_value.Bind(iterator);
   __ Goto(&done);

diff --git a/src/objects-inl.h b/src/objects-inl.h
@@ -3520,6 +3520,7 @@ ACCESSORS(JSIteratorResult, done, Object, kDoneOffset)
 
 ACCESSORS(JSAsyncFromSyncIterator, sync_iterator, JSReceiver,
           kSyncIteratorOffset)
+ACCESSORS(JSAsyncFromSyncIterator, next, Object, kNextOffset)
 
 ACCESSORS(JSStringIterator, string, String, kStringOffset)
 SMI_ACCESSORS(JSStringIterator, index, kNextIndexOffset)

diff --git a/src/objects.h b/src/objects.h
@@ -4602,9 +4602,14 @@ class JSAsyncFromSyncIterator : public JSObject {
   // (proposal-async-iteration/#table-async-from-sync-iterator-internal-slots)
   DECL_ACCESSORS(sync_iterator, JSReceiver)
 
+  // The "next" method is loaded during GetIterator, and is not reloaded for
+  // subsequent "next" invocations.
+  DECL_ACCESSORS(next, Object)
+
   // Offsets of object fields.
   static const int kSyncIteratorOffset = JSObject::kHeaderSize;
-  static const int kSize = kSyncIteratorOffset + kPointerSize;
+  static const int kNextOffset = kSyncIteratorOffset + kPointerSize;
+  static const int kSize = kNextOffset + kPointerSize;
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSAsyncFromSyncIterator);

diff --git a/src/runtime/runtime-internal.cc b/src/runtime/runtime-internal.cc
@@ -634,8 +634,13 @@ RUNTIME_FUNCTION(Runtime_CreateAsyncFromSyncIterator) {
         isolate, NewTypeError(MessageTemplate::kSymbolIteratorInvalid));
   }
 
+  Handle<Object> next;
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, next,
+      Object::GetProperty(sync_iterator, isolate->factory()->next_string()));
+
   return *isolate->factory()->NewJSAsyncFromSyncIterator(
-      Handle<JSReceiver>::cast(sync_iterator));
+      Handle<JSReceiver>::cast(sync_iterator), next);
 }
 
 RUNTIME_FUNCTION(Runtime_GetTemplateObject) {

diff --git a/test/mjsunit/harmony/async-from-sync-iterator.js b/test/mjsunit/harmony/async-from-sync-iterator.js
@@ -717,8 +717,8 @@ if (testFailed) {
     next_: 0,
     get next() {
       log.push("get syncIterable.next");
-      let i = this.next_++;
       return (...args) => {
+        let i = this.next_++;
         log.push("call syncIterable.next(" + args.join(", ") + ")");
         return results[i];
       }
@@ -748,14 +748,12 @@ if (testFailed) {
       "get nextValue#1.then",
       "call nextValue#1.then",
       "got value value1",
-      "get syncIterable.next",
       "call syncIterable.next()",
       "get iterResult #2.done",
       "get iterResult #2.value",
       "get nextValue#2.then",
       "call nextValue#2.then",
       "got value value2",
-      "get syncIterable.next",
       "call syncIterable.next()",
       "get iterResult #3.done",
       "get iterResult #3.value",

diff --git a/test/test262/test262.status b/test/test262/test262.status
@@ -419,16 +419,6 @@
   'built-ins/Proxy/ownKeys/return-duplicate-entries-throws': [FAIL],
   'built-ins/Proxy/ownKeys/return-duplicate-symbol-entries-throws': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=6861
-  'language/expressions/object/method-definition/async-gen-yield-star-sync-next': [FAIL],
-  'language/expressions/class/async-gen-method-static-yield-star-sync-next': [FAIL],
-  'language/expressions/async-generator/yield-star-sync-next': [FAIL],
-  'language/statements/class/async-gen-method-static-yield-star-sync-next': [FAIL],
-  'language/expressions/async-generator/named-yield-star-sync-next': [FAIL],
-  'language/expressions/class/async-gen-method-yield-star-sync-next': [FAIL],
-  'language/statements/class/async-gen-method-yield-star-sync-next': [FAIL],
-  'language/statements/async-generator/yield-star-sync-next': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=6791
   'built-ins/BigInt/prototype/Symbol.toStringTag': [SKIP],
   'built-ins/DataView/prototype/getBigInt64/*': [SKIP],