Bug 148053 – [ES6] Instantiate Module Environment bindings and execute module

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Instantiate Module Environment when instantiating the module instance.

[Yusuke Suzuki]

Created attachment 260413 [details]
Patch

[Yusuke Suzuki]

If it does not cause any errors in EWS, it's ready for review.

[Yusuke Suzuki]

This patch includes the many tests for modules (it makes the patch larger).

[Yusuke Suzuki]

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).

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260414 [details]
Patch

[Yusuke Suzuki]

Created attachment 260415 [details]
Patch

[Yusuke Suzuki]

Created attachment 260416 [details]
Patch

[Yusuke Suzuki]

Patch is ready for review.

[Yusuke Suzuki]

I'll slightly modify the patch to maintain the export / import occurrence order.

[Yusuke Suzuki]

Created attachment 260426 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Saam Barati]

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.

[Yusuke Suzuki]

Created attachment 260501 [details]
Patch

[Yusuke Suzuki]

Created attachment 260503 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260506 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 260506 [details]
Patch

Ah, I found one problem in my new design (about SymbolTable), I'll slightly update it.

[Yusuke Suzuki]

Created attachment 260514 [details]
Patch

[Yusuke Suzuki]

Created attachment 260525 [details]
Patch

[Yusuke Suzuki]

Created attachment 260527 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 260536 [details]
Patch

[Yusuke Suzuki]

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?

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r189339: <http://trac.webkit.org/changeset/189339>