Bug 150792 – [ES6] Implement LLInt/Baseline Support for ES6 Generators and enable this feature

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 265004 [details]
Patch

WIP: Just add necessary stubs

[Yusuke Suzuki]

Created attachment 265015 [details]
Patch

WIP: part 2

[Yusuke Suzuki]

Created attachment 265017 [details]
Patch

WIP: part 3

[Yusuke Suzuki]

Created attachment 265018 [details]
Patch

WIP: part 4

[Yusuke Suzuki]

Created attachment 265020 [details]
Patch

WIP: part 5

[Yusuke Suzuki]

Created attachment 265069 [details]
Patch

WIP: part 6

[Yusuke Suzuki]

Created attachment 265306 [details]
Patch

WIP: part 7

[Yusuke Suzuki]

Created attachment 265310 [details]
Patch

WIP: part 8

[Yusuke Suzuki]

Comment on attachment 265310 [details]
Patch

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

For MTG discussion, I left some notes :) This patch is still highly development phase.

> Source/JavaScriptCore/runtime/GeneratorState.h:33
> +class GeneratorState : public JSCell {

This should store the current Frame.
I'm still considering which is the best; storing Frame into a generator object directly OR allocating this separately like this.
As a first step, I'm now implementing naive approach. But later, I'll figure out which design is better, may change the patch and submit it for reviews.

> Source/JavaScriptCore/runtime/GeneratorState.h:50
> +        return bitwise_cast<WriteBarrierBase<Unknown>*>(bitwise_cast<char*>(this) + offsetOfVariables());

I'm now planning to store registers as WriteBarrierBase<Unknown>.
So when doing this in FTL / DFG, we need to unbox. Value recovery information would help it.
Storing this as JSValue makes two things easy. (1) when storing, we correctly activate write barriers. (2) visitChildren easily marks all.

[Yusuke Suzuki]

Created attachment 265486 [details]
Patch

WIP: part 9. super naive prototype. there are many bugs, missing parts and inefficient parts. but small example works.

[Saam Barati]

(In reply to comment #10)
> Created attachment 265486 [details]
> Patch
> 
> WIP: part 9. super naive prototype. there are many bugs, missing parts and
> inefficient parts. but small example works.

👍

[Yusuke Suzuki]

Created attachment 265539 [details]
Patch

WIP: part 10

[Yusuke Suzuki]

Created attachment 265545 [details]
Patch

WIP: part 11, return/throw support.

[Yusuke Suzuki]

Created attachment 265558 [details]
Patch

WIP: part 12, test enable part 1

[Yusuke Suzuki]

Created attachment 265583 [details]
Patch

WIP: part 13, almost all compat-table tests are passed. still several edge cases are not covered.

[Yusuke Suzuki]

Created attachment 265681 [details]
Patch

WIP: part 14. Add new_generator_func etc. bytecodes.

[Yusuke Suzuki]

Created attachment 265686 [details]
Patch

WIP: part 15. Support throw/return before first calls. Add tests.

[Yusuke Suzuki]

Created attachment 265895 [details]
Patch

WIP: part 16. Except for correct 'super' reference, all the functionality is implemented. Code cleanup, some optimization is needed.

[Yusuke Suzuki]

Created attachment 265954 [details]
Patch

WIP: part 17. Correctly handle incorrect js values in stack.

[Yusuke Suzuki]

Created attachment 266054 [details]
Patch

WIP: part 18. Correctly handle eval('this').

[Yusuke Suzuki]

Created attachment 266056 [details]
Patch

WIP: part 19. Done, let's tweak OSX and Windows project files.

[Yusuke Suzuki]

(In reply to comment #11)
> (In reply to comment #10)
> > Created attachment 265486 [details]
> > Patch
> > 
> > WIP: part 9. super naive prototype. there are many bugs, missing parts and
> > inefficient parts. but small example works.
> 
> 👍

:D

[Yusuke Suzuki]

Created attachment 266059 [details]
Patch

[WebKit Commit Bot]

Attachment 266059 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:26:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:30:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:36:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:37:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:38:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:39:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:40:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:43:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:44:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:46:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:47:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:48:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:49:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:50:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:51:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:52:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:53:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:54:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:55:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:56:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:58:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:59:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:60:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:61:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:62:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:63:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:64:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:65:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:66:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:68:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:69:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:70:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:71:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:72:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:74:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:75:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 47 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266060 [details]
Patch

[WebKit Commit Bot]

Attachment 266060 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:26:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:30:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:36:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:37:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:38:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:39:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:40:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:43:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:44:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:46:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:47:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:48:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:49:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:50:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:51:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:52:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:53:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:54:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:55:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:56:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:58:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:59:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:60:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:61:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:62:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:63:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:64:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:65:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:66:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:68:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:69:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:70:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:71:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:72:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:74:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 46 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 266060 [details]
Patch

Attachment 266060 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/464496

New failing tests:
js/dom/reserved-words-as-property.html

[Build Bot]

Created attachment 266063 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266060 [details]
Patch

Attachment 266060 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/464494

New failing tests:
js/dom/reserved-words-as-property.html

[Build Bot]

Created attachment 266064 [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

[Build Bot]

Comment on attachment 266060 [details]
Patch

Attachment 266060 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/464500

New failing tests:
js/dom/reserved-words-as-property.html

[Build Bot]

Created attachment 266065 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 266066 [details]
Patch

[WebKit Commit Bot]

Attachment 266066 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 266066 [details]
Patch

I'll move more ops to Generator.prototype side.

[Yusuke Suzuki]

Created attachment 266076 [details]
Patch

[WebKit Commit Bot]

Attachment 266076 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266077 [details]
Patch

[WebKit Commit Bot]

Attachment 266077 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Ready for review!

[Yusuke Suzuki]

Created attachment 266078 [details]
Patch

minor style tweak

[WebKit Commit Bot]

Attachment 266078 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 266078 [details]
Patch

More tweak.

[Yusuke Suzuki]

Created attachment 266080 [details]
Patch

[WebKit Commit Bot]

Attachment 266080 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266083 [details]
Patch

[WebKit Commit Bot]

Attachment 266083 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 105 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 266083 [details]
Patch

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

LGTM so far.
I still have a lot of the interesting code to review closely.
Just posting a few comments, mostly stylistic, and then I'll post more comments as I review the patch further.

> Source/JavaScriptCore/ChangeLog:45
> +                    sentValue = @generatorValue;

is sentValue what is passed into next?
i.e, next(@generatorValue)?

> Source/JavaScriptCore/builtins/Generator.prototype.js:26
> +function next(sentValue)

All three of these functions are identical except for a difference in GeneratorResumeMode.
Is it worth creating a common implementation? Is that even doable? Would it be too slow?
Maybe we want different functions for optimization reasons.
What do you think?

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:243
> +            functor(codeBlock, instruction, opcodeID, virtualRegisterForLocal(local).offset());

This seems like it might be wrong in certain situations for the same reasons you've stated
for needing the isLiveCell function. I'm not sure if it's a problem if this is exact or if it can
be a conservative estimate.

> Source/JavaScriptCore/bytecode/CodeBlock.h:661
> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }

style: lets make this function have  in-between newlines.

> Source/JavaScriptCore/bytecode/CodeBlock.h:662
> +    const FastBitVector& liveCalleeRegistersAtYield(int index) { RELEASE_ASSERT(m_rareData); return m_rareData->m_liveCalleeRegistersAtYield[index]; }

I wonder if a better name for liveCalleeRegistersXXX would be "liveCalleeLocalsXXX" or "liveCalleeTempsXXX"
Whenever I think about the phrase Callee Registers I think about machine registers, not VirtualRegisters.
I'm not sure what the precedent for this is inside JSC. What do you think? I know we call it numCalleeRegisters() above, so this name is consistent with that name.
FWIW, I think that name is also kind of weird, and would sound better as numCalleeLocals or numCalleeTemps or something similar.

> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
> +// FIXME: These flags, ParserModes and propagation to XXXCodeBlocks should be reorganized.

👍

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:254
> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }

style: this probably doesn't matter much but you can make this "unsigned" and instead of using Instruction.operand you can use Instruction.unsignedValue

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:521
> +        // FIXME: Emit to_this only when Generator uses it.

It's worth having a bug # here.

> Source/JavaScriptCore/parser/Parser.cpp:329
> +

nit: new whitespace

> Source/JavaScriptCore/parser/Parser.cpp:492
> +    info.parameterCount = 4; // generator, state, value, type

type => resume mode

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:98
> +        program = makeString("{function ", (isGenerator) ? "*" : "", functionName.string(), "() {\n\n}}");

style: I don't think you need parens around isGenerator.

> Source/JavaScriptCore/runtime/GeneratorFrame.h:65
> +    static ptrdiff_t offsetOfRegister(size_t index)

This is unused.

> Source/JavaScriptCore/runtime/GeneratorFunctionConstructor.cpp:56
> +    return JSValue::encode(constructFunction(exec, asInternalFunction(exec->callee())->globalObject(), args, /* isGenerator */ true));

style: I think we should use an enum class for isGenerator instead of a boolean.
That would solve having to add the comment: "/* isGenerator */"

> Source/JavaScriptCore/runtime/GeneratorThis.h:54
> +// The following flag manages this state. When GeneratorFunction is called as a constructor, it returnes a Generator that function has GeneratorThis::Empty flag.

typo: "returnes" => "returns"

> Source/JavaScriptCore/runtime/GeneratorThis.h:56
> +enum class GeneratorThis : unsigned {

Style: I think a better name for this would be GeneratorThisMode

[Yusuke Suzuki]

Comment on attachment 266083 [details]
Patch

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

Thank you for your great comments! I've reconsidered my design. I'll upload the revised patch, that leverages BytecodeLivenessAnalysis and it becomes much much cleaner :)

>> Source/JavaScriptCore/ChangeLog:45
>> +                    sentValue = @generatorValue;
> 
> is sentValue what is passed into next?
> i.e, next(@generatorValue)?

Yup. I've noted it :)

>> Source/JavaScriptCore/builtins/Generator.prototype.js:26
>> +function next(sentValue)
> 
> All three of these functions are identical except for a difference in GeneratorResumeMode.
> Is it worth creating a common implementation? Is that even doable? Would it be too slow?
> Maybe we want different functions for optimization reasons.
> What do you think?

They are slightly different (for example, in throw, when seeing Completed state, it throws.), but not so much different.
So, to check the difference, I've created the merged function. The result seems not so large and I believe it's ok for performance.
I'll update this patch with it :)

Only one concern is, of cource, the merged function is larger than this original `next`. Compared to `throw` and `return`, `next` should be so frequently called.
Making `next` small is good for DFG's inlining decision. (But seeing the result, I think it's not so large).
So I'll upload the revised one and I would like to hear your opinion.

> Source/JavaScriptCore/builtins/Generator.prototype.js:58
> +    return { done, value };

This is intentional. Only one object allocation site makes object allocation sinking easier.

>> Source/JavaScriptCore/bytecode/CodeBlock.h:661
>> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }
> 
> style: lets make this function have  in-between newlines.

Inserted :)

>> Source/JavaScriptCore/bytecode/CodeBlock.h:662
>> +    const FastBitVector& liveCalleeRegistersAtYield(int index) { RELEASE_ASSERT(m_rareData); return m_rareData->m_liveCalleeRegistersAtYield[index]; }
> 
> I wonder if a better name for liveCalleeRegistersXXX would be "liveCalleeLocalsXXX" or "liveCalleeTempsXXX"
> Whenever I think about the phrase Callee Registers I think about machine registers, not VirtualRegisters.
> I'm not sure what the precedent for this is inside JSC. What do you think? I know we call it numCalleeRegisters() above, so this name is consistent with that name.
> FWIW, I think that name is also kind of weird, and would sound better as numCalleeLocals or numCalleeTemps or something similar.

Locals looks significantly nice.
"callee registers" is confusing to me too :). I've replaced this to `CalleeLocals` and `numCalleeRegisters` to `numCalleeLocals`.

>> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
>> +// FIXME: These flags, ParserModes and propagation to XXXCodeBlocks should be reorganized.
> 
> 👍

Yeah!

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:254
>> +    int32_t addLiveCalleeRegistersAtYield(FastBitVector vector) { createRareDataIfNecessary(); m_rareData->m_liveCalleeRegistersAtYield.append(vector); return m_rareData->m_liveCalleeRegistersAtYield.size() - 1; }
> 
> style: this probably doesn't matter much but you can make this "unsigned" and instead of using Instruction.operand you can use Instruction.unsignedValue

That's nice. Changed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:521
>> +        // FIXME: Emit to_this only when Generator uses it.
> 
> It's worth having a bug # here.

Nice. I've created the bug and pasted it here.
https://bugs.webkit.org/show_bug.cgi?id=151586

>> Source/JavaScriptCore/parser/Parser.cpp:329
>> +
> 
> nit: new whitespace

Removed :)

>> Source/JavaScriptCore/parser/Parser.cpp:492
>> +    info.parameterCount = 4; // generator, state, value, type
> 
> type => resume mode

Fixed :)

>> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:98
>> +        program = makeString("{function ", (isGenerator) ? "*" : "", functionName.string(), "() {\n\n}}");
> 
> style: I don't think you need parens around isGenerator.

Fixed.

>> Source/JavaScriptCore/runtime/GeneratorFrame.h:65
>> +    static ptrdiff_t offsetOfRegister(size_t index)
> 
> This is unused.

Dropped.

>> Source/JavaScriptCore/runtime/GeneratorFunctionConstructor.cpp:56
>> +    return JSValue::encode(constructFunction(exec, asInternalFunction(exec->callee())->globalObject(), args, /* isGenerator */ true));
> 
> style: I think we should use an enum class for isGenerator instead of a boolean.
> That would solve having to add the comment: "/* isGenerator */"

Added `FunctionConstructionMode` and use it :)

>> Source/JavaScriptCore/runtime/GeneratorThis.h:54
>> +// The following flag manages this state. When GeneratorFunction is called as a constructor, it returnes a Generator that function has GeneratorThis::Empty flag.
> 
> typo: "returnes" => "returns"

Nice! Fixed.

>> Source/JavaScriptCore/runtime/GeneratorThis.h:56
>> +enum class GeneratorThis : unsigned {
> 
> Style: I think a better name for this would be GeneratorThisMode

Renamed :)

[Yusuke Suzuki]

Created attachment 266127 [details]
Patch

[Yusuke Suzuki]

Created attachment 266128 [details]
Patch

[Yusuke Suzuki]

https://goo.gl/photos/CnTvLR1Z7wtd7JiDA figure to explain how save / resume works with use-def analysis.

[Yusuke Suzuki]

Created attachment 266134 [details]
Patch

[WebKit Commit Bot]

Attachment 266134 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1777:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266135 [details]
Patch

[WebKit Commit Bot]

Attachment 266135 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1777:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 266083 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:243
>> +            functor(codeBlock, instruction, opcodeID, virtualRegisterForLocal(local).offset());
> 
> This seems like it might be wrong in certain situations for the same reasons you've stated
> for needing the isLiveCell function. I'm not sure if it's a problem if this is exact or if it can
> be a conservative estimate.

Nice catch. That's a problem.
I've changed the design slightlhy. op_save only uses a first generator and defines nothing.
And op_resume defines all the used variables at the head of the successor basic block.
I added the virtual jump edge from op_save to the point that is called the merge point.
And op_resume is also has the flow to this merge point.

            (global generator switch) -> (initial sequence) -> (op_save) ----+-> (merge point) -> (next sequence)*
                   |                                              |          |
                   |                                              v          |
                   |                                           (op_ret)      |
                   |                                                         |
                   +------------------------------------------->(op_resume)--+

By constructing such a graph,

1. Since we have a flow from (op_save) to (merge point), at merge point, we can *use* locals that are defined before (op_save)
2. op_save should claim that it does not define anything.
3. at op_resume, we see *use*d locals at merge point and define all of them.

We can do the above things in use-def analysis because use-def analysis is backward analysis.
And after analyzing use-def chains, in op_save / op_resume we only save / resume live registers at the head of merge point.

[Yusuke Suzuki]

Created attachment 266152 [details]
Patch

[WebKit Commit Bot]

Attachment 266152 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1783:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266161 [details]
Patch

[WebKit Commit Bot]

Attachment 266161 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1783:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 266161 [details]
Patch

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

Added comment for ease of review.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:62
> +    case op_save:

op_save has a virtual jump to a merge point. (the virtual jump means, that is a jump edge, but it is not used actually. It is used to construct CFG, and this flow will be leveraged for DFG analysis.)

Since op_save has a virtual jump to a merge point, there is a edge from op_save to the merge point.
And the merge point can use registers that are defined predecessor basic blocks that is dominated by this op_save!

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:253
> +        return;

op_save *use*s

1. generator register to save registers to its GeneratorFrame member.
2. registers that are used at the merge point. Since use-def is backward analysis, when analyzing op_save, its information is already analyzed.

And op_save *def*ines

1. Nothing

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:421
> +        return;

op_resume *use*s

1. generator register to resume registers from GeneratorFrame

And op_resume *def*ines

2. registers that are used at the merge point.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1550
> +        }

And added liveness information to bytecode dump. When you see bytecode with `-d`, you can see which registers will be saved / resumed at that op_save / op_resume.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2275
> +    }

This is important part. We perform BytecodeLivenessAnalysis to find which register is live at a given merge point.
This information will be used to save / resume registers.

And interestinglly, in typical cases, the number of live registers is very small.
We will emit asm directly for this save / resume operation in DFG/FTL! This enables significantly fast generator operations.

[Yusuke Suzuki]

Comment on attachment 266161 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:62
>> +    case op_save:
> 
> op_save has a virtual jump to a merge point. (the virtual jump means, that is a jump edge, but it is not used actually. It is used to construct CFG, and this flow will be leveraged for DFG analysis.)
> 
> Since op_save has a virtual jump to a merge point, there is a edge from op_save to the merge point.
> And the merge point can use registers that are defined predecessor basic blocks that is dominated by this op_save!

s/dominated/that will reach to the merge point through this op_save/.

[Saam Barati]

Comment on attachment 266161 [details]
Patch

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

LGTM, just a few comments. I have some more comments I'll write tomorrow but I'm about to board a flight.

> Source/JavaScriptCore/ChangeLog:79
> +        Registers reside in C stack. So if we don't perform it, we need to determine whether a given value is *live* and *correct* JSCell or not

I'm not sure if this last sentence is necessary and it's a bit confusing to me. I think the above sentence saying that we run BytecodeLiveness is enough to get this idea across
that we need to know which registers to save and restore.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2247
> +            mergePointBytecodeOffsets.resize(liveCalleeLocalsIndex + 1);

Nit: Even though this number will always get larger because of how we parse/generate bytecode, we really should only resize when we're resizing to a larger number than the current size.
It might be worth making that explicit either through an assert/if statement/max function call.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4128
> +    RegisterID* completedState = emitLoad(nullptr, jsNumber(static_cast<int32_t>(state)));

style: cast not needed

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3006
> +        generator.emitProfileType(generator.generatorRegister(), ProfileTypeBytecodeFunctionReturnStatement); // Do not emit expression info for this profile because it's not in the user's source code.

I don't think this is needed.

> Source/JavaScriptCore/tests/es6.yaml:841
>  - path: es6/generators_yield_star_instances_of_iterables.js

Can we also add some of our own "yeild*" tests with different variations?

> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:1
> +function unreachable()

I think it's worth having more return(X) and throw(X) tests.

> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:12
> +g.return("Hello");

We should have tests for the return value here.

[Yusuke Suzuki]

Created attachment 266224 [details]
Patch

Update yield*, added tests, but still return/throw tests are not added yet.

[WebKit Commit Bot]

Attachment 266224 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1783:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266343 [details]
Patch

[WebKit Commit Bot]

Attachment 266343 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1792:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 266346 [details]
Patch

[WebKit Commit Bot]

Attachment 266346 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1792:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 266346 [details]
Patch

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

r=me with a couple comments

> Source/JavaScriptCore/bytecode/PreciseJumpTargets.cpp:57
> +    case op_save:

Maybe a small comment is worth it here just to say that this is purely
for calculating liveness.

> Source/JavaScriptCore/parser/ParserModes.h:50
> +    GeneratorFunctionMode,

GeneratorMode and GeneratorFunctionMode confuse me a bit as names. 
I wonder if we can name them better to be more indicative of what they do
Maybe GeneratorBodyMode instead of GeneratorMode. Maybe GeneratorWrapperFunctionMode
instead of GeneratorFunctionMode?

[Yusuke Suzuki]

Comment on attachment 266161 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:79
>> +        Registers reside in C stack. So if we don't perform it, we need to determine whether a given value is *live* and *correct* JSCell or not
> 
> I'm not sure if this last sentence is necessary and it's a bit confusing to me. I think the above sentence saying that we run BytecodeLiveness is enough to get this idea across
> that we need to know which registers to save and restore.

Yes. Running BytecodeLiveness is enough. Dropped this sequence.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2247
>> +            mergePointBytecodeOffsets.resize(liveCalleeLocalsIndex + 1);
> 
> Nit: Even though this number will always get larger because of how we parse/generate bytecode, we really should only resize when we're resizing to a larger number than the current size.
> It might be worth making that explicit either through an assert/if statement/max function call.

OK, I've added one if statement that avoid `resize` when vector size is enough for a given index.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4128
>> +    RegisterID* completedState = emitLoad(nullptr, jsNumber(static_cast<int32_t>(state)));
> 
> style: cast not needed

Nice! Dropped.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3006
>> +        generator.emitProfileType(generator.generatorRegister(), ProfileTypeBytecodeFunctionReturnStatement); // Do not emit expression info for this profile because it's not in the user's source code.
> 
> I don't think this is needed.

OK, dropped.

>> Source/JavaScriptCore/tests/es6.yaml:841
>>  - path: es6/generators_yield_star_instances_of_iterables.js
> 
> Can we also add some of our own "yeild*" tests with different variations?

Added :)

>> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:1
>> +function unreachable()
> 
> I think it's worth having more return(X) and throw(X) tests.

Added :)

>> Source/JavaScriptCore/tests/stress/generator-return-before-first-call.js:12
>> +g.return("Hello");
> 
> We should have tests for the return value here.

Added :D

[Yusuke Suzuki]

Comment on attachment 266346 [details]
Patch

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

Thanks for your great review!

>> Source/JavaScriptCore/bytecode/PreciseJumpTargets.cpp:57
>> +    case op_save:
> 
> Maybe a small comment is worth it here just to say that this is purely
> for calculating liveness.

Sounds nice. Added a small comment.

>> Source/JavaScriptCore/parser/ParserModes.h:50
>> +    GeneratorFunctionMode,
> 
> GeneratorMode and GeneratorFunctionMode confuse me a bit as names. 
> I wonder if we can name them better to be more indicative of what they do
> Maybe GeneratorBodyMode instead of GeneratorMode. Maybe GeneratorWrapperFunctionMode
> instead of GeneratorFunctionMode?

Your suggestion looks nice. More descriptive name is nice for readability :D Changed.

[Yusuke Suzuki]

Committed r192914: <http://trac.webkit.org/changeset/192914>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 151734

[Ryan Haddad]

64-bit JSC test failures:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/843>

32-bit JSC test failures:
<https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/7137>

[Yusuke Suzuki]

Created attachment 266421 [details]
Patch

[WebKit Commit Bot]

Attachment 266421 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1792:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:213:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 266421 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:-229
> -#endif

Drop this. Now create_this can be used for non constructor.

[Saam Barati]

Comment on attachment 266421 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r192937: <http://trac.webkit.org/changeset/192937>

[Yusuke Suzuki]

oops, Windows build's ENABLE_ES6_GENERATORS is not enabled.
I'll land the follow up patch that enables ENABLE_ES6_GENERATORS. (Maybe, CMake)

[Yusuke Suzuki]

Hmm, OptionsWin.cmake is correctly updated...
I've just requested Win force rebuild.

[Yusuke Suzuki]

OK, it seems that ENABLE_ES6_MODULES is enabled.