Skip to content

Introducing the new state, "ResolveDependencies" #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Constellation opened this issue Aug 15, 2015 · 11 comments
Closed

Introducing the new state, "ResolveDependencies" #68

Constellation opened this issue Aug 15, 2015 · 11 comments
Assignees

Comments

@Constellation
Copy link
Member

I think that the current draft spec incurs infinite recursion when there is the circular dependencies.
The situation is the following,

There are several modules,

A: This is the entry point.

import "B"

B

import "C"

C

import "B"
  1. At that time, before linking the modules, we call RequestInstantiateAll(A).
  2. In RequestInstantiateAll(A), step 1-b-i-1, we extract the B dependency.
  3. So, step 1-b-i-3, we perform RequestInstantiateAll(B).
  4. Later, it will extract the C dependency. So, step 1-b-i-3 in RequestInstantiateAll(B), we call RequestInstantiateAll(C).
  5. Since C has the B dependency, in RequestInstantiateAll(C) step 1-b-i-3, we again call RequestInstantiateAll(B).
  6. Here, now, since once RequestInstantiate(B) is called inside RequestInstantiateAll(B), it immediately returns the promise B.[[Instantiate]]. But, Now, we stored the status of instantiation in [[Instantiate]], but we didn't store the status of collecting the dependencies.
  7. So, in RequestInstantiateAll(B), RequestInstantiate(B) returns the stored promise, but it performs the dependency resolution again in 1-a or later. So it will call RequestInstantiateAll(C) at step 1-b-i-3 again.
  8. RequestInstantiateAll(B) and RequestInstantiateAll(C) are called repeatedly.

This problem comes from that the current algorithm does not store the status of "resolving the dependencies".

To solve this, I would like to propose introducing the new state ResolveDependencies. That means, "Now, the instantiation is done, so the entry is ready to resolve the dependencies. And if there's entry.[[ResolveDependencies]] promise and the state is ResolveDependencies, we're resolving the dependency of the module just now".

The algorithm becomes the following,

CommitInstantiated(loader, key)

  1. Let instance be Instantiation(loader, optionalInstance, source).
  2. ReturnIfAbrupt(instance).
  3. // TODO: edge case: what if instance is a thenable function?
  4. Fulfill entry.[[Instantiate]] with instance.
  5. Let deps be a new empty List.
  6. If instance is a Module Record, then:
    1. Assert: instance is a Source Text Module Record.
    2. Set instance.[[RegistryEntry]] to entry.
    3. For each dep in instance.[[RequestedModules]], do:
      1. Append the record { [[key]]: dep, [[value]]: undefined } to deps.
  7. Set entry.[[Dependencies]] to deps.
  8. Set entry.[[Module]] to instance.
  9. SetStateToMax(entry, "resolveDependencies").

RequestResolveDependencies(loader, key)

  1. Let entry be EnsureRegistered(loader, key).
  2. Let linkStateValue be GetStateValue("link").
  3. If stateValue is greater than linkStateValue, return a new error promise.
  4. If entry.[[ResolveDependencies]] is not undefined, return entry.[[ResolveDependencies]].
  5. Return the result of transforming RequestInstantiate(loader, key) with a fulfillment handler that, when called with argument entry, runs the following steps:
    1. Let depLoads be a new empty List.
    2. For each pair in entry.[[Dependencies]], do:
      1. Let p be the result of transforming Resolve(loader, pair.[[key]], key) with a fulfillment handler that, when called with value depKey, runs the following steps:
        1. Let depEntry be EnsureRegistered(loader, depKey).
        2. If depEntry.[[ResolveDependencies]] is not undefined
          1. Return the result of transforming depEntry.[[Instantiate]] with a fulfillment handler that, when called with value depEntry, runs the following steps:
            1. Let dep be depEntry.[[Module]].
            2. Set pair.[[value]] to dep.
            3. Return depEntry.
        3. Return the result of transforming RequestCollect(loader, depKey) with a fulfillment handler that, when called with value depEntry, runs the following steps:
        4. Let dep be depEntry.[[Module]].
        5. Set pair.[[value]] to dep.
        6. Return depEntry.
      2. Append p to depLoads.
    3. Let p be the result of waiting for all depLoads.
    4. Return the result of transforming p with a fulfillment handler that, when called, runs the following steps:
      1. SetStateToMax(entry, "link").
      2. Return entry.
@caridy caridy added the bug label Aug 15, 2015
@caridy caridy self-assigned this Aug 15, 2015
@caridy
Copy link
Contributor

caridy commented Aug 15, 2015

hey @Constellation, happy to see you guys digging around here ;).

I will review this asap.

@Constellation
Copy link
Member Author

@caridy Thank you. We're now prototyping the loader implementation in WebKit. So I think we can help your spec. (And your spec already helps us a lot :D)

hubot pushed a commit to WebKit/WebKit-http that referenced this issue Aug 21, 2015
https://bugs.webkit.org/show_bug.cgi?id=147876

Reviewed by Saam Barati.

Source/JavaScriptCore:

This patch implements ES6 Module Loader part. The implementation is based on
the latest draft[1, 2]. The naive implementation poses several problems.
This patch attempts to solve the spec issues and proposes the fix[3, 4, 5].

We construct the JSC internal module loader based on the ES6 Promises.
The chain of the promises represents the dependency graph of the modules and
it automatically enables asynchronous module fetching.
To leverage the Promises internally, we use the InternalPromise landed in r188681.

The loader has several platform-dependent hooks. The platform can implement
these hooks to provide the functionality missing in the module loaders, like
"how to fetch the resources". The method table of the JSGlobalObject is extended
to accept these hooks from the platform.

This patch focus on the loading part. So we don't create the module environment
and don't link the modules yet.

To test the current module progress easily, we add the `-m` option to the JSC shell.
When this option is specified, we load the given script as the module. And to use
the module loading inside the JSC shell, we added the simple loader hook for fetching.
It fetches the module content from the file system.

And to use the ES6 Map in the Loader implementation, we added @get and @set methods to the Map.
But it conflicts with the existing `getPrivateName` method. Rename it to `lookUpPrivateName`.

[1]: https://whatwg.github.io/loader/
[2]: whatwg/loader@214c7a6
[3]: whatwg/loader#66
[4]: whatwg/loader#67
[5]: whatwg/loader#68
[6]: https://bugs.webkit.org/show_bug.cgi?id=148136

* CMakeLists.txt:
* DerivedSources.make:
* JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
* JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
* JavaScriptCore.xcodeproj/project.pbxproj:
* builtins/BuiltinNames.h:
(JSC::BuiltinNames::lookUpPrivateName):
(JSC::BuiltinNames::lookUpPublicName):
(JSC::BuiltinNames::getPrivateName): Deleted.
(JSC::BuiltinNames::getPublicName): Deleted.
* builtins/ModuleLoaderObject.js: Added.
(setStateToMax):
(newRegistryEntry):
(forceFulfillPromise):
(fulfillFetch):
(fulfillTranslate):
(fulfillInstantiate):
(instantiation):
(requestFetch):
(requestTranslate):
(requestInstantiate):
(requestResolveDependencies.resolveDependenciesPromise.this.requestInstantiate.then.):
(requestResolveDependencies.resolveDependenciesPromise.this.requestInstantiate.then):
(requestResolveDependencies):
(requestInstantiateAll):
(provide):
* jsc.cpp:
(stringFromUTF):
(jscSource):
(GlobalObject::moduleLoaderFetch):
(functionCheckModuleSyntax):
(dumpException):
(runWithScripts):
(printUsageStatement):
(CommandLine::parseArguments):
(jscmain):
(CommandLine::CommandLine): Deleted.
* parser/Lexer.cpp:
(JSC::Lexer<LChar>::parseIdentifier):
(JSC::Lexer<UChar>::parseIdentifier):
* parser/ModuleAnalyzer.cpp:
(JSC::ModuleAnalyzer::ModuleAnalyzer):
(JSC::ModuleAnalyzer::exportVariable):
(JSC::ModuleAnalyzer::analyze):
* parser/ModuleAnalyzer.h:
(JSC::ModuleAnalyzer::moduleRecord):
* parser/ModuleRecord.cpp:
(JSC::printableName): Deleted.
(JSC::ModuleRecord::dump): Deleted.
* parser/ModuleRecord.h:
(JSC::ModuleRecord::ImportEntry::isNamespace): Deleted.
(JSC::ModuleRecord::create): Deleted.
(JSC::ModuleRecord::appendRequestedModule): Deleted.
(JSC::ModuleRecord::addImportEntry): Deleted.
(JSC::ModuleRecord::addExportEntry): Deleted.
(JSC::ModuleRecord::addStarExportEntry): Deleted.
* parser/Nodes.h:
* parser/NodesAnalyzeModule.cpp:
(JSC::ImportDeclarationNode::analyzeModule):
(JSC::ExportAllDeclarationNode::analyzeModule):
(JSC::ExportNamedDeclarationNode::analyzeModule):
* runtime/CommonIdentifiers.cpp:
(JSC::CommonIdentifiers::lookUpPrivateName):
(JSC::CommonIdentifiers::lookUpPublicName):
(JSC::CommonIdentifiers::getPrivateName): Deleted.
(JSC::CommonIdentifiers::getPublicName): Deleted.
* runtime/CommonIdentifiers.h:
* runtime/Completion.cpp:
(JSC::checkModuleSyntax):
(JSC::evaluateModule):
* runtime/Completion.h:
* runtime/ExceptionHelpers.cpp:
(JSC::createUndefinedVariableError):
* runtime/Identifier.h:
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildren):
* runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::moduleLoader):
(JSC::JSGlobalObject::moduleRecordStructure):
* runtime/JSModuleRecord.cpp: Renamed from Source/JavaScriptCore/parser/ModuleRecord.cpp.
(JSC::JSModuleRecord::destroy):
(JSC::JSModuleRecord::finishCreation):
(JSC::printableName):
(JSC::JSModuleRecord::dump):
* runtime/JSModuleRecord.h: Renamed from Source/JavaScriptCore/parser/ModuleRecord.h.
(JSC::JSModuleRecord::ImportEntry::isNamespace):
(JSC::JSModuleRecord::createStructure):
(JSC::JSModuleRecord::create):
(JSC::JSModuleRecord::requestedModules):
(JSC::JSModuleRecord::JSModuleRecord):
(JSC::JSModuleRecord::appendRequestedModule):
(JSC::JSModuleRecord::addImportEntry):
(JSC::JSModuleRecord::addExportEntry):
(JSC::JSModuleRecord::addStarExportEntry):
* runtime/MapPrototype.cpp:
(JSC::MapPrototype::finishCreation):
* runtime/ModuleLoaderObject.cpp: Added.
(JSC::ModuleLoaderObject::ModuleLoaderObject):
(JSC::ModuleLoaderObject::finishCreation):
(JSC::ModuleLoaderObject::getOwnPropertySlot):
(JSC::printableModuleKey):
(JSC::ModuleLoaderObject::provide):
(JSC::ModuleLoaderObject::requestInstantiateAll):
(JSC::ModuleLoaderObject::resolve):
(JSC::ModuleLoaderObject::fetch):
(JSC::ModuleLoaderObject::translate):
(JSC::ModuleLoaderObject::instantiate):
(JSC::moduleLoaderObjectParseModule):
(JSC::moduleLoaderObjectRequestedModules):
(JSC::moduleLoaderObjectResolve):
(JSC::moduleLoaderObjectFetch):
(JSC::moduleLoaderObjectTranslate):
(JSC::moduleLoaderObjectInstantiate):
* runtime/ModuleLoaderObject.h: Added.
(JSC::ModuleLoaderObject::create):
(JSC::ModuleLoaderObject::createStructure):
* runtime/Options.h:

Source/WebCore:

Just fill Loader hooks with nullptr.

* bindings/js/JSDOMWindowBase.cpp:
* bindings/js/JSWorkerGlobalScopeBase.cpp:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@188752 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@Constellation
Copy link
Member Author

After implementing this part in the prototype in WebKit, we slightly modified the proposal and its name.
I've reflected it.

@Constellation Constellation changed the title Introducing the new state, "Collect" Introducing the new state, "ResolveDependencies" Aug 28, 2015
@caridy
Copy link
Contributor

caridy commented Aug 28, 2015

most likely related to #79

@guybedford
Copy link

I do think a new state is needed here, it's about distinguishing between instantiate (just this module) and instantiate all (this module and all its dependencies) as two separate states before link.

@guybedford
Copy link

Note this resolves the last issue in #40.

@caridy
Copy link
Contributor

caridy commented Aug 28, 2015

we were unable to get to this issue yesterday, we were stuck on the promise issues, but we will try to think through this issue early next week. stay tuned.

@Constellation
Copy link
Member Author

How about this? @caridy @domenic

@caridy
Copy link
Contributor

caridy commented Sep 26, 2015

sorry for the delay on this, will try to get dave and yehuda to discuss it next week. Btw, @dherman already spent some time on this few weeks ago, we haven't get to conclusion yet.

caridy added a commit that referenced this issue Oct 8, 2015
dherman pushed a commit that referenced this issue Oct 15, 2015
@dherman
Copy link

dherman commented Oct 16, 2015

@Constellation — I just want to thank you for your contribution here. I've gone ahead and specified essentially the same algorithm (but with different names) in 261543c.

@dherman dherman closed this as completed Oct 16, 2015
@Constellation
Copy link
Member Author

@dherman: :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants