[webkit-reviews] review granted: [Bug 201159] [JSC] Generator should have internal fields : [Attachment 378875] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 201159: [JSC] Generator should have internal fields
https://bugs.webkit.org/show_bug.cgi?id=201159

Attachment 378875: Patch

https://bugs.webkit.org/attachment.cgi?id=378875&action=review




--- Comment #21 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 378875
  --> https://bugs.webkit.org/attachment.cgi?id=378875
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:14
> +	   If we make these structures adaptively poly-proto ones, some
generators get poly-proto structures while others are not, resulting
> +	   in megamorphic lookup in Generator.prototype.next. If we make all
the generator's structure poly-proto ones, it makes Generator.prototype.next
> +	   lookup suboptimal for now.

Nit: poly-proto ones -> poly-proto. 

while others do not,

> Source/JavaScriptCore/ChangeLog:18
> +	   In this patch, we start with relatively simple way. This patch
introduces JSGenerator class, and it has internal fields for generator's
internal
> +	   states. We extend promise-internal-field access bytecodes to access
to these fields from bytecode so that Generator.prototype.next can access
> +	   these fields without using megamorphic get_by_id_direct.

Nit: start with relatively simple way => start with a relatively simple
solution

> Source/JavaScriptCore/ChangeLog:20
> +	   And we can attach JSGeneratorType to JSGenerator so that we can
efficiently implement `@isGenerator()` check in bytecode.

Nit: we can attach => we attach

> Source/JavaScriptCore/ChangeLog:27
> +	   Currently, we start having op_create_generator since it could be
different from create_promise when we add PolyProto support. But when adding
create_async_generator-like
> +	   thing, we will introduce one abstracted bytecode for both
create_generator and create_async_generator to efficiently support both.

Nit: I would probably phrase this as This patch adds op_create_generator since
it is distinct from op_create_promise once we add PolyProto support. In the
future when we introduce some kind of op_create_async_generator we will
probably share only one bytecode for both generator and async generator.

> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:80
> +    m_generatorFieldState.set(m_vm,
jsNumber(static_cast<unsigned>(JSGenerator::Field::State)));
> +    m_generatorFieldNext.set(m_vm,
jsNumber(static_cast<unsigned>(JSGenerator::Field::Next)));
> +    m_generatorFieldThis.set(m_vm,
jsNumber(static_cast<unsigned>(JSGenerator::Field::This)));
> +    m_generatorFieldFrame.set(m_vm,
jsNumber(static_cast<unsigned>(JSGenerator::Field::Frame)));

Hmm, it feels like we should simplify this because we have so many of these
now... I wish we could combine all the offset registries (FTLOffset, LLInt
offsets, and this) into one class...

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4989
> +			       addToGraph(Phantom, callee);

Doesn't this need to be after the NewGenerator? If we exit in NewGenerator then
we won't be able to recover callee correct? Callee's Phantom has already
passed.