Bug 230086 - [JSC] Optimize op_get_property_enumerator further
Summary: [JSC] Optimize op_get_property_enumerator further
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 229888 229889 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-09-09 00:32 PDT by Yusuke Suzuki
Modified: 2021-09-20 01:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (38.17 KB, patch)
2021-09-09 00:35 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (39.90 KB, patch)
2021-09-09 00:40 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***