Fix default parameter evaluation ordering for generators #2176
Conversation
| */ | ||
| public static void evaluateGeneratorDefaultParams( | ||
| JSFunction funObj, Scriptable activationScope) { | ||
| Context cx = Context.getCurrentContext(); |
There was a problem hiding this comment.
Why not providing the context as param? Looks like you have the context already in hand when calling this
| } | ||
| } | ||
|
|
||
| public static Scriptable createNativeGenerator( |
There was a problem hiding this comment.
Same here regarding the context, i think this is a more or less internal function, we cahn change the params without having to take care of backward compatibility. Again we can provide the context from the outside.
| String[] generatorDefaultParams = | ||
| jsFunc.getDescriptor().getGeneratorDefaultParams(); | ||
| if (generatorDefaultParams != null && generatorDefaultParams.length > 0) { | ||
| for (int i = 0; i < generatorDefaultParams.length; i += 2) { |
There was a problem hiding this comment.
This seems to be a copy of wha we are doing in the opt runtime in this case. Maybe we have to have it only once
There was a problem hiding this comment.
Yeah, will refactor it out to ScriptRuntime and just call it from the interpreter. It's the exact same code. I intended to do that and forgot. Thanks for calling it out!
| public static Scriptable createNativeGenerator( | ||
| JSFunction funObj, Scriptable scope, Scriptable thisObj, int maxLocals, int maxStack) { | ||
| Context cx = Context.getCurrentContext(); | ||
| JSFunction funObj, |
There was a problem hiding this comment.
again see the comments on the other pr
andreabergia
left a comment
There was a problem hiding this comment.
If I understand this correctly, for:
function *f(a = 1) { yield 2; }what you are doing is storing in the JSDescriptor for f an array of string which contains ["a", "1"] and then you evaluate the string with 1 at runtime.
This feels really horrible for performances.
Why aren't we generating some code that does:
function *f(a) {
{
var a = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : 'dEFUALT';
}
yield 2;
}like babel or swc do? I get that this is pretty complex for generators, given that they have their state, but dynamically eval-ing strings at every single call doesn't seem good to me.
|
I agree that evaluating at runtime is pretty terrible for perf. However, I think, we cannot use Babel's approach of putting the initialization inside the generator body. According to spec, EvaluateGeneratorBody, generator evaluation must:
Default parameter evaluation happens in FunctionDeclarationInstantiation,
There's a corresponding 262 case. For example: function* g(a = (g.prototype = null)) {}
var oldProto = g.prototype;
var it = g();
Object.getPrototypeOf(it) === oldProto; // Should be FALSE per specThe default param mutates While It appears that v8 fixed this reasonably recently (Apr 2025) and their rationale seems to be coherent with the above. Given that we need to evaluate the arguments at runtime, to improve the performance perhaps I can store the |
8534f77 to
137bd8a
Compare
|
I've changed the implementation to just generate the bytecode directly instead of storing the source for expressions evaluation for defaults, so for the 262 test case above: function* g(a = (g.prototype = null)) {}
var oldProto = g.prototype;
var it = g();
Object.getPrototypeOf(it) === oldProto; // => falseand for compiled code: |
|
This follows what |
This looks way better! Thanks for doing it! |
|
Closing this as #2173 which was stacked on this is already merged. |
Fixes a few things in generators for spec compliance:
In FunctionDeclarationInstatiation, the ordering is
This is called from EvaluateGeneratorBody.
generatorProto.From InstantiateGeneratorFunctionExpression