- Revision
- 223894
- Author
- utatane....@gmail.com
- Date
- 2017-10-24 10:11:39 -0700 (Tue, 24 Oct 2017)
Log 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/_javascript_Core:
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):
Modified Paths
Diff
Modified: trunk/JSTests/ChangeLog (223893 => 223894)
--- trunk/JSTests/ChangeLog 2017-10-24 17:05:23 UTC (rev 223893)
+++ trunk/JSTests/ChangeLog 2017-10-24 17:11:39 UTC (rev 223894)
@@ -1,3 +1,12 @@
+2017-10-15 Yusuke Suzuki <utatane....@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....@gmail.com>
[JSC] Use fastJoin in Array#toString
Modified: trunk/JSTests/test262.yaml (223893 => 223894)
--- trunk/JSTests/test262.yaml 2017-10-24 17:05:23 UTC (rev 223893)
+++ trunk/JSTests/test262.yaml 2017-10-24 17:11:39 UTC (rev 223894)
@@ -85790,7 +85790,7 @@
- path: test262/test/language/module-code/instn-iee-star-cycle-indirect-x_FIXTURE.js
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]
- path: test262/test/language/module-code/instn-iee-trlng-comma_FIXTURE.js
@@ -85892,7 +85892,7 @@
- path: test262/test/language/module-code/instn-named-star-cycle-indirect-x_FIXTURE.js
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]
- path: test262/test/language/module-code/instn-resolve-empty-export.js
@@ -85998,7 +85998,7 @@
- path: test262/test/language/module-code/instn-star-star-cycle-indirect-x_FIXTURE.js
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
- path: test262/test/language/module-code/instn-uniq-env-rec.js
Modified: trunk/Source/_javascript_Core/ChangeLog (223893 => 223894)
--- trunk/Source/_javascript_Core/ChangeLog 2017-10-24 17:05:23 UTC (rev 223893)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-10-24 17:11:39 UTC (rev 223894)
@@ -1,3 +1,22 @@
+2017-10-15 Yusuke Suzuki <utatane....@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....@gmail.com>
[JSC] Use emitDumbVirtualCall in 32bit JIT
Modified: trunk/Source/_javascript_Core/runtime/AbstractModuleRecord.cpp (223893 => 223894)
--- trunk/Source/_javascript_Core/runtime/AbstractModuleRecord.cpp 2017-10-24 17:05:23 UTC (rev 223893)
+++ trunk/Source/_javascript_Core/runtime/AbstractModuleRecord.cpp 2017-10-24 17:11:39 UTC (rev 223894)
@@ -35,6 +35,9 @@
#include "UnlinkedModuleProgramCodeBlock.h"
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) };
@@ -177,6 +180,7 @@
static bool equal(const ResolveQuery&, const ResolveQuery&);
static const bool safeToCompareToEmptyOrDeleted = true;
};
+ using HashTraits = WTF::CustomHashTraits<ResolveQuery>;
ResolveQuery(AbstractModuleRecord* moduleRecord, UniquedStringImpl* exportName)
: moduleRecord(moduleRecord)
@@ -211,6 +215,15 @@
return exportName.isHashTableDeletedValue();
}
+ 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.
AbstractModuleRecord* moduleRecord;
@@ -245,8 +258,11 @@
VM& vm = exec->vm();
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:
// This function avoids C++ recursion of the naive ResolveExport implementation.
// Flatten the recursion to the loop with the task queue and frames.
@@ -480,7 +496,7 @@
// 4. Once we follow star links, we should not retrieve the result from the cache and should not cache the result.
// 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 {
ResolveQuery query;
@@ -487,9 +503,21 @@
Type type;
};
+ 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;
@@ -500,7 +528,7 @@
// Call when the query is not resolved in the current module.
// 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,
// Rationale to this error: A default export cannot be provided by an export *.
@@ -509,10 +537,6 @@
if (query.exportName == vm.propertyNames->defaultKeyword.impl())
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.
pendingTasks.append(Task { query, Type::GatherStars });
@@ -519,7 +543,6 @@
foundStarLinks = true;
frames.append(Resolution::notFound());
-
// Enqueue the tasks in reverse order.
for (auto iterator = query.moduleRecord->starExportEntries().rbegin(), end = query.moduleRecord->starExportEntries().rend(); iterator != end; ++iterator) {
const RefPtr<UniquedStringImpl>& starModuleName = *iterator;
@@ -563,6 +586,9 @@
const Task task = pendingTasks.takeLast();
const ResolveQuery& query = task.query;
+ if (AbstractModuleRecordInternal::verbose)
+ dataLog(" ", typeString(task.type), " ", task.query, "\n");
+
switch (task.type) {
case Type::Query: {
AbstractModuleRecord* moduleRecord = query.moduleRecord;
