Bug 148581 – [ES6] Introduce ModuleProgramExecutable families and compile Module code to bytecode

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

[Yusuke Suzuki]

Introduce ModuleProgramExecutable families (ModuleProgramExecutable, UnlinkedModuleProgramCodeBlock, ModuleProgramCodeBlock, etc.)

[Yusuke Suzuki]

CodeBlock linking will not be included in this patch yet, because it involves JSModuleEnvironment instantiation.

[Yusuke Suzuki]

Created attachment 260186 [details]
Patch

[Yusuke Suzuki]

Created attachment 260197 [details]
Patch

[Yusuke Suzuki]

Build fix for Internals.cpp in WebCore.

[Yusuke Suzuki]

Created attachment 260198 [details]
Patch

slightly update the code comment

[Yusuke Suzuki]

I'll just modify one thing, calling makeFunction after putting the correct variable environment to the TDZ stack.

[Yusuke Suzuki]

Created attachment 260219 [details]
Patch

Fix the makeFunction timing. It should be called after the TDZ stack is set

[Saam Barati]

Comment on attachment 260219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260219&action=review

r=me

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:-415
> -    WriteBarrier<SymbolTable> m_symbolTable;

This was unused before?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:544
> +    // The unlinked module program code block only holds the symbol table. Not holds the module environment. So while

"Not holds the module environment" => "It does not hold the module environment."

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:566
> +    WriteBarrier<SymbolTable> m_symbolTable;

Nit: Since a CodeBlock may own many SymbolTables in its constant pool, maybe we can give this a more descriptive name?
Maybe something like "moduleEnvironmentSymbolTable"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:582
> +    {

It seems like this code has a lot in common with pushLexicalScope. I wonder if we can find a way to abstract it nicely.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:617
> +    m_symbolTableStack.append(SymbolTableStackEntry { Strong<SymbolTable>(*m_vm, symbolTable), m_scopeRegister, false, constantSymbolTable->index() });

I think this may be wrong. We really want a dedicated scope register for this environment.
m_scopeRegister won't always refer to the "module scope" because as we push
other lexical scopes on top of this, m_scopeRegister will take on those values.
Maybe one solution is to do this:
RegisterID* moduleScope = addVar();
emitMove(moduleScope, m_scopeRegister);
and then pass this value into m_symbolTableStack.append(.)

Another approach is to use m_topMostScope (which basically does that I stated above).

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:621
> +    for (auto& entry : lexicalVariables) {

Same here. We can probably abstract this somehow. Maybe we can create a function that acccepts a filter function which causes some entries to be accepter/ignored?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:679
> +            // Module EntryPoint (executed last):

I like these comments. I'd also add link order as well as execution order,
so maybe here:
"(executed last, linked first)"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2898
> +void ModuleProgramNode::emitBytecode(BytecodeGenerator& generator, RegisterID*)

Nit: This looks identical to ProgramNode::emitBytecode.
Maybe we can abstract them into both calling a static function.

[Yusuke Suzuki]

Comment on attachment 260219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260219&action=review

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:-415
>> -    WriteBarrier<SymbolTable> m_symbolTable;
> 
> This was unused before?

Yup! It was the dead code.

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:544
>> +    // The unlinked module program code block only holds the symbol table. Not holds the module environment. So while
> 
> "Not holds the module environment" => "It does not hold the module environment."

Oops. Fixed.

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:566
>> +    WriteBarrier<SymbolTable> m_symbolTable;
> 
> Nit: Since a CodeBlock may own many SymbolTables in its constant pool, maybe we can give this a more descriptive name?
> Maybe something like "moduleEnvironmentSymbolTable"

Sounds very nice. Descriptive name is always fine. I've changed it.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:582
>> +    {
> 
> It seems like this code has a lot in common with pushLexicalScope. I wonder if we can find a way to abstract it nicely.

Yup. These 2 parts are similar to pushLexicalScopeInternal.
The difference between this function and pushLexicalScopeInternal are,
1. pushLexicalScopeInternal creates the SymbolTable
2. pushLexicalScopeInternal emit the byte code to create the scope
and the same part is,
1. add entries to SymbolTable
2. append symbol table to the symbol table stack
3. append TDZ stack and fill the TDZ stack values

So I'll extract 1, 2, 3 functions from the pushLexicalScopeInternal and use it in pushLexicalScopeInternal and module symbol table instantiation phase.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:617
>> +    m_symbolTableStack.append(SymbolTableStackEntry { Strong<SymbolTable>(*m_vm, symbolTable), m_scopeRegister, false, constantSymbolTable->index() });
> 
> I think this may be wrong. We really want a dedicated scope register for this environment.
> m_scopeRegister won't always refer to the "module scope" because as we push
> other lexical scopes on top of this, m_scopeRegister will take on those values.
> Maybe one solution is to do this:
> RegisterID* moduleScope = addVar();
> emitMove(moduleScope, m_scopeRegister);
> and then pass this value into m_symbolTableStack.append(.)
> 
> Another approach is to use m_topMostScope (which basically does that I stated above).

Using m_topMostScope is nice because the module environment is always the top most scope under the module.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:621
>> +    for (auto& entry : lexicalVariables) {
> 
> Same here. We can probably abstract this somehow. Maybe we can create a function that acccepts a filter function which causes some entries to be accepter/ignored?

Yeah, extracted function will take a filter function.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:679
>> +            // Module EntryPoint (executed last):
> 
> I like these comments. I'd also add link order as well as execution order,
> so maybe here:
> "(executed last, linked first)"

Nice. I've added it.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2898
>> +void ModuleProgramNode::emitBytecode(BytecodeGenerator& generator, RegisterID*)
> 
> Nit: This looks identical to ProgramNode::emitBytecode.
> Maybe we can abstract them into both calling a static function.

Looks nice. Fixed.

[Yusuke Suzuki]

Comment on attachment 260219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260219&action=review

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:582
>>> +    {
>> 
>> It seems like this code has a lot in common with pushLexicalScope. I wonder if we can find a way to abstract it nicely.
> 
> Yup. These 2 parts are similar to pushLexicalScopeInternal.
> The difference between this function and pushLexicalScopeInternal are,
> 1. pushLexicalScopeInternal creates the SymbolTable
> 2. pushLexicalScopeInternal emit the byte code to create the scope
> and the same part is,
> 1. add entries to SymbolTable
> 2. append symbol table to the symbol table stack
> 3. append TDZ stack and fill the TDZ stack values
> 
> So I'll extract 1, 2, 3 functions from the pushLexicalScopeInternal and use it in pushLexicalScopeInternal and module symbol table instantiation phase.

At least, extracting (3) was nice. So, in the meantime, I just extracted the (3).

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:679
>>> +            // Module EntryPoint (executed last):
>> 
>> I like these comments. I'd also add link order as well as execution order,
>> so maybe here:
>> "(executed last, linked first)"
> 
> Nice. I've added it.

Technically, "Link" order is the same to the execution order.
But, the "link" order is not so important because when linking we already instantiated the all module environment. And when executing, we already linked the all modules.

The interesting point is this. When the execution starts, the all linking is already finished.
So function bindings are already instantiated. This causes the above crazy behavior :-)

[Yusuke Suzuki]

Comment on attachment 260219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260219&action=review

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:582
>>>> +    {
>>> 
>>> It seems like this code has a lot in common with pushLexicalScope. I wonder if we can find a way to abstract it nicely.
>> 
>> Yup. These 2 parts are similar to pushLexicalScopeInternal.
>> The difference between this function and pushLexicalScopeInternal are,
>> 1. pushLexicalScopeInternal creates the SymbolTable
>> 2. pushLexicalScopeInternal emit the byte code to create the scope
>> and the same part is,
>> 1. add entries to SymbolTable
>> 2. append symbol table to the symbol table stack
>> 3. append TDZ stack and fill the TDZ stack values
>> 
>> So I'll extract 1, 2, 3 functions from the pushLexicalScopeInternal and use it in pushLexicalScopeInternal and module symbol table instantiation phase.
> 
> At least, extracting (3) was nice. So, in the meantime, I just extracted the (3).

After considering about this, I also extracted (1). But extracting (2) is not so good (because its so small). So I've just extracted (1) & (3).

[Yusuke Suzuki]

Committed r189201: <http://trac.webkit.org/changeset/189201>