[webkit-reviews] review granted: [Bug 201498] [JSC] AsyncGenerator should have internal fields : [Attachment 379600] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 201498: [JSC] AsyncGenerator should have internal fields
https://bugs.webkit.org/show_bug.cgi?id=201498

Attachment 379600: Patch

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




--- Comment #11 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 379600
  --> https://bugs.webkit.org/attachment.cgi?id=379600
Patch

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

r=me

>> Source/JavaScriptCore/ChangeLog:8
>> +	    This patch introduces JSAsyncGenerator. We do the same thing to
JSGenerator to improve async generator performance.
> 
> I don't understand your second sentence. Is this patch just for async
generator. Or regular generator too? If both, the title should be renamed

it looks like this patch is just JSAsyncGenerator. I think a better sentence is
something like:
"We did this already for JSGenerator. This patch does the same thing for
JSAsyncGenerator"

> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:98
> +    m_asyncGeneratorFieldSuspendReason.set(m_vm,
jsNumber(static_cast<unsigned>(JSAsyncGenerator::Field::SuspendReason)));
> +    m_asyncGeneratorFieldQueueFirst.set(m_vm,
jsNumber(static_cast<unsigned>(JSAsyncGenerator::Field::QueueFirst)));
> +    m_asyncGeneratorFieldQueueLast.set(m_vm,
jsNumber(static_cast<unsigned>(JSAsyncGenerator::Field::QueueLast)));
> +    m_AsyncGeneratorStateCompleted.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::Completed)
));
> +    m_AsyncGeneratorStateExecuting.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::Executing)
));
> +    m_AsyncGeneratorStateSuspendedStart.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::SuspendedS
tart)));
> +    m_AsyncGeneratorStateSuspendedYield.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::SuspendedY
ield)));
> +    m_AsyncGeneratorStateAwaitingReturn.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::AwaitingRe
turn)));
> +    m_AsyncGeneratorSuspendReasonYield.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::Yi
eld)));
> +    m_AsyncGeneratorSuspendReasonAwait.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::Aw
ait)));
> +    m_AsyncGeneratorSuspendReasonNone.set(m_vm,
jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::No
ne)));

why some capital "A" and some lower case "a"?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2694
> +	   auto findConstant = [&] (const ClassInfo* classInfo) -> bool {

nit: maybe a better name is "tryToFold" or similar, since we're not actually
producing a constant value, but a constant structure

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12897
> +    auto initialValues = JSClass::initialValues();
> +    for (unsigned index = 0; index < JSClass::numberOfInternalFields;
++index)

same thing about assert

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5952
> +	   auto initialValues = JSClass::initialValues();
> +	   for (unsigned index = 0; index < JSClass::numberOfInternalFields;
++index)

maybe assert that initialValues.size() == numberOfInternalFields?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6426
> +	   auto initialValues = JSClass::initialValues();
> +	   for (unsigned index = 0; index < JSClass::numberOfInternalFields;
++index)

maybe assert that initialValues.size()	matches numberOfInternalFields?

> Source/JavaScriptCore/runtime/JSGenerator.cpp:58
> +    for (unsigned index = 0; index < numberOfInternalFields; ++index)
> +	   internalField(index).set(vm, this, values[index]);

nit: it feels slightly weird to not just iterate over the size of the array
here (which is also constexpr). (Same comment elsewhere based on how we
iterate.)