Bug 201498 – [JSC] AsyncGenerator should have internal fields

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 379439 [details]
Patch

[Yusuke Suzuki]

Created attachment 379440 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 379441 [details]
Patch

WIP

[Yusuke Suzuki]

Note that, this improves JetStream2/async-fs by ~10%.

[Yusuke Suzuki]

Created attachment 379527 [details]
Patch

[Yusuke Suzuki]

I'll quickly fix debug-wk1.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #6)
> I'll quickly fix debug-wk1.

After adding a new bytecode, maybe, hash order is changed? and op_add becomes 0.
And we are hitting crash because we are using op_add << 32 | metadataID as a key to hash table. And we cannot use 0 as a key.
I'll add 1 to the opcode to make key non-zero.

[Yusuke Suzuki]

Created attachment 379600 [details]
Patch

[Saam Barati]

Comment on attachment 379600 [details]
Patch

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

> 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

> Source/JavaScriptCore/ChangeLog:12
> +        We also adjust ArithProfile key in JIT by adding +1 to opcodeID since `op_add` can be zero, and `key` can be zero in this case.

what is "key" here? What's the benefit here?

[Yusuke Suzuki]

Comment on attachment 379600 [details]
Patch

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

>> 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

Introducing JSAsyncGenerator, basically the same mechanism to JSGenerator.

>> Source/JavaScriptCore/ChangeLog:12
>> +        We also adjust ArithProfile key in JIT by adding +1 to opcodeID since `op_add` can be zero, and `key` can be zero in this case.
> 
> what is "key" here? What's the benefit here?

This is pre-existing bug. We are using OpcodeID for the key of hashmap. And using op_add code as a part of key. By adding a new bytecode, it suddenly becomes 0. And 0 is not valid key in WTF::HashMap.

[Saam Barati]

Comment on attachment 379600 [details]
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::SuspendedStart)));
> +    m_AsyncGeneratorStateSuspendedYield.set(m_vm, jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::SuspendedYield)));
> +    m_AsyncGeneratorStateAwaitingReturn.set(m_vm, jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorState::AwaitingReturn)));
> +    m_AsyncGeneratorSuspendReasonYield.set(m_vm, jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::Yield)));
> +    m_AsyncGeneratorSuspendReasonAwait.set(m_vm, jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::Await)));
> +    m_AsyncGeneratorSuspendReasonNone.set(m_vm, jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::None)));

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.)

[Saam Barati]

Comment on attachment 379600 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:12
>>> +        We also adjust ArithProfile key in JIT by adding +1 to opcodeID since `op_add` can be zero, and `key` can be zero in this case.
>> 
>> what is "key" here? What's the benefit here?
> 
> This is pre-existing bug. We are using OpcodeID for the key of hashmap. And using op_add code as a part of key. By adding a new bytecode, it suddenly becomes 0. And 0 is not valid key in WTF::HashMap.

got it. Can you explain that here?

[Yusuke Suzuki]

Comment on attachment 379600 [details]
Patch

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

Thanks!

>>>> 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
>> 
>> Introducing JSAsyncGenerator, basically the same mechanism to JSGenerator.
> 
> 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"

Fixed!

>>>> Source/JavaScriptCore/ChangeLog:12
>>>> +        We also adjust ArithProfile key in JIT by adding +1 to opcodeID since `op_add` can be zero, and `key` can be zero in this case.
>>> 
>>> what is "key" here? What's the benefit here?
>> 
>> This is pre-existing bug. We are using OpcodeID for the key of hashmap. And using op_add code as a part of key. By adding a new bytecode, it suddenly becomes 0. And 0 is not valid key in WTF::HashMap.
> 
> got it. Can you explain that here?

Fixed.

>> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:98
>> +    m_AsyncGeneratorSuspendReasonNone.set(m_vm, jsNumber(static_cast<int32_t>(JSAsyncGenerator::AsyncGeneratorSuspendReason::None)));
> 
> why some capital "A" and some lower case "a"?

Capital "A" things exist before this patch and it is used to represent a constant. I use lower case "a" for field index.

>> 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

Sounds nice. Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12897
>> +    for (unsigned index = 0; index < JSClass::numberOfInternalFields; ++index)
> 
> same thing about assert

Fixed. Adding `static_assert`.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5952
>> +        for (unsigned index = 0; index < JSClass::numberOfInternalFields; ++index)
> 
> maybe assert that initialValues.size() == numberOfInternalFields?

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6426
>> +        for (unsigned index = 0; index < JSClass::numberOfInternalFields; ++index)
> 
> maybe assert that initialValues.size()  matches numberOfInternalFields?

Fixed.

>> Source/JavaScriptCore/runtime/JSGenerator.cpp:58
>> +        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.)

OK, use initialValues.size() instead.

[Yusuke Suzuki]

Comment on attachment 379600 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12897
>>> +    for (unsigned index = 0; index < JSClass::numberOfInternalFields; ++index)
>> 
>> same thing about assert
> 
> Fixed. Adding `static_assert`.

Use ASSERT for now.

[Yusuke Suzuki]

Committed r250630: <https://trac.webkit.org/changeset/250630>

[Radar WebKit Bug Importer]

<rdar://problem/55924481>