Instantiate Module Environment when instantiating the module instance.
Created attachment 260413 [details] Patch
If it does not cause any errors in EWS, it's ready for review.
This patch includes the many tests for modules (it makes the patch larger).
Hm, still seems a little bit large. I'll extract the namespace object part from this patch. And temporary remove the tests. (this will be added in the subsequent patch).
Comment on attachment 260413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260413&action=review Added comments. > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:410 > + return resolveExportLoop(exec, ResolveQuery(this, exportName.impl())); Currently, this is a naive implementation. Ideally, the found resolution should be cached in the JSModuleRecord. We will do so in the subsequent patch. > Tools/Scripts/run-javascriptcore-tests:289 > + # "Source/JavaScriptCore/tests/modules.yaml" The test is disabled currently. In https://bugs.webkit.org/show_bug.cgi?id=148689, we will enable it and enable the modules in JSC shell by default.
Created attachment 260414 [details] Patch
Created attachment 260415 [details] Patch
Created attachment 260416 [details] Patch
Patch is ready for review.
I'll slightly modify the patch to maintain the export / import occurrence order.
Created attachment 260426 [details] Patch
Hm, after considering about the occurrence order of import / export, I decide we don't keep this currently. I added some rationales why we don't need to keep the occurrence of import / export entries.
Comment on attachment 260426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260426&action=review > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:276 > + // correct result under the above complex cases. I'm not sure that "Error" may be produced in the module namespace creation phase. I guess this is never produced. But since the spec algorithm itself allows the error here (`ReturnIfAbrupt(resolution).` step 3-d-ii), we take the conservative approach.
Comment on attachment 260426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260426&action=review patch LGTM, I've added some comments. There is still some of the patch I still have to read, so I won't r+ until I finish reading it, but I'm posting some initial comments now. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3839 > + // we need not to keep it alive by the Phantom node. Since the slow paths for LLInt/basline would break if we ever went there with ModuleVar ResolveType, we should assert in the slow paths that we never reach them with ModuleVar. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4099 > + case ModuleVar: Ditto. > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:55 > + // [ JSLexicalEnvironment ][ variable slots ][ additional slots for JSModuleEnvironment ] nifty. > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:82 > + const auto resolution = thisObject->moduleRecord()->resolveImport(exec, Identifier::fromUid(exec, propertyName.uid())); Nit: Can we annotate the type here explicitly? > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:86 > + if (importedModuleEnvironment->methodTable(exec->vm())->getOwnPropertySlot(importedModuleEnvironment, exec, propertyName, redirectSlot)) { Could we just do: JSModuleEnvironment::getOwnPropertySlot(...) ? > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:89 > + if (exec->hadException()) { This is scary. It looks like this should never throw. It's also a bit scary we call getValue() here. We should prove that doing so never runs any JS code. I think this assertion holds. We probably could assert that the PropertySlot has a simple value. And we should assert this much if we're calling getValue() here. > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:105 > + const auto& importEntry = pair.value; Nit: Can we annotate the type here? > Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:117 > + const auto resolution = thisObject->moduleRecord()->resolveImport(exec, Identifier::fromUid(exec, propertyName.uid())); ditto here. > Source/JavaScriptCore/runtime/JSModuleEnvironment.h:63 > + return WTF::roundUpToMultipleOf<sizeof(WriteBarrier<JSModuleRecord>)>(Base::allocationSize(symbolTable)); I thought JSEnvironmentRecord::allocationSize already returns a multiple of sizeof(WriteBarrier<JSModuleRecord>). If it does, I think we should just ASSERT that it does and remove this rounding. > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:221 > +static JSModuleRecord::Resolution resolveExportLoop(ExecState* exec, const ResolveQuery& root) Note to self: I still need to go through this. > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:440 > + SymbolTable* symbolTable = unlinkedCodeBlock->moduleEnvironmentSymbolTable(); I think we want this symbol table to be cloned. This is probably wrong if we ever cache the unlinked module program because we will do constant inference based on information the symbol table tells us (though actually it probably won't happen because caching this will cause a second write to the symbol table's singleton scope InferredValue). I think it's cleaner for each linked module code block to own its linked symbol table. > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:484 > + const auto resolution = importedModule->resolveExport(exec, importEntry.importName); Nit: Can we explicitly write the type here? > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:506 > + // Module environment contains the heap allocated "var", "let" and "const". I think it should also contain "class" > Source/JavaScriptCore/runtime/JSScope.cpp:72 > + const auto resolution = moduleRecord->resolveImport(exec, ident); nit: ditto. Also explicitly write the type.
Created attachment 260501 [details] Patch
Created attachment 260503 [details] Patch
Comment on attachment 260426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260426&action=review Thank you for your comment! I've uploaded the revised patch. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3839 >> + // we need not to keep it alive by the Phantom node. > > Since the slow paths for LLInt/basline would break if we ever went there with ModuleVar ResolveType, we should > assert in the slow paths that we never reach them with ModuleVar. Sounds nice. I've added it. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4099 >> + case ModuleVar: > > Ditto. Yes. Currently, the operation function called in the baseline does not take the resolveType. So if we would like to check it inside the operation function, we need to pass it as the new argument OR pass the byte code pointer to retrieve the value. But I think it's not so good. So before emitting the call to that operation function, I've inserted the ASSERT(resolveType != ModuleVar) in baseline JIT compiler side. >> Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:55 >> + // [ JSLexicalEnvironment ][ variable slots ][ additional slots for JSModuleEnvironment ] > > nifty. Yeah! >> Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:82 >> + const auto resolution = thisObject->moduleRecord()->resolveImport(exec, Identifier::fromUid(exec, propertyName.uid())); > > Nit: Can we annotate the type here explicitly? Sure. Annotated. >> Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:86 >> + if (importedModuleEnvironment->methodTable(exec->vm())->getOwnPropertySlot(importedModuleEnvironment, exec, propertyName, redirectSlot)) { > > Could we just do: JSModuleEnvironment::getOwnPropertySlot(...) ? Looks nice. This call should succeed because `resolveImport` resolves the existence. Changed. >> Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:89 >> + if (exec->hadException()) { > > This is scary. It looks like this should never throw. > It's also a bit scary we call getValue() here. We should > prove that doing so never runs any JS code. I think this assertion holds. > We probably could assert that the PropertySlot has a simple value. And we should assert this much > if we're calling getValue() here. Looks very nice. I've inserted the assertion, ASSERT(redirectSlot.isValue()) etc. >> Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:105 >> + const auto& importEntry = pair.value; > > Nit: Can we annotate the type here? Annotated. >> Source/JavaScriptCore/runtime/JSModuleEnvironment.cpp:117 >> + const auto resolution = thisObject->moduleRecord()->resolveImport(exec, Identifier::fromUid(exec, propertyName.uid())); > > ditto here. Annotated. >> Source/JavaScriptCore/runtime/JSModuleEnvironment.h:63 >> + return WTF::roundUpToMultipleOf<sizeof(WriteBarrier<JSModuleRecord>)>(Base::allocationSize(symbolTable)); > > I thought JSEnvironmentRecord::allocationSize already returns a multiple of sizeof(WriteBarrier<JSModuleRecord>). > If it does, I think we should just ASSERT that it does and remove this rounding. Sounds nice. Fixed. >> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:440 >> + SymbolTable* symbolTable = unlinkedCodeBlock->moduleEnvironmentSymbolTable(); > > I think we want this symbol table to be cloned. > This is probably wrong if we ever cache the unlinked module program > because we will do constant inference based on information the symbol > table tells us (though actually it probably won't happen because caching > this will cause a second write to the symbol table's singleton scope InferredValue). > I think it's cleaner for each linked module code block to own its linked symbol table. Very nice catch!!! I've changed the design slightly. Now, UnlinkedModuleProgramCodeBlock does not have SymbolTable*. Instead, it register it to the constant pool and remember the offset to that table. (As the same to the UnlinkedFunctionCodeBlock) CodeBlock linking automatically clones the SymbolTables inside the constant register pool (This is the same to the UnlinkedFunctionCodeBlock). And here, by using the remembered constant register offset, we retrieve the correctly cloned symbol table from the linked CodeBlock. >> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:484 >> + const auto resolution = importedModule->resolveExport(exec, importEntry.importName); > > Nit: Can we explicitly write the type here? Fixed. >> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:506 >> + // Module environment contains the heap allocated "var", "let" and "const". > > I think it should also contain "class" Nice, fixed. >> Source/JavaScriptCore/runtime/JSScope.cpp:72 >> + const auto resolution = moduleRecord->resolveImport(exec, ident); > > nit: ditto. Also explicitly write the type. Yeah, fixed.
Created attachment 260506 [details] Patch
Comment on attachment 260506 [details] Patch Ah, I found one problem in my new design (about SymbolTable), I'll slightly update it.
Created attachment 260514 [details] Patch
Created attachment 260525 [details] Patch
Created attachment 260527 [details] Patch
Comment on attachment 260527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260527&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1799 > + } This is needed because (1): Linking CodeBlock requires that the all imported module environments are already created. So the symbol table for the module environments should be already cloned and used. (2): The module environment is the top-most lexical/variable environment for the original module at the same time. So like "LocalClosureVar" will be performed to the module environment. So when linking, the symbol table used for the module environment should be locate in the constant registers of the linked code block.
Created attachment 260536 [details] Patch
Comment on attachment 260536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260536&action=review Added comments for ease of reviews. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1799 > + } This is needed because (1): Linking CodeBlock requires that the all imported module environments are already created. So the symbol table for the module environments should be already cloned and used. (2): The module environment is the top-most lexical/variable environment for the original module at the same time. So like "LocalClosureVar" will be performed to the module environment. So when linking, the symbol table used for the module environment should be locate in the constant registers of the linked code block. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:662 > + codeBlock->setModuleEnvironmentSymbolTableConstantRegisterOffset(constantSymbolTable->index()); Changed. Now, UnlinkedModuleProgramCodeBlock stores the symbol table in the constant pool as well as the UnlinkedFunctionCodeBlock does. And remember the index to retrieve the symbol table in ModuleProgramExecutable to create the ModuleEnvironment. > Source/JavaScriptCore/jit/JITOperations.cpp:1944 > + Insert the assertion. > Source/JavaScriptCore/jit/JITOperations.cpp:1983 > + Insert the assertion. > Source/JavaScriptCore/llint/LLIntData.cpp:155 > + static_assert(ClosureVarWithVarInjectionChecks == 9, "LLInt assumes ClosureVarWithVarInjectionChecks ResultType is == 9"); Rebase the LLIntData. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1371 > + Insert the assertion. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:200 > +const ClosureVarWithVarInjectionChecks = 9 Rebase the LLIntData. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2117 > + getConstantScope(1) Use the getConstantScope. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1985 > + getConstantScope(1) Ditto. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:680 > + assertion inserted. > Source/JavaScriptCore/runtime/Executable.cpp:443 > + executable->m_moduleEnvironmentSymbolTable.set(exec->vm(), executable, jsCast<SymbolTable*>(unlinkedModuleProgramCode->constantRegister(unlinkedModuleProgramCode->moduleEnvironmentSymbolTableConstantRegisterOffset()).get())->cloneScopePart(exec->vm())); Cloning the symbol table and storing it in ModuleProgramExecutable. > Source/JavaScriptCore/runtime/JSScope.cpp:230 > + } Collect the imported bindings for the TDZ in direct eval call. > Source/JavaScriptCore/runtime/JSScope.cpp:265 > + Currently, I decided to make the Module environment as (1) JSModuleEnvironment* and (2) its scope type is LexicalScope. It also makes JSModuleEnvironment->isLexicalScope() true. I think it is nice, what do you think of?
Comment on attachment 260536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260536&action=review r=me with comments > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1798 > + m_constantRegisters[unlinkedModuleProgramCodeBlock->moduleEnvironmentSymbolTableConstantRegisterOffset() - FirstConstantRegisterIndex].set(*m_vm, ownerExecutable, clonedSymbolTable); There is a method on CodeBlock that already does this: CodeBlock::replaceConstant > Source/JavaScriptCore/runtime/Executable.cpp:442 > + // Clone the module environment symbol table for this executable (and the code block linked and instantiated later). this comment is not needed. > Source/JavaScriptCore/runtime/JSModuleRecord.cpp:532 > + symbolTablePut(moduleEnvironment, exec, unlinkedFunctionExecutable->name(), function, false, true); I wonder why we have two of these. Both JSSymbolTableObject implements one and JSLexicalEnvironment. Since moduleEnvironment is a JSLexicalEnvironment, we should probably call that implementation, but the implementations look nearly identical except for what we do with the watchpoint. We should consolidate if possible. > Source/JavaScriptCore/runtime/JSScope.cpp:226 > + if (scope->isModuleScope()) { Nit: I'd move this below the "continue" check. >> Source/JavaScriptCore/runtime/JSScope.cpp:265 >> + > > Currently, I decided to make the Module environment as (1) JSModuleEnvironment* and (2) its scope type is LexicalScope. > It also makes JSModuleEnvironment->isLexicalScope() true. I think it is nice, what do you think of? Seems good to me. We can change it later if need be.
Comment on attachment 260536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260536&action=review Thank you for your review! >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1798 >> + m_constantRegisters[unlinkedModuleProgramCodeBlock->moduleEnvironmentSymbolTableConstantRegisterOffset() - FirstConstantRegisterIndex].set(*m_vm, ownerExecutable, clonedSymbolTable); > > There is a method on CodeBlock that already does this: CodeBlock::replaceConstant Great! Changed to use it. >> Source/JavaScriptCore/runtime/Executable.cpp:442 >> + // Clone the module environment symbol table for this executable (and the code block linked and instantiated later). > > this comment is not needed. Dropped. >> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:532 >> + symbolTablePut(moduleEnvironment, exec, unlinkedFunctionExecutable->name(), function, false, true); > > I wonder why we have two of these. Both JSSymbolTableObject implements one and JSLexicalEnvironment. > Since moduleEnvironment is a JSLexicalEnvironment, we should probably call that implementation, but > the implementations look nearly identical except for what we do with the watchpoint. We should consolidate if possible. After investigating the implementation, I've found that JSLexicalEnvironment calls `isValid()` to protect against the inspector call. In the meantime, I leave it as is. And attempt to fix this in the subsequent patch. https://bugs.webkit.org/show_bug.cgi?id=148783 >> Source/JavaScriptCore/runtime/JSScope.cpp:226 >> + if (scope->isModuleScope()) { > > Nit: I'd move this below the "continue" check. Sounds nice. Fixed.
Committed r189339: <http://trac.webkit.org/changeset/189339>