Changeset 223894 in webkit

Timestamp:

Oct 24, 2017 10:11:39 AM (6 years ago)

Author:

Yusuke Suzuki

Message:

[JSC] modules can be visited more than once when resolving bindings through "star" exports as long as the exportName is different each time
​https://bugs.webkit.org/show_bug.cgi?id=178308

Reviewed by Mark Lam.

JSTests:

• test262.yaml:

Source/JavaScriptCore:

With the change of the spec[1], we now do not need to remember star resolution modules.
We reflect this change to our implementation. Since this change is covered by test262,
this patch improves the score of test262.

We also add logging to ResolveExport to debug it easily.

[1]: ​https://github.com/tc39/ecma262/commit/a865e778ff0fc60e26e3e1c589635103710766a1

• runtime/AbstractModuleRecord.cpp:

(JSC::AbstractModuleRecord::ResolveQuery::dump const):
(JSC::AbstractModuleRecord::resolveExportImpl):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/test262.yaml (modified) (3 diffs)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp (modified) (10 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2017-10-15  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] modules can be visited more than once when resolving bindings through "star" exports as long as the exportName is different each time
+        https://bugs.webkit.org/show_bug.cgi?id=178308
+
+        Reviewed by Mark Lam.
+
+        * test262.yaml:
+
 2017-10-23  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/JSTests/test262.yaml

@@ -85791 +85791 @@
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-iee-star-cycle.js
-  cmd: runTest262 :fail, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
+  cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-iee-trlng-comma.js
   cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
@@ -85893 +85893 @@
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-named-star-cycle.js
-  cmd: runTest262 :fail, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
+  cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-once.js
   cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
@@ -85999 +85999 @@
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-star-star-cycle.js
-  cmd: runTest262 :fail, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
+  cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-uniq-env-rec-other_FIXTURE.js
   cmd: prepareTest262Fixture

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-10-15  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] modules can be visited more than once when resolving bindings through "star" exports as long as the exportName is different each time
+        https://bugs.webkit.org/show_bug.cgi?id=178308
+
+        Reviewed by Mark Lam.
+
+        With the change of the spec[1], we now do not need to remember star resolution modules.
+        We reflect this change to our implementation. Since this change is covered by test262,
+        this patch improves the score of test262.
+
+        We also add logging to ResolveExport to debug it easily.
+
+        [1]: https://github.com/tc39/ecma262/commit/a865e778ff0fc60e26e3e1c589635103710766a1
+
+        * runtime/AbstractModuleRecord.cpp:
+        (JSC::AbstractModuleRecord::ResolveQuery::dump const):
+        (JSC::AbstractModuleRecord::resolveExportImpl):
+
 2017-10-24  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp

@@ -36 +36 @@
 
 namespace JSC {
+namespace AbstractModuleRecordInternal {
+static const bool verbose = false;
+} // namespace AbstractModuleRecordInternal
 
 const ClassInfo AbstractModuleRecord::s_info = { "AbstractModuleRecord", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(AbstractModuleRecord) };
@@ -178 +181 @@
         static const bool safeToCompareToEmptyOrDeleted = true;
     };
+    using HashTraits = WTF::CustomHashTraits<ResolveQuery>;
 
     ResolveQuery(AbstractModuleRecord* moduleRecord, UniquedStringImpl* exportName)
@@ -212 +216 @@
     }
 
+    void dump(PrintStream& out) const
+    {
+        if (!moduleRecord) {
+            out.print("<empty>");
+            return;
+        }
+        out.print(moduleRecord->moduleKey(), " \"", exportName.get(), "\"");
+    }
+
     // The module record is not marked from the GC. But these records are reachable from the JSGlobalObject.
     // So we don't care the reachability to this record.
@@ -246 +259 @@
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    // http://www.ecma-international.org/ecma-262/6.0/#sec-resolveexport
+    if (AbstractModuleRecordInternal::verbose)
+        dataLog("Resolving ", root, "\n");
+
+    // https://tc39.github.io/ecma262/#sec-resolveexport
 
     // How to avoid C++ recursion in this function:
@@ -481 +497 @@
     //  5. Once we see star links, even if we have not yet traversed that star link path, we should disable caching.
 
-    typedef WTF::HashSet<ResolveQuery, ResolveQuery::Hash, WTF::CustomHashTraits<ResolveQuery>> ResolveSet;
+    using ResolveSet = WTF::HashSet<ResolveQuery, ResolveQuery::Hash, ResolveQuery::HashTraits>;
     enum class Type { Query, IndirectFallback, GatherStars };
     struct Task {
@@ -488 +504 @@
     };
 
+    auto typeString = [] (Type type) -> const char* {
+        switch (type) {
+        case Type::Query:
+            return "Query";
+        case Type::IndirectFallback:
+            return "IndirectFallback";
+        case Type::GatherStars:
+            return "GatherStars";
+        }
+        RELEASE_ASSERT_NOT_REACHED();
+        return nullptr;
+    };
+
     Vector<Task, 8> pendingTasks;
     ResolveSet resolveSet;
-    HashSet<AbstractModuleRecord*> starSet;
 
     Vector<Resolution, 8> frames;
@@ -501 +529 @@
     // It will enqueue the star resolution requests. Return "false" if the error occurs.
     auto resolveNonLocal = [&](const ResolveQuery& query) -> bool {
-        // http://www.ecma-international.org/ecma-262/6.0/#sec-resolveexport
+        // https://tc39.github.io/ecma262/#sec-resolveexport
         // section 15.2.1.16.3, step 6
         // If the "default" name is not resolved in the current module, we need to throw an error and stop resolution immediately,
@@ -510 +538 @@
             return false;
 
-        // step 7, If exportStarSet contains module, then return null.
-        if (!starSet.add(query.moduleRecord).isNewEntry)
-            return true;
-
         // Enqueue the task to gather the results of the stars.
         // And append the new Resolution frame to gather the local result of the stars.
@@ -519 +543 @@
         foundStarLinks = true;
         frames.append(Resolution::notFound());
-
 
         // Enqueue the tasks in reverse order.
@@ -563 +586 @@
         const Task task = pendingTasks.takeLast();
         const ResolveQuery& query = task.query;
+
+        if (AbstractModuleRecordInternal::verbose)
+            dataLog("    ", typeString(task.type), " ", task.query, "\n");
 
         switch (task.type) {