Bug 230086

Summary:

[JSC] Optimize op_get_property_enumerator further

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	saam: review+

Description Yusuke Suzuki 2021-09-09 00:32:31 PDT

[JSC] Optimize op_get_property_enumerator further

Comment 1 Yusuke Suzuki 2021-09-09 00:35:46 PDT

Created attachment 437718 [details]
Patch

Comment 2 Yusuke Suzuki 2021-09-09 00:40:17 PDT

Created attachment 437719 [details]
Patch

Comment 3 Saam Barati 2021-09-09 09:01:54 PDT

Comment on attachment 437719 [details]
Patch

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

r=me with one bug to fix and test again

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13980
> +        if ((baseValue.m_type && !(baseValue.m_type & ~SpecObject)) && baseValue.m_structure.isFinite()) {

Nit: first two clauses can be baseValue.isType(SpecObject)

We don’t care about checking zero since zero just means we couldn’t be executing at this point anyways.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13256
> +            if ((baseValue.m_type && !(baseValue.m_type & ~SpecObject)) && baseValue.m_structure.isFinite()) {

Ditto. Can use isType

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2816
> +    genericCases.link(this);

Why not use the baseline JIT’s slow path mechanism?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3100
> +    loadb JSCell::m_type[t0], t1

Should be m_indexingTypeAndMisc. I wonder if this just made us go slow all the time. Worth checking the rest of the fast path is correct.

Comment 4 Yusuke Suzuki 2021-09-09 12:26:20 PDT

Comment on attachment 437719 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13980
>> +        if ((baseValue.m_type && !(baseValue.m_type & ~SpecObject)) && baseValue.m_structure.isFinite()) {
> 
> Nit: first two clauses can be baseValue.isType(SpecObject)
> 
> We don’t care about checking zero since zero just means we couldn’t be executing at this point anyways.

Right! Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13256
>> +            if ((baseValue.m_type && !(baseValue.m_type & ~SpecObject)) && baseValue.m_structure.isFinite()) {
> 
> Ditto. Can use isType

Fixed.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2816
>> +    genericCases.link(this);
> 
> Why not use the baseline JIT’s slow path mechanism?

Because this is still not slow path. It is used if indexed properties exist. We have a super fast path inlined, but this does not mean that this slowpath will not be used.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3100
>> +    loadb JSCell::m_type[t0], t1
> 
> Should be m_indexingTypeAndMisc. I wonder if this just made us go slow all the time. Worth checking the rest of the fast path is correct.

Oops, fixed.

Comment 5 Yusuke Suzuki 2021-09-09 14:36:47 PDT

Committed r282239 (241523@main): <https://commits.webkit.org/241523@main>

Comment 6 Radar WebKit Bug Importer 2021-09-09 14:37:18 PDT

<rdar://problem/82943743>

Comment 7 Yusuke Suzuki 2021-09-20 01:46:16 PDT

*** Bug 229888 has been marked as a duplicate of this bug. ***

Comment 8 Yusuke Suzuki 2021-09-20 01:46:39 PDT

*** Bug 229889 has been marked as a duplicate of this bug. ***