* SUMMARY
Arrow Function before super() causes TDZ, should it?
You get a non-obvious TDZ runtime ReferenceError when you create an arrow function before calling super() in a subclass.
* TEST
<script>
class Parent { }
class Subclass extends Parent {
constructor() {
let x = () => {}; // TDZ because of capturing "this"
super();
}
}
console.log(new Subclass);
</script>
* PRODUCES
[Error] ReferenceError: Cannot access uninitialized variable.
Subclass
(anonymous function)
> constructor() {
> let x = () => {}; // TDZ because of capturing "this"
> super();
> }
I should clarify that the comment is my assumption. But I don't know what else could be causing the issue =)
I don't think this should trigger a TDZ. Even if `x` was evaluated `x()` I don't think it should be an issue as long as it doesn't actually reference `this`.
Yes, this is error message caused because of TDZ check before super() call. Now it always captures 'this' without checking if it is used and whether or not
I think we can be smart and not do anything if the "this"
isn't used in the arrow function. But after speaking with Joe,
I believe it should never throw on construction. The "this"
should be under TDZ, which means it should be only dependent on
time and not location in code. So, as long as super() is invoked
before a "this" is evaluated inside the arrow function, we should
be okay. For example, this is ok:
class C {
constructor() {
let x = () => (false ? this : 20);
x();
super();
}
}
(In reply to comment #3)
> I think we can be smart and not do anything if the "this"
> isn't used in the arrow function. But after speaking with Joe,
> I believe it should never throw on construction. The "this"
> should be under TDZ, which means it should be only dependent on
> time and not location in code. So, as long as super() is invoked
> before a "this" is evaluated inside the arrow function, we should
> be okay. For example, this is ok:
>
> class C {
> constructor() {
> let x = () => (false ? this : 20);
> x();
> super();
> }
> }
And also things like this should work:
class C {
constructor() {
let p = false;
let x = () => (p ? this : 20);
x(); // returns 20
super();
p = true;
x(); // returns 'this'
}
}
This should return without throwing.
Comment on attachment 262329[details]
Patch
I've prepared new patch with merged op_codes and correct TDZ check of 'this'. I'll clear it little bit and upload tomorrow.
Comment on attachment 262397[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=262397&action=review> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:485
> + RefPtr<RegisterID> parentScope = m_lexicalEnvironmentRegister
We have a byte code variable m_topMostScope that does what you're doing here, but I think this logic is wrong.
Consider this program:
constructor() {
if (c) {
let x = 20;
function captureX() { }
if (c) {
let x = 20;
function captureX() { return x; }
let arr = (blah) => blah;
}
}
}
The "arr" won't be created with the parent scope that contains the "this".
I think you just want a resolveScope followed by a getFromScope.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2949
> + emitPutToScope(scopeRegister(), thisVar, thisRegister(), ThrowIfNotFound, NotInitialization);
I think special casing "this" as a new thing both in terms of a resolve type
and in terms of a variable on JSScope is the wrong way to go about implementing this feature.
Here is one suggestion on how to solve this differently:
Anytime a function has an arrow function nested inside of it,
the parent function should create a lexical environment. Once this parent
function also creates the "this" variable, it should place it inside
the lexical environment it created. (This solves the problem in this code which keeps putting
the "this" into the activation every time an arrow function is created
even if "this" hasn't changed). Any time you make a call to super()
and you have a nested arrow function, you update the "this" inside
the lexical environment. Child functions that read from "this" can
just do so the normal way: resolveScope() then getFromScope().
The parent function that has the "this" inside the lexical environment
should just do what it normally does for lexical environments. The "this"
identifier should have a slot inside the symbol table, etc. I think this
would take away almost all this special case code for "this". Then, the "thisNode",
when inside an arrow function, should be smart and load the "this" from
the lexical environment using resolveScope() then getFromScope(). I believe
this suggested solution will cause "this" inside an environment to just work
for the most part.
> Source/JavaScriptCore/jit/JITOperations.cpp:1995
> + if (getPutInfo.resolveType() == LexicallyBoundVar) {
I think special casing this is wrong. We should just be able to put the "this" identifier into an environment and have this code work.
Created attachment 262435[details]
Patch
Updated draft version of the patch. Implemented in function context. (EvalNode & ProgramNode is not implemented). Version #3
Attachment 262435[details] did not pass style-queue:
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1951: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1951: Should have a space between // and comment [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2020: Should have a space between // and comment [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2020: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2022: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2023: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 6 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262397[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=262397&action=review
I've uploaded new patch. It is as previous just Draft, but it look more 'mature' for me. Patch covers only 'Function' case in BytecodeGenerator, because creating of lexical_env already implemented in it. If new patch more or less ok in approach to store 'this' I'll try to implement lexical_env for 'Program' & 'Eval' case
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:485
>> + RefPtr<RegisterID> parentScope = m_lexicalEnvironmentRegister
>
> We have a byte code variable m_topMostScope that does what you're doing here, but I think this logic is wrong.
> Consider this program:
> constructor() {
> if (c) {
> let x = 20;
> function captureX() { }
> if (c) {
> let x = 20;
> function captureX() { return x; }
> let arr = (blah) => blah;
> }
> }
> }
>
> The "arr" won't be created with the parent scope that contains the "this".
>
> I think you just want a resolveScope followed by a getFromScope.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2949
>> + emitPutToScope(scopeRegister(), thisVar, thisRegister(), ThrowIfNotFound, NotInitialization);
>
> I think special casing "this" as a new thing both in terms of a resolve type
> and in terms of a variable on JSScope is the wrong way to go about implementing this feature.
>
> Here is one suggestion on how to solve this differently:
> Anytime a function has an arrow function nested inside of it,
> the parent function should create a lexical environment. Once this parent
> function also creates the "this" variable, it should place it inside
> the lexical environment it created. (This solves the problem in this code which keeps putting
> the "this" into the activation every time an arrow function is created
> even if "this" hasn't changed). Any time you make a call to super()
> and you have a nested arrow function, you update the "this" inside
> the lexical environment. Child functions that read from "this" can
> just do so the normal way: resolveScope() then getFromScope().
>
> The parent function that has the "this" inside the lexical environment
> should just do what it normally does for lexical environments. The "this"
> identifier should have a slot inside the symbol table, etc. I think this
> would take away almost all this special case code for "this". Then, the "thisNode",
> when inside an arrow function, should be smart and load the "this" from
> the lexical environment using resolveScope() then getFromScope(). I believe
> this suggested solution will cause "this" inside an environment to just work
> for the most part.
Done. New patch is smaller than previous :-)
Created attachment 262441[details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 262442[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 262540[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=262540&action=review
This patch is much better than before, but I think there are a few things that still need to be done to make it cleaner and correct.
> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1663
> + , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && (unlinkedCodeBlock->codeType() == FunctionCode || unlinkedCodeBlock->codeType() == GlobalCode || unlinkedCodeBlock->codeType() == EvalCode) )
Why is this needed?
> Source/JavaScriptCore/bytecode/CodeBlock.h:350
> + void setNeedActivation(bool value)
this is unused.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:170
> + if (programNode->usesArrowFunction())
Should this be "if (programNode->usesArrowFunction() || usesEval())"?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:485
> + if (functionNode->usesThis() || codeBlock->usesEval() || functionNode->usesArrowFunction()) {
I think we need to emitResolveScoe then emitGetFromScope for every "this" node, right?
Just doing it once I think is not sufficient. Consider:
class C {
constructor() {
let f = (c) => c ? this : {};
f(false);
super();
f(true);
}
}
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:490
> }
I think you should add a line of code here that calls "emitUpdateScopeIfNeeded()"
Currently, you emit code for "emitUpdateScopeIfNeeded()" on each arrow function that's emitted.
This is excessive. If you simply call "emitUpdateScopeIfNeeded()" here and anytime you call super(),
we should always have the latest "this" value.
Also, I think you probably need to call "initializeEmptyVarLexicalEnvironment()" somewhere
inside this function to be consistent with ProgramNode/EvalNode.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:540
> + initializeEmptyVarLexicalEnvironment(vm);
(I have a note on this function below. I think it can be nicer).
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:541
> + else
This shouldn't be in an else clause. It should happen regardless of the above condition.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:788
> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironment(VM& vm)
I think we can do this in a better way. We can write this purely in terms of pushLexicalScopeInternal().
The reason is that pushLexicalScopeInternal does necessary bookkeeping for handling
scopes, etc (especially important within try/catch).
If you look at initializeDefaultParameterValuesAndSetupFunctionScopeStack() it interfaces with this function.
I think something like this would work:
```
VariableEnvironment environment;
auto addResult = environment.add(thisIdentifier);
addResult.iterator->value.setIsCaptured();
pushLexicalScopeInternal(environment, false, nullptr, TDZRequirement::(Whatever it doesn't matter), ScopeType::LetConstScope, ScopeRegisterType::Block);
```
Note that this works because the ::variable() function explicitly looks for the "this" identifier and
therefore we won't ever search the scope stack for "this".
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2937
> +void BytecodeGenerator::emitUpdateScopeIfNeed()
Lets call this "emitPutThisToScopeIfNeeded" or something along those lines.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2942
> + emitPutToScope(scopeRegister(), thisVar, thisRegister(), DoNotThrowIfNotFoundAndReturnEmpty, NotInitialization);
I don't like the idea of adding new resolve modes like "DoNotThrowIfNotFoundAndReturnEmpty".
Why would we ever not find "this"?
We should always find it.
We should make this guarantee by making sure every function places "this" into the lexical environment if needed.
Comment on attachment 262540[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=262540&action=review
Thanks for the review!
Now I'm running the tests in debug mode.
After success I'll upload the patch. I hope that it will be happened today :-)
>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1663
>> + , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && (unlinkedCodeBlock->codeType() == FunctionCode || unlinkedCodeBlock->codeType() == GlobalCode || unlinkedCodeBlock->codeType() == EvalCode) )
>
> Why is this needed?
It allow to avoid ASSERT error in method needsActivation for Eval and Program case. As I understand we create lexical scope for them so we need activate it.
See assert ASSERT(m_lexicalEnvironmentRegister.isValid() == m_needsActivation);
>> Source/JavaScriptCore/bytecode/CodeBlock.h:350
>> + void setNeedActivation(bool value)
>
> this is unused.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:170
>> + if (programNode->usesArrowFunction())
>
> Should this be "if (programNode->usesArrowFunction() || usesEval())"?
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:485
>> + if (functionNode->usesThis() || codeBlock->usesEval() || functionNode->usesArrowFunction()) {
>
> I think we need to emitResolveScoe then emitGetFromScope for every "this" node, right?
> Just doing it once I think is not sufficient. Consider:
> class C {
> constructor() {
> let f = (c) => c ? this : {};
> f(false);
> super();
> f(true);
> }
> }
Hmm, it seems that I don't understand clearly what does this methods. Your examples very close to scripts that I use to test TDZ in 'super()', see tests with name arrowfunction-tdz-1.js.
How I understand this code. Each time when arrow function is invoked, before run body of function it loads 'this' value to thisRegister. So if I this value is changed by parent function by 'super()', we get from scope of arrow function new 'this' value, because 'this' in lexical_scope will be updated by emitUpdateScopeIfNeed method that we call just after calling 'super()' see change in Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp line 730.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:490
>> }
>
> I think you should add a line of code here that calls "emitUpdateScopeIfNeeded()"
> Currently, you emit code for "emitUpdateScopeIfNeeded()" on each arrow function that's emitted.
> This is excessive. If you simply call "emitUpdateScopeIfNeeded()" here and anytime you call super(),
> we should always have the latest "this" value.
>
> Also, I think you probably need to call "initializeEmptyVarLexicalEnvironment()" somewhere
> inside this function to be consistent with ProgramNode/EvalNode.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:541
>> + else
>
> This shouldn't be in an else clause. It should happen regardless of the above condition.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:788
>> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironment(VM& vm)
>
> I think we can do this in a better way. We can write this purely in terms of pushLexicalScopeInternal().
> The reason is that pushLexicalScopeInternal does necessary bookkeeping for handling
> scopes, etc (especially important within try/catch).
>
> If you look at initializeDefaultParameterValuesAndSetupFunctionScopeStack() it interfaces with this function.
> I think something like this would work:
>
> ```
> VariableEnvironment environment;
> auto addResult = environment.add(thisIdentifier);
> addResult.iterator->value.setIsCaptured();
> pushLexicalScopeInternal(environment, false, nullptr, TDZRequirement::(Whatever it doesn't matter), ScopeType::LetConstScope, ScopeRegisterType::Block);
> ```
> Note that this works because the ::variable() function explicitly looks for the "this" identifier and
> therefore we won't ever search the scope stack for "this".
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2937
>> +void BytecodeGenerator::emitUpdateScopeIfNeed()
>
> Lets call this "emitPutThisToScopeIfNeeded" or something along those lines.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2942
>> + emitPutToScope(scopeRegister(), thisVar, thisRegister(), DoNotThrowIfNotFoundAndReturnEmpty, NotInitialization);
>
> I don't like the idea of adding new resolve modes like "DoNotThrowIfNotFoundAndReturnEmpty".
> Why would we ever not find "this"?
> We should always find it.
> We should make this guarantee by making sure every function places "this" into the lexical environment if needed.
In case if we put 'this' inside of the constructor before 'super()' without flag we will receive ASSERT error. If we will try to get 'this' inside of arrow function that invokes before 'super()' we will receive undefined and miss the TDZ error.
So that why I made changes in JITOperation.cpp operationGetFromScope/operationPutToScope
Comment on attachment 263614[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=263614&action=review
Overall, I like this approach. There are some details that need fixing. I've commented below.
> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1658
> + , m_scopeForThisRegister(unlinkedCodeBlock->scopeForThisRegister())
not used.
> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1660
> + , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && (unlinkedCodeBlock->codeType() == FunctionCode || unlinkedCodeBlock->codeType() == GlobalCode || unlinkedCodeBlock->codeType() == EvalCode) )
Why is this change needed?
> Source/JavaScriptCore/bytecode/CodeBlock.h:349
> + VirtualRegister scopeForThisRegister()
It doesn't look like this is used. And this also seems weird and unnecessary, we should be able to accomplish whatever this can without caching the scope.
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:121
> + bool isDerivedContext() const { return m_isDerivedContext;}
> + bool isArrowFunctionContext() const { return m_isArrowFunctionContext;}
style: ";}" => "; }"
> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
> + unsigned m_isDerivedContext: 1;
What exactly does m_isDerivedContext mean?
Does it strictly mean I'm inheriting "this/super/new.target/arguments" from my parent context?
If so, maybe a better name is m_isInheritedLexicalEnvironment.
If not, what does it mean?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:488
> + if (functionNode->usesThis() || m_isDerivedContext) {
When is an arrow function ever not an "m_isDerivedContext"?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:492
> + emitGetThisFromArrowFunctionLexicalEnvironment();
Is this needed here?
Doesn't each "this" AST node call this function? Maybe it's needed for other things that assume m_thisRegister contains the correct value?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:493
> + emitGetNewTargeFromArrowFunctionLexicalEnvironment();
why does this depend on "usesThis()"? I think this should happen regardless.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1883
> +Variable BytecodeGenerator::variable(const Identifier& property, bool isLocal)
naming nit: "isLocal" => "isLocalThis"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1950
> +Variable BytecodeGenerator::createThisVariable()
I would name this to: "createScopedThisVariable"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1955
> +Variable BytecodeGenerator::createVariableFromStack(Identifier varIdentifier)
This seems very wrong to me. Why would this always be in the top-most scope on the stack? I don't think that's true.
We already know what lexicalEnvironmentRegister holds the scoped "this", we should use that to construct our
own "this" variable. I would remove this function entirely and do the logic in the function above.
Maybe one way to create the proper scoped "this" variable is to just implement your own walking
of the m_symbolTableStack. We know for sure that it must be in there, so we should just assert
that we always find it. That way we ensure any callers of this function only call it if the "this"
is indeed on the scope stack.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3919
> +void BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment()
nit: I think a better name for this is "emitResolveArrowFunctionContextLexicalEnvironment".
Also, I think this only ever has to happen once. It should always resolve to the same scope.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3920
> +{
It's probably good form to assert that we're an arrow function in this function.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:492
> + void emitLoadArrowFunctionLexicalEnvironment();
I think you can make this private (if it's not already) and only have it be called from emitGetThisFromArrowFunctionLexicalEnvironment
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:683
> + template<typename LookUpVarKindFunctor>
> + bool instantiateLexicalVariables2(const VariableEnvironment&, SymbolTable*, ScopeRegisterType, LookUpVarKindFunctor);
debugging code?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:789
> + void initializeEmptyVarLexicalEnvironmentIfNeeded(SymbolTable* = nullptr);
I think this needs a better name, something like:
"initializeLexicalEnvironmentContextForArrowFunctions"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:831
> + RegisterID* m_arrowFunctionlexicalEnvRegister { nullptr };
This is a very weird member variable. It means two different things depending on what context we're generating code in.
I think it'd be clearer to separate this out into two different member variables.
One member variable should represent the environment that this gets assigned to in "initializeEmptyVarLexicalEnvironmentIfNeeded()" (or whatever you rename it to).
The other member variable should represent the result of "emitLoadArrowFunctionEnvironment"
Even though these things are similar because they're the environment registers that hold "this"/"new.target"/etc,
they're not exactly equal because we decide to create one, and we resolve to the other. I think separating them
is cleaner. It only costs us an extra pointer in memory which is fine inside the BytecodeGenerator.
Comment on attachment 263614[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=263614&action=review
Thanks for the review! I'll hope to land the patch tomorrow
>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
>> + unsigned m_isDerivedContext: 1;
>
> What exactly does m_isDerivedContext mean?
> Does it strictly mean I'm inheriting "this/super/new.target/arguments" from my parent context?
> If so, maybe a better name is m_isInheritedLexicalEnvironment.
> If not, what does it mean?
It means: arrow function was created in constructor of class that has parent (constructorKind() == ConstructorKind::Derived). So in such arrow function we can invoke super and we need put 'this' to arrow function lexical scope after invoking 'super' and etc.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:492
>> + emitGetThisFromArrowFunctionLexicalEnvironment();
>
> Is this needed here?
> Doesn't each "this" AST node call this function? Maybe it's needed for other things that assume m_thisRegister contains the correct value?
We call this function in 'this' AST node only when we in constructor that contains arrow function, and there is possibility that 'this' is created by 'super()' in arrow function. In current place we load bound value to m_thisRegister only at start of arrow function if 'this' is used in body of current function.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:493
>> + emitGetNewTargeFromArrowFunctionLexicalEnvironment();
>
> why does this depend on "usesThis()"? I think this should happen regardless.
I'll fix
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1955
>> +Variable BytecodeGenerator::createVariableFromStack(Identifier varIdentifier)
>
> This seems very wrong to me. Why would this always be in the top-most scope on the stack? I don't think that's true.
> We already know what lexicalEnvironmentRegister holds the scoped "this", we should use that to construct our
> own "this" variable. I would remove this function entirely and do the logic in the function above.
>
> Maybe one way to create the proper scoped "this" variable is to just implement your own walking
> of the m_symbolTableStack. We know for sure that it must be in there, so we should just assert
> that we always find it. That way we ensure any callers of this function only call it if the "this"
> is indeed on the scope stack.
I double checked and I see now that this function is unnecessary. It is covered by variable() function.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3919
>> +void BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment()
>
> nit: I think a better name for this is "emitResolveArrowFunctionContextLexicalEnvironment".
> Also, I think this only ever has to happen once. It should always resolve to the same scope.
OK. Will do, but can we avoid using 'Context' word? Name so long
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3920
>> +{
>
> It's probably good form to assert that we're an arrow function in this function.
For now I'm calling this function in following cases
1) In arrow function
2) In 'eval' in case if 'eval' is called inside of arrow function
3) In constructor during access to 'this', because arrow function can put 'this' back.
So assert has to be ASSERT(m_codeBlock.isArrowFunction() || m_codeBlock.isArrowFunctionContext() || constructorKind() == ConstructorKind::Derived); is it OK?
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:683
>> + bool instantiateLexicalVariables2(const VariableEnvironment&, SymbolTable*, ScopeRegisterType, LookUpVarKindFunctor);
>
> debugging code?
removed
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:831
>> + RegisterID* m_arrowFunctionlexicalEnvRegister { nullptr };
>
> This is a very weird member variable. It means two different things depending on what context we're generating code in.
> I think it'd be clearer to separate this out into two different member variables.
> One member variable should represent the environment that this gets assigned to in "initializeEmptyVarLexicalEnvironmentIfNeeded()" (or whatever you rename it to).
> The other member variable should represent the result of "emitLoadArrowFunctionEnvironment"
>
> Even though these things are similar because they're the environment registers that hold "this"/"new.target"/etc,
> they're not exactly equal because we decide to create one, and we resolve to the other. I think separating them
> is cleaner. It only costs us an extra pointer in memory which is fine inside the BytecodeGenerator.
OK. I'll separate cases.
Created attachment 263943[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 263944[details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 263945[details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 264017[details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 264018[details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 264019[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 264023[details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 264024[details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 264025[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 264022[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=264022&action=review
this is looking better and better. Some comments:
> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
> + unsigned m_isDerivedConstructorContext: 1;
style: "m_isDerivedConstructorContext:" => "m_isDerivedConstructorContext :"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:807
> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironmentIfNeeded(SymbolTable* systemTable)
"systemTable" => "symbolTable"
I also don't like the name "initializeEmptyVarLexicalEnvironmentIfNeeded". It doesn't really mean what you're doing here.
I think a better name would be something along the lines of: "initializeArrowFunctionContextScopeIfNeeded".
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:819
> + systemTable->set(propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
It's worth asserting here that "this" identifier is not in the symbol table already.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:821
> + if (m_codeType == FunctionCode) {
Aren't there more specific situations where we need this? Like if we're in a derived constructor context?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
> + m_arrowFunctionVarLexicalEnvRegister = emitMove(newRegister(), m_scopeRegister);
I think you want "newBlockScopeVariable" instead of "newRegister"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2981
> +void BytecodeGenerator::emitUpdateScopeIfNeeded()
I think a better name for this would be something like:
"emitUpdateArrowFunctionContextScope".
This names seems to general. It would be good to pin it down to being arrowfunction-related.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2996
> +void BytecodeGenerator::emitPutThisToScope()
Again, I would signify in the name that this is related to arrow functions.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3897
> +void BytecodeGenerator::emitGetNewTargeFromArrowFunctionLexicalEnvironment()
typo: "emitGetNewTarge..." => "emitGetNewTarget..."
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:827
> + RegisterID* m_arrowFunctionVarLexicalEnvRegister { nullptr };
nit: I would call this something like: "m_arrowFunctionContextLexicalEnvironmentRegister".
I think "var" is the wrong word here.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
> + RegisterID* m_arrowFunctionResolvedLexicalEnvRegister { nullptr };
style: I would spell out "Environment" in this name, even though it's verbose.
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:699
> +// We need try to load 'this' before call eval, because it can created by 'super' in some of the arrow function
style: Indent this.
Also, I'm a bit confused about this. Do we get the current function's "this" when making a function call?
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:738
> + && (generator.usesArrowFunction() || generator.usesEval())))
style: I'd move this to the above line.
WebKit style says that this should be 4 spaces from the indentation of the "if", which would look bad here. I would just move it one line up.
Comment on attachment 264022[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=264022&action=review>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
>> + unsigned m_isDerivedConstructorContext: 1;
>
> style: "m_isDerivedConstructorContext:" => "m_isDerivedConstructorContext :"
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:807
>> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironmentIfNeeded(SymbolTable* systemTable)
>
> "systemTable" => "symbolTable"
> I also don't like the name "initializeEmptyVarLexicalEnvironmentIfNeeded". It doesn't really mean what you're doing here.
> I think a better name would be something along the lines of: "initializeArrowFunctionContextScopeIfNeeded".
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:821
>> + if (m_codeType == FunctionCode) {
>
> Aren't there more specific situations where we need this? Like if we're in a derived constructor context?
I've decided to include into this patch the implementation of lexically binding of new.target for all cases, because it is required small changes in comparison to patch without it( current condition + condition to load new.target into function context + tests)
new.target is always exist for function and lexically bound to the arrow function
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
>> + m_arrowFunctionVarLexicalEnvRegister = emitMove(newRegister(), m_scopeRegister);
>
> I think you want "newBlockScopeVariable" instead of "newRegister"
fixed
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2981
>> +void BytecodeGenerator::emitUpdateScopeIfNeeded()
>
> I think a better name for this would be something like:
> "emitUpdateArrowFunctionContextScope".
> This names seems to general. It would be good to pin it down to being arrowfunction-related.
OK. I think emitUpdateVariablesInArrowFunctionContextScope will be better, because we change values of the variables in arrow function scope?
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2996
>> +void BytecodeGenerator::emitPutThisToScope()
>
> Again, I would signify in the name that this is related to arrow functions.
Renamed to emitPutThisToArrowFunctionContextScope
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3897
>> +void BytecodeGenerator::emitGetNewTargeFromArrowFunctionLexicalEnvironment()
>
> typo: "emitGetNewTarge..." => "emitGetNewTarget..."
fixed
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:827
>> + RegisterID* m_arrowFunctionVarLexicalEnvRegister { nullptr };
>
> nit: I would call this something like: "m_arrowFunctionContextLexicalEnvironmentRegister".
> I think "var" is the wrong word here.
renamed
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
>> + RegisterID* m_arrowFunctionResolvedLexicalEnvRegister { nullptr };
>
> style: I would spell out "Environment" in this name, even though it's verbose.
Done
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:699
>> +// We need try to load 'this' before call eval, because it can created by 'super' in some of the arrow function
>
> style: Indent this.
> Also, I'm a bit confused about this. Do we get the current function's "this" when making a function call?
I think it has to rewritten -> // We need try to load 'this' before call eval in constructor, because 'this' can created by 'super' in some of the arrow function
It is necessary to cover following case :
var A = class A {
constructor () { this.id = 'A'; }
}
var B = class B extend A {
constructor () {
var arrow = () => super();
arrow();
eval("this.id = 'B'");
}
}
new B();
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:738
>> + && (generator.usesArrowFunction() || generator.usesEval())))
>
> style: I'd move this to the above line.
> WebKit style says that this should be 4 spaces from the indentation of the "if", which would look bad here. I would just move it one line up.
I've changed, now it has two variable and condition in one line:
bool isConstructorKindDerived = generator.constructorKind() == ConstructorKind::Derived;
bool usesArrowFunctionOrEval = generator.usesArrowFunction() || generator.usesEval();
if (generator.isDerivedConstructorContext() || (isConstructorKindDerived && usesArrowFunctionOrEval))
Created attachment 264069[details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 264078[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=264078&action=review
Patch looks mostly good. Just have some comments regarding some tricky problems with the implementation for some edge cases.
> Source/JavaScriptCore/ChangeLog:9
> + 'this' is stored inside of the lexical environment of the function. To store and load are used
nit: "are used" => "we use"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
> + m_arrowFunctionContextLexicalEnvironmentRegister = emitMove(newBlockScopeVariable(), m_scopeRegister);
I was wrong about advising you on this earlier.
I forgot that pushLexicalScopeInternal already creates a variable for us.
Because you have captured variables, we will create a block scoped variable for
the scope.
So all you have to do is this:
m_arrowFunctionContextLexcialEnvironmentRegister = m_symbolTableStack.last().m_scope.
Feel free to add asserts around the pushLexicalScopeInternal call to make sure it actually adds exactly one item to the symbol table stack.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2989
> + if (m_codeType == FunctionCode) {
I realized we don't need this.
We should only need to do this once per function.
If you look how new_target is materialized now, it happens
before we create a "this". So I think it'd be wrong to update
this value over time. It should just happen once at the very beginning of the program.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3881
> +{
There are a few interesting things to consider here that seem like they are problems.
1) consider the problem of using the literal string "__proto__":
class A extends B {
let arr1 = () => {
let __proto__ = "blah";
let arr2 = () => super();
arr2();
}
}
This strategy worked with "this" because no variable can be named "this". But variables can be named "__proto__"
2) what happens when __proto__ is overwritten (I'm not really sure if this is a problem or what's supposed to happen with respect to the specification, but it's probably worth looking into).
class A extends B {
let arr1 = () => { ... }
A.__proto__ = null;
arr1();
}
I wonder what happens just in normal classes with this.
3) If we're storing values in m_calleeRegister, maybe we're doing this wrong. I think this might break stack traces because it will look like an arrow functions parent frame is displayed twice on a stack trace if an error is thrown.
It's worth considering this problem.
For all problems above, if they really are problems, we should have tests for them, too.
Comment on attachment 264078[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=264078&action=review>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
>> + m_arrowFunctionContextLexicalEnvironmentRegister = emitMove(newBlockScopeVariable(), m_scopeRegister);
>
> I was wrong about advising you on this earlier.
> I forgot that pushLexicalScopeInternal already creates a variable for us.
> Because you have captured variables, we will create a block scoped variable for
> the scope.
> So all you have to do is this:
> m_arrowFunctionContextLexcialEnvironmentRegister = m_symbolTableStack.last().m_scope.
> Feel free to add asserts around the pushLexicalScopeInternal call to make sure it actually adds exactly one item to the symbol table stack.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2989
>> + if (m_codeType == FunctionCode) {
>
> I realized we don't need this.
> We should only need to do this once per function.
> If you look how new_target is materialized now, it happens
> before we create a "this". So I think it'd be wrong to update
> this value over time. It should just happen once at the very beginning of the program.
I've split this into two methods
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3881
>> +{
>
> There are a few interesting things to consider here that seem like they are problems.
> 1) consider the problem of using the literal string "__proto__":
> class A extends B {
> let arr1 = () => {
> let __proto__ = "blah";
> let arr2 = () => super();
> arr2();
> }
> }
> This strategy worked with "this" because no variable can be named "this". But variables can be named "__proto__"
>
> 2) what happens when __proto__ is overwritten (I'm not really sure if this is a problem or what's supposed to happen with respect to the specification, but it's probably worth looking into).
> class A extends B {
> let arr1 = () => { ... }
> A.__proto__ = null;
> arr1();
> }
> I wonder what happens just in normal classes with this.
>
> 3) If we're storing values in m_calleeRegister, maybe we're doing this wrong. I think this might break stack traces because it will look like an arrow functions parent frame is displayed twice on a stack trace if an error is thrown.
> It's worth considering this problem.
>
> For all problems above, if they really are problems, we should have tests for them, too.
Added additional tests to arrowfunction-lexical-bind-supercall-2.js module for cases that you mention.
Stack trace looks correct
for instance for following code:
var errorStack;
var ParentClass = class ParentClass {
constructor() {
try {
this.idValue = 'testValue';
throw new Error('Error');
} catch (e) {
errorStack = e.stack;
}
}
};
var ChildClass = class ChildClass extends ParentClass {
constructor () {
var arrowInChildConstructor = () => {
var nestedArrow = () => {
super();
}
nestedArrow();
};
arrowInChildConstructor();
}
};
Stack trace is:
ParentClass@test_class_14.js:9:22
nestedArrow@test_class_14.js:20:14
arrowInChildConstructor@test_class_14.js:23:18
ChildClass@test_class_14.js:26:28
global code@test_class_14.js:31:25
Created attachment 264606[details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 264607[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 264622[details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 264685[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=264685&action=review> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:831
> + auto addTarget = environment.add(propertyNames().target);
This should be the private name as well, right?
It might be good to have tests that this code
would fail using the public identifier.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:837
> + auto protoObject = environment.add(propertyNames().underscoreProtoScopeLocalPrivateName);
Instead of "underscoreProtoScopeLocalPrivateName" we can maybe give this a more descriptive name.
This is just the current class we're in super class, right?
like:
class A extends B { constructor() { let arr = () => { ... } }
this will be "B", right?
Maybe we can just name this using some form of "super class", then?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3843
> + // 'this' can be uninitialized in constructor of derived class so we resolve with underscoreProtoScope variable that stored in arrow function lexical environment in of such cases
Even though this is uninitialized, don't we always store it in the symbol table?
So if we read from the scope, we might get jsTDZValue(), but since we're just
resolving the scope, shouldn't "this" always resolve to the proper scope?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3892
> + emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);
Why can't we always use "m_arrowFunctionContextLexicalEnvironmentRegister" here?
We could always call "emitLoadArrowFunctionContextScope()", too, just to make sure we've resolved to it.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:296
> + Variable variable(const Identifier&, bool = true);
I think an "enum class" would be better than bool here, that way all call sites of variable() are more descriptive.
maybe: "enum class ThisResolutionType { Local, Scoped };"
or something like that.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
> + RefPtr<RegisterID> m_arrowFunctionResolvedLexicalEnvRegister { nullptr };
I would make "env" to "environment" here, or maybe rename to something shorter like:
"m_resolvedArrowFunctionScopeContext"
> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:-1024
> - exactRead = ArrowFunctionBoundThisPLoc;
I think we should remove the definition and other code that looks at "ArrowFunctionBoundThisPLoc" since it's no longer used.
Created attachment 264970[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265000[details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 264685[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=264685&action=review
I've uploaded new patch. Something wrong with mac-debug tests . They are always fails with different errors.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:831
>> + auto addTarget = environment.add(propertyNames().target);
>
> This should be the private name as well, right?
>
> It might be good to have tests that this code
> would fail using the public identifier.
I've added tests to check if variable visible in scope
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:837
>> + auto protoObject = environment.add(propertyNames().underscoreProtoScopeLocalPrivateName);
>
> Instead of "underscoreProtoScopeLocalPrivateName" we can maybe give this a more descriptive name.
> This is just the current class we're in super class, right?
> like:
> class A extends B { constructor() { let arr = () => { ... } }
> this will be "B", right?
> Maybe we can just name this using some form of "super class", then?
Renamed to superClassScope
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3843
>> + // 'this' can be uninitialized in constructor of derived class so we resolve with underscoreProtoScope variable that stored in arrow function lexical environment in of such cases
>
> Even though this is uninitialized, don't we always store it in the symbol table?
> So if we read from the scope, we might get jsTDZValue(), but since we're just
> resolving the scope, shouldn't "this" always resolve to the proper scope?
Fixed
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3892
>> + emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);
>
> Why can't we always use "m_arrowFunctionContextLexicalEnvironmentRegister" here?
> We could always call "emitLoadArrowFunctionContextScope()", too, just to make sure we've resolved to it.
Done.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:296
>> + Variable variable(const Identifier&, bool = true);
>
> I think an "enum class" would be better than bool here, that way all call sites of variable() are more descriptive.
> maybe: "enum class ThisResolutionType { Local, Scoped };"
> or something like that.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
>> + RefPtr<RegisterID> m_arrowFunctionResolvedLexicalEnvRegister { nullptr };
>
> I would make "env" to "environment" here, or maybe rename to something shorter like:
> "m_resolvedArrowFunctionScopeContext"
Done
>> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:-1024
>> - exactRead = ArrowFunctionBoundThisPLoc;
>
> I think we should remove the definition and other code that looks at "ArrowFunctionBoundThisPLoc" since it's no longer used.
Removed
Comment on attachment 265001[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=265001&action=review
This patch is just about done. It's in really good shape. I just have a few final comments and then I think it's ready to land.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:833
> + addResult.iterator->value.setIsConst();
Why is "this" modeled as a "const" variable?
I think it's better to just model them as normal variables because
we write to them more than once. Modeling them as
"const" will probably make the put_to_scope code fail when
it goes down the slow path. Unless we always make sure the put_to_scope
passes in the Initialize flag.
It's worth having tests to ensure the code works going
down the slow path. One way to write such a test is to
make the eval var injection watchpoint fire. I think something like this should make
the "this"/"new.target" go down the slow path (you should verify):
```
class C {
constructor() {
eval("var x = 20");
super();
let f = () => this;
}
}
```
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3863
> +
nit: only one newline is needed.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3876
> + emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, newTargetVar, newTarget(), DoNotThrowIfNotFound, NotInitialization);
This should be "Initialization". As I commented above, I think this code would fail if it went down the slow path. We should ensure it works going down the slow path of put_to_scope.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3885
> + emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, protoScope, &m_calleeRegister, DoNotThrowIfNotFound, NotInitialization);
Ditto: should be "Initialization".
Also, I was totally wrong about naming this "super...", this is really the derived constructor that we're storing in the environemnt
and then later loading __proto__ on. We should name it like "derivedConstructorPrivateName" or "derivedClassPrivateName". Sorry about that.
I had thought we were eagerly storing the __proto__ of the derived constructor.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3894
> + emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);
I think it's arguable whether or not this should be "Initialization" or "NotInitialization". I'd say it should be "Initialization" even though it may execute more than once.
Either way, I think the "this" variable probably shouldn't be marked as being like "const".
Also, we don't want to have more than one op_resolve_scope here because it will always resolve to the same thing. I'm not sure if this code
will run more than once unless we call "super()" more than once in a constructor. This seems really unlikely in real code (but I think it's legal in ES6 to do so),
so it's cleaner to ensure we never emit op_resolve_scope unnecessarily by doing something like this:
if (isDerivedConstructorContext())
emitLoadArrowFunctionLexicalEnvironment()
emitPutToScope(isDerivedConstructorContext() ? m_resolvedArrowFunctionScopeContextRegister.get() : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, Initialization);
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:830
> + RefPtr<RegisterID> m_resolvedArrowFunctionScopeContextRegister { nullptr };
you don't need the nullptr initializer here, RefPtrs are by default initialized to null.
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3003
> + bool usesArrowOrEval = generator.usesArrowFunction() || generator.usesEval();
There are a lot of places where you do usesArrow() || usesEval(), I wonder if it's worth giving this
condition a more descriptive name in the various bytecodegenerator constructors.
Maybe like "m_needsToUpdateArrowFunctionContext(generator.usesArrowFunction() || generator.usesEval())"
Just a thought, I'm also okay with having this condition tested at all the interesting places.
> Source/JavaScriptCore/runtime/Executable.h:348
> + bool isArrowFunctionContext() const { return m_isArrowFunctionContext; }
Could we get rid of these properties (not these methods) and just ask the unlinked code block for this data or get at it through CodeFeatures?
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:1
> +var testCase = function (actual, expected, message) {
style: Let's make this file 4-space indented throughout
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:13
> +for (var i=0; i<10000; i++) {
I think we can get away w/ 1000 iterations for all the loops in this test.
10000 seems overkill.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:27
> + passed = new.target === B;
Shouldn't this be '&=' and the below just be "=" since below "passed &= new.target === B" is executed first?
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:50
> + passed &= new.target === B;
Why would this be "B"?
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:75
> +
I take my previous comment back, I don't think we really need a test for this, it's just confusing.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:1
> +var testCase = function (actual, expected, message) {
style: You should make all your test files have a consistent 4-space indent.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:16
> + constructor (inArrowFuction, inConstructor, repeatInArrowFunction) {
"repeatInArrowFunction" is unused, maybe remove it or were you planning on calling super() twice?
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:35
> +for (var i=0; i < 10000; i++) {
I think 1000 iterations is also good for tests in this file.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:17
> + var arrow = () => () => () => {
👍
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:34
> +for (var i=0; i < 10000; i++) {
1000 iterations is probably good for this file too.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:39
> +var testException = function (value1, value2, value3, index) {
value3 is unused.
I would make this function take only the index parameter
because value1 and value2 are always false. It's easier
to just pass in "false" yourself
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:95
> + E.__proto__ = function () {};
Might be worth having a test that sets __proto__ to "null" and make sure that we throw an error.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:31
> +for (var i=0; i < 10000; i++) {
I think all your tests can just be 1000 iterations.
> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:35
> +// Fixme: Failed test with 'Segmentation fault: 11' error in release mode in 6 cases from 14. List of failed case:
Is this fixed?
I renamed the bug to better indicate what the patch is doing.
How is performance on this bug? Can you run the perf tests
just to make sure we haven't regressed on things non-arrow function related.
We may have regressed slightly on some arrow-function tests, but that's expected. Also, this will get better once you implement some basic static analysis for "this" usage so we don't store "this" in the environment unless we really need to.
(In reply to comment #73)
> I renamed the bug to better indicate what the patch is doing.
>
> How is performance on this bug? Can you run the perf tests
> just to make sure we haven't regressed on things non-arrow function related.
> We may have regressed slightly on some arrow-function tests, but that's
> expected. Also, this will get better once you implement some basic static
> analysis for "this" usage so we don't store "this" in the environment unless
> we really need to.
Thanks for the review, hope I soon land patch with fixes. I'll provide the perf test result with patch
(In reply to comment #73)
> I renamed the bug to better indicate what the patch is doing.
>
> How is performance on this bug? Can you run the perf tests
> just to make sure we haven't regressed on things non-arrow function related.
> We may have regressed slightly on some arrow-function tests, but that's
> expected. Also, this will get better once you implement some basic static
> analysis for "this" usage so we don't store "this" in the environment unless
> we really need to.
See result by link https://bugs.webkit.org/attachment.cgi?id=265179, Results are neutral except following tests:
arrowfunction-call 95.9133+-5.5082 ! 338.3422+-14.9020 ! definitely 3.5276x slower
and small performance degradation:
function-with-eval 708.1514+-8.0792 ! 748.3060+-10.7312 ! definitely 1.0567x slower
is-object-or-null-tricky-internal-function
59.8600+-1.4660 ! 74.9355+-10.1182 ! definitely 1.2518x slower
setter 50.4615+-2.1227 ! 64.9183+-12.1934 ! definitely 1.2865x slower
(In reply to comment #76)
> (In reply to comment #73)
> > I renamed the bug to better indicate what the patch is doing.
> >
> > How is performance on this bug? Can you run the perf tests
> > just to make sure we haven't regressed on things non-arrow function related.
> > We may have regressed slightly on some arrow-function tests, but that's
> > expected. Also, this will get better once you implement some basic static
> > analysis for "this" usage so we don't store "this" in the environment unless
> > we really need to.
>
> See result by link https://bugs.webkit.org/attachment.cgi?id=265179, Results
> are neutral except following tests:
> arrowfunction-call 95.9133+-5.5082 !
> 338.3422+-14.9020 ! definitely 3.5276x slower
>
> and small performance degradation:
> function-with-eval 708.1514+-8.0792 !
> 748.3060+-10.7312 ! definitely 1.0567x slower
> is-object-or-null-tricky-internal-function
> 59.8600+-1.4660 !
> 74.9355+-10.1182 ! definitely 1.2518x slower
> setter 50.4615+-2.1227 !
> 64.9183+-12.1934 ! definitely 1.2865x slower
These are expected.
Eval is slower b/c we must be pessimistic and assume that it could have an arrow function.
The arrowfunction-call is a tiny micro-benchmark. It will get back to normal speed once you implement the patch that statically analyzes an arrow function to see if it really needs the lexical environment created for it.
What happens when we call super once we've exited the constructor function?
Is there anything in the spec on this? Does is mutate "this"?
Like:
```
class C extends B {
constructor() {
this.weird = () => super();
super();
}
foo() {
this.weird();
}
}
(new C).foo();
```
(In reply to comment #78)
> What happens when we call super once we've exited the constructor function?
> Is there anything in the spec on this? Does is mutate "this"?
>
> Like:
> ```
> class C extends B {
> constructor() {
> this.weird = () => super();
> super();
> }
> foo() {
> this.weird();
> }
> }
>
> (new C).foo();
> ```
I've not checked this case, but I'm sure that we can't run super() twice in constructor, second call should lead to RuntimeException. See tred on es6-discuss https://esdiscuss.org/topic/duplicate-super-call-behaviour.
But this behavior is not related to the arrow function.
(In reply to comment #79)
> (In reply to comment #78)
> > What happens when we call super once we've exited the constructor function?
> > Is there anything in the spec on this? Does is mutate "this"?
> >
> > Like:
> > ```
> > class C extends B {
> > constructor() {
> > this.weird = () => super();
> > super();
> > }
> > foo() {
> > this.weird();
> > }
> > }
> >
> > (new C).foo();
> > ```
>
> I've not checked this case, but I'm sure that we can't run super() twice in
> constructor, second call should lead to RuntimeException. See tred on
> es6-discuss https://esdiscuss.org/topic/duplicate-super-call-behaviour.
> But this behavior is not related to the arrow function.
Interesting. We currently don't throw on a second call to super().
That's bad, we should file a bug.
(In reply to comment #80)
> (In reply to comment #79)
> > (In reply to comment #78)
> > > What happens when we call super once we've exited the constructor function?
> > > Is there anything in the spec on this? Does is mutate "this"?
> > >
> > > Like:
> > > ```
> > > class C extends B {
> > > constructor() {
> > > this.weird = () => super();
> > > super();
> > > }
> > > foo() {
> > > this.weird();
> > > }
> > > }
> > >
> > > (new C).foo();
> > > ```
> >
> > I've not checked this case, but I'm sure that we can't run super() twice in
> > constructor, second call should lead to RuntimeException. See tred on
> > es6-discuss https://esdiscuss.org/topic/duplicate-super-call-behaviour.
> > But this behavior is not related to the arrow function.
> Interesting. We currently don't throw on a second call to super().
> That's bad, we should file a bug.
I created this bug for this problem:
https://bugs.webkit.org/show_bug.cgi?id=151113
Created attachment 265303[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 265305[details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265001[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=265001&action=review>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:833
>> + addResult.iterator->value.setIsConst();
>
> Why is "this" modeled as a "const" variable?
> I think it's better to just model them as normal variables because
> we write to them more than once. Modeling them as
> "const" will probably make the put_to_scope code fail when
> it goes down the slow path. Unless we always make sure the put_to_scope
> passes in the Initialize flag.
>
> It's worth having tests to ensure the code works going
> down the slow path. One way to write such a test is to
> make the eval var injection watchpoint fire. I think something like this should make
> the "this"/"new.target" go down the slow path (you should verify):
> ```
> class C {
> constructor() {
> eval("var x = 20");
> super();
> let f = () => this;
> }
> }
> ```
I've changed to 'let' variable type. I've added one test case to cover going down the slow path.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3863
>> +
>
> nit: only one newline is needed.
Removed
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3876
>> + emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, newTargetVar, newTarget(), DoNotThrowIfNotFound, NotInitialization);
>
> This should be "Initialization". As I commented above, I think this code would fail if it went down the slow path. We should ensure it works going down the slow path of put_to_scope.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3885
>> + emitPutToScope(m_arrowFunctionContextLexicalEnvironmentRegister, protoScope, &m_calleeRegister, DoNotThrowIfNotFound, NotInitialization);
>
> Ditto: should be "Initialization".
> Also, I was totally wrong about naming this "super...", this is really the derived constructor that we're storing in the environemnt
> and then later loading __proto__ on. We should name it like "derivedConstructorPrivateName" or "derivedClassPrivateName". Sorry about that.
> I had thought we were eagerly storing the __proto__ of the derived constructor.
NP. I've changed name to derivedConstructorPrivateName
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3894
>> + emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);
>
> I think it's arguable whether or not this should be "Initialization" or "NotInitialization". I'd say it should be "Initialization" even though it may execute more than once.
> Either way, I think the "this" variable probably shouldn't be marked as being like "const".
>
> Also, we don't want to have more than one op_resolve_scope here because it will always resolve to the same thing. I'm not sure if this code
> will run more than once unless we call "super()" more than once in a constructor. This seems really unlikely in real code (but I think it's legal in ES6 to do so),
> so it's cleaner to ensure we never emit op_resolve_scope unnecessarily by doing something like this:
>
> if (isDerivedConstructorContext())
> emitLoadArrowFunctionLexicalEnvironment()
> emitPutToScope(isDerivedConstructorContext() ? m_resolvedArrowFunctionScopeContextRegister.get() : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, Initialization);
Changed const to 'let', and used your snipped.
I'm sure that we can't run super() twice in constructor, second call should lead to RuntimeException. See tred on es6-discuss https://esdiscuss.org/topic/duplicate-super-call-behaviour.
But this behavior is not related to the arrow function.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:830
>> + RefPtr<RegisterID> m_resolvedArrowFunctionScopeContextRegister { nullptr };
>
> you don't need the nullptr initializer here, RefPtrs are by default initialized to null.
Done
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3003
>> + bool usesArrowOrEval = generator.usesArrowFunction() || generator.usesEval();
>
> There are a lot of places where you do usesArrow() || usesEval(), I wonder if it's worth giving this
> condition a more descriptive name in the various bytecodegenerator constructors.
> Maybe like "m_needsToUpdateArrowFunctionContext(generator.usesArrowFunction() || generator.usesEval())"
> Just a thought, I'm also okay with having this condition tested at all the interesting places.
I've created property needsToUpdateArrowFunctionContext in generator, and now it is used in several placed
>> Source/JavaScriptCore/runtime/Executable.h:348
>> + bool isArrowFunctionContext() const { return m_isArrowFunctionContext; }
>
> Could we get rid of these properties (not these methods) and just ask the unlinked code block for this data or get at it through CodeFeatures?
I've added CodeFeature. To force it works I made small 'trick'. Please take a look if it look ok.
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:1
>> +var testCase = function (actual, expected, message) {
>
> style: Let's make this file 4-space indented throughout
Done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:13
>> +for (var i=0; i<10000; i++) {
>
> I think we can get away w/ 1000 iterations for all the loops in this test.
> 10000 seems overkill.
Done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:27
>> + passed = new.target === B;
>
> Shouldn't this be '&=' and the below just be "=" since below "passed &= new.target === B" is executed first?
Ohh, my fault.
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-newtarget.js:50
>> + passed &= new.target === B;
>
> Why would this be "B"?
Removed this condition
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:1
>> +var testCase = function (actual, expected, message) {
>
> style: You should make all your test files have a consistent 4-space indent.
Done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:16
>> + constructor (inArrowFuction, inConstructor, repeatInArrowFunction) {
>
> "repeatInArrowFunction" is unused, maybe remove it or were you planning on calling super() twice?
I've removed. I thought about this, but I'm going to call super() twice in another bug.
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-1.js:35
>> +for (var i=0; i < 10000; i++) {
>
> I think 1000 iterations is also good for tests in this file.
done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:34
>> +for (var i=0; i < 10000; i++) {
>
> 1000 iterations is probably good for this file too.
done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:39
>> +var testException = function (value1, value2, value3, index) {
>
> value3 is unused.
> I would make this function take only the index parameter
> because value1 and value2 are always false. It's easier
> to just pass in "false" yourself
Done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-2.js:95
>> + E.__proto__ = function () {};
>
> Might be worth having a test that sets __proto__ to "null" and make sure that we throw an error.
Added new tests
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:31
>> +for (var i=0; i < 10000; i++) {
>
> I think all your tests can just be 1000 iterations.
Done
>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-supercall-3.js:35
>> +// Fixme: Failed test with 'Segmentation fault: 11' error in release mode in 6 cases from 14. List of failed case:
>
> Is this fixed?
Now it works, I'll rollback soon to check this error again.
Created attachment 265316[details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265370[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 265375[details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 265378[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265544[details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265543[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=265543&action=review> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:846
> + auto protoObject = environment.add(propertyNames().derivedConstructorPrivateName);
Nit: I would call this variable a different name after your renaming.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3857
> + if (m_resolvedArrowFunctionScopeContextRegister == nullptr)
Style: I think it's official WebKit style to check "!m_resolved..." Instead of "m_resolved... == nullptr"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3863
> +void BytecodeGenerator::emitGetThisFromArrowFunctionLexicalEnvironment()
Style: I would call this "emitLoad..." Instead of "emitGet..."
And the same below
> Source/JavaScriptCore/runtime/Executable.cpp:137
> + , m_features((isInStrictContext ? StrictModeFeature : 0) | (isInArrowFunctionContext ? ArrowFunctionContextFeature : 0))
This seems awkward, actually. I'm not sure why we just have
the features mean one thing here. I think it's probably better just
to have a separate property like you had before.
Created attachment 265575[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265643[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265737[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265735[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=265735&action=review
Some drive by style comments.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1884
> +
Whitespace!
> Source/JavaScriptCore/runtime/CommonIdentifiers.h:333
> + macro(newTargetLocal)\
> + macro(derivedConstructor)\
Style: Seems the normal style is to have a space after the macro.
> Source/JavaScriptCore/runtime/Executable.h:398
> + ScriptExecutable(Structure*, VM&, const SourceCode&, bool, bool, bool);
These bools are rather mysterious. When it is non-obvious the header should normally include the name. If someone only had the header they should understand what these bools mean!
> Source/JavaScriptCore/runtime/JSGlobalObject.h:664
> + UnlinkedEvalCodeBlock* createEvalCodeBlock(CallFrame*, EvalExecutable*, ThisTDZMode, bool, const VariableEnvironment*);
Same, this bool should really have a name.
Attachment 265865[details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/Executable.h:398: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Executable.h:398: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Executable.h:398: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 3 in 50 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 265881[details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Attachment 265903[details] did not pass style-queue:
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4547: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4549: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 2 in 53 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 265911[details]
Archive of layout-test-results from ews100 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265917[details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265903[details]
Patch
I've managed to repeat failed test on my local pc with Tools/Scripts/run-webkit-tests --debug inspector/heap --iterations=20, then VM was running.
I'll try to find what is wrong with my patch
Comment on attachment 266216[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=266216&action=review> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:132
> + UnlinkedFunctionExecutable(VM*, Structure*, const SourceCode&, RefPtr<SourceProvider>&& sourceOverride, FunctionMetadataNode*, UnlinkedFunctionKind, ConstructAbility, VariableEnvironment&, bool);
Style: Can we name this bool in a follow up patch.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2590
> + instructions().append(0);
Lets remove this argument in a follow up patch
> Source/JavaScriptCore/runtime/Executable.h:463
> + EvalExecutable(ExecState*, const SourceCode&, bool, bool, bool);
style: Lets name these bools in a follow up patch.
Comment on attachment 266724[details]
Patch
1. Fixed bug in GTK+
2. Merged with changes that are related to the ES6 Generators. Possible some need some refactoring to use reuse properties created for ES6 Generators. I would prefer make this refactoring in separate patch.
3. Can't check results of tests in mac-debug build bot, because tests fails before this patch.
Comment on attachment 266850[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=266850&action=review> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4572
> + if (opcodeID == op_new_func_exp) {
> + // Curly braces are necessary
> + NEXT_OPCODE(op_new_func_exp);
> + } else {
> + // Curly braces are necessary
> + NEXT_OPCODE(op_new_arrow_func_exp);
> + }
Why not just "NEXT_OPCODE(opcodeID)" ?
> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2288
> +
> +
Revert this please.
> Source/JavaScriptCore/runtime/Executable.h:659
> + // TODO:Think about avoid using isArrowFunction veriabl
Style: I don't think we need a variable.
WebKit style handles this is with a FIXME and not a TODO.
Also, the best FIXMEs are ones with bug numbers.
Anyways, I think for this patch this can just be removed.
Comment on attachment 266850[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=266850&action=review>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4572
>> + }
>
> Why not just "NEXT_OPCODE(opcodeID)" ?
Hmm, I couldn't build without curly braces. I think because NEXT_OPCODE is two line macros, and this leads to syntax error
#define NEXT_OPCODE(name) \
m_currentIndex += OPCODE_LENGTH(name); \
continue
>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2288
>> +
>
> Revert this please.
Done
>> Source/JavaScriptCore/runtime/Executable.h:659
>> + // TODO:Think about avoid using isArrowFunction veriabl
>
> Style: I don't think we need a variable.
> WebKit style handles this is with a FIXME and not a TODO.
> Also, the best FIXMEs are ones with bug numbers.
> Anyways, I think for this patch this can just be removed.
Removed
(In reply to comment #155)
> (In reply to comment #154)
> > Did anyone run performance tests on this?
>
> https://bugs.webkit.org/attachment.cgi?id=265179
That's in debug mode. Debug mode performance numbers are not meaningful.
We should make sure to do release mode performance testing of patches like this.
(In reply to comment #156)
> (In reply to comment #155)
> > (In reply to comment #154)
> > > Did anyone run performance tests on this?
> >
> > https://bugs.webkit.org/attachment.cgi?id=265179
>
> That's in debug mode. Debug mode performance numbers are not meaningful.
>
> We should make sure to do release mode performance testing of patches like
> this.
I didn't run the tests so I don't know for sure but the results text result looks like a release build. What made you say it was Debug?
(In reply to comment #157)
> (In reply to comment #156)
> > (In reply to comment #155)
> > > (In reply to comment #154)
> > > > Did anyone run performance tests on this?
> > >
> > > https://bugs.webkit.org/attachment.cgi?id=265179
> >
> > That's in debug mode. Debug mode performance numbers are not meaningful.
> >
> > We should make sure to do release mode performance testing of patches like
> > this.
>
> I didn't run the tests so I don't know for sure but the results text result
> looks like a release build. What made you say it was Debug?
The performance is very bad in absolute terms.
I also just noticed that the performance numbers are super noisy.
Either way, someone needs to run independent performance numbers on this because these don't look right.
(In reply to comment #158)
> (In reply to comment #157)
> > (In reply to comment #156)
> > > (In reply to comment #155)
> > > > (In reply to comment #154)
> > > > > Did anyone run performance tests on this?
> > > >
> > > > https://bugs.webkit.org/attachment.cgi?id=265179
> > >
> > > That's in debug mode. Debug mode performance numbers are not meaningful.
> > >
> > > We should make sure to do release mode performance testing of patches like
> > > this.
> >
> > I didn't run the tests so I don't know for sure but the results text result
> > looks like a release build. What made you say it was Debug?
>
> The performance is very bad in absolute terms.
>
> I also just noticed that the performance numbers are super noisy.
>
> Either way, someone needs to run independent performance numbers on this
> because these don't look right.
I'll run them tonight.
(In reply to comment #159)
> (In reply to comment #158)
> > (In reply to comment #157)
> > > (In reply to comment #156)
> > > > (In reply to comment #155)
> > > > > (In reply to comment #154)
> > > > > > Did anyone run performance tests on this?
> > > > >
> > > > > https://bugs.webkit.org/attachment.cgi?id=265179
> > > >
> > > > That's in debug mode. Debug mode performance numbers are not meaningful.
> > > >
> > > > We should make sure to do release mode performance testing of patches like
> > > > this.
> > >
> > > I didn't run the tests so I don't know for sure but the results text result
> > > looks like a release build. What made you say it was Debug?
> >
> > The performance is very bad in absolute terms.
> >
> > I also just noticed that the performance numbers are super noisy.
> >
> > Either way, someone needs to run independent performance numbers on this
> > because these don't look right.
>
> I'll run them tonight.
When measuring perf in Linux, I can see these noisy result.
On the other hand, when executing it in OSX with exactly the same run-jsc-benchmarks cmd,
I can get somewhat solid result. So usually I measure perf on OSX machine.
(In Linux, I ensured that cpu freq is fixed as "performance" mode, not "ondemand")
I guess that is due to FTL's system allocator... But anyway, retaking it in OSX is nice.
(In reply to comment #160)
> (In reply to comment #159)
> > (In reply to comment #158)
> > > (In reply to comment #157)
> > > > (In reply to comment #156)
> > > > > (In reply to comment #155)
> > > > > > (In reply to comment #154)
> > > > > > > Did anyone run performance tests on this?
> > > > > >
> > > > > > https://bugs.webkit.org/attachment.cgi?id=265179
> > > > >
> > > > > That's in debug mode. Debug mode performance numbers are not meaningful.
> > > > >
> > > > > We should make sure to do release mode performance testing of patches like
> > > > > this.
> > > >
> > > > I didn't run the tests so I don't know for sure but the results text result
> > > > looks like a release build. What made you say it was Debug?
> > >
> > > The performance is very bad in absolute terms.
> > >
> > > I also just noticed that the performance numbers are super noisy.
> > >
> > > Either way, someone needs to run independent performance numbers on this
> > > because these don't look right.
> >
> > I'll run them tonight.
>
> When measuring perf in Linux, I can see these noisy result.
> On the other hand, when executing it in OSX with exactly the same
> run-jsc-benchmarks cmd,
> I can get somewhat solid result. So usually I measure perf on OSX machine.
> (In Linux, I ensured that cpu freq is fixed as "performance" mode, not
> "ondemand")
> I guess that is due to FTL's system allocator... But anyway, retaking it in
> OSX is nice.
I've run benchmarks on Linux, and I don't see it being this slow.
(In reply to comment #164)
>
> I've run benchmarks on Linux, and I don't see it being this slow.https://gist.github.com/Constellation/a5cd0256bde09b5d2c68
<arithmetic> 6.7587+-0.2600 ? 6.8858+-0.3450 ? might be 1.0188x slower
Like this... on simple Ubuntu 14.04 desktop with closing all apps. (but it's not related to this issue anyway ;) )
(In reply to comment #167)
> Here are two different runs of sunspider and one run of longspider.
> Looks OK to me. What do you think Fil?
>
...
The two sunspider runs look very noisy. The longspider looks fine.
2015-10-02 06:53 PDT, GSkachkov
2015-10-04 02:47 PDT, GSkachkov
2015-10-05 07:56 PDT, GSkachkov
2015-10-05 08:37 PDT, Build Bot
2015-10-05 08:47 PDT, Build Bot
2015-10-06 12:54 PDT, GSkachkov
2015-10-20 14:10 PDT, GSkachkov
2015-10-23 13:21 PDT, GSkachkov
2015-10-23 14:06 PDT, Build Bot
2015-10-23 14:15 PDT, Build Bot
2015-10-23 14:26 PDT, Build Bot
2015-10-25 06:19 PDT, GSkachkov
2015-10-25 10:43 PDT, GSkachkov
2015-10-25 11:31 PDT, Build Bot
2015-10-25 11:37 PDT, Build Bot
2015-10-25 11:38 PDT, Build Bot
2015-10-25 13:29 PDT, GSkachkov
2015-10-25 14:16 PDT, Build Bot
2015-10-25 14:21 PDT, Build Bot
2015-10-25 14:29 PDT, Build Bot
2015-10-26 11:42 PDT, GSkachkov
2015-10-26 13:25 PDT, Build Bot
2015-10-26 14:57 PDT, GSkachkov
2015-11-02 09:26 PST, GSkachkov
2015-11-02 10:33 PST, GSkachkov
2015-11-02 11:45 PST, Build Bot
2015-11-02 11:49 PST, Build Bot
2015-11-02 12:14 PST, GSkachkov
2015-11-02 13:45 PST, Build Bot
2015-11-03 03:44 PST, GSkachkov
2015-11-06 12:24 PST, GSkachkov
2015-11-06 16:06 PST, Build Bot
2015-11-07 02:11 PST, GSkachkov
2015-11-07 02:33 PST, GSkachkov
2015-11-07 04:15 PST, Build Bot
2015-11-07 06:58 PST, GSkachkov
2015-11-10 08:11 PST, GSkachkov
2015-11-10 14:20 PST, GSkachkov
2015-11-11 08:10 PST, GSkachkov
2015-11-11 09:03 PST, GSkachkov
2015-11-11 09:32 PST, GSkachkov
2015-11-11 10:31 PST, Build Bot
2015-11-11 10:43 PST, Build Bot
2015-11-11 11:22 PST, GSkachkov
2015-11-11 13:49 PST, Build Bot
2015-11-11 23:37 PST, GSkachkov
2015-11-12 01:05 PST, Build Bot
2015-11-12 02:16 PST, GSkachkov
2015-11-12 03:14 PST, Build Bot
2015-11-12 03:23 PST, Build Bot
2015-11-14 09:20 PST, GSkachkov
2015-11-14 11:04 PST, Build Bot
2015-11-16 00:20 PST, GSkachkov
2015-11-16 01:23 PST, Build Bot
2015-11-16 14:40 PST, GSkachkov
2015-11-16 17:22 PST, Build Bot
2015-11-17 23:21 PST, GSkachkov
2015-11-18 00:24 PST, Build Bot
2015-11-19 09:33 PST, GSkachkov
2015-11-19 10:14 PST, GSkachkov
2015-11-19 12:16 PST, Build Bot
2015-11-19 12:52 PST, GSkachkov
2015-11-19 14:27 PST, GSkachkov
2015-11-19 15:25 PST, Build Bot
2015-11-19 16:35 PST, Build Bot
2015-11-28 17:01 PST, GSkachkov
2015-11-28 17:14 PST, GSkachkov
2015-12-01 00:23 PST, GSkachkov
2015-12-05 15:06 PST, GSkachkov
2015-12-05 15:36 PST, GSkachkov
2015-12-08 00:12 PST, GSkachkov
2015-12-08 10:23 PST, GSkachkov