Bug 224275

Summary: [JSC] Use FixedVector more in bytecode dir and JumpTable
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch msaboff: review+

Description Yusuke Suzuki 2021-04-07 02:37:44 PDT
[JSC] Use FixedVector more in bytecode dir and JumpTable
Comment 1 Yusuke Suzuki 2021-04-07 02:41:53 PDT
Created attachment 425372 [details]
Patch
Comment 2 Michael Saboff 2021-04-07 11:39:33 PDT
Comment on attachment 425372 [details]
Patch

r=me
Comment 3 Mark Lam 2021-04-07 11:53:33 PDT
Comment on attachment 425372 [details]
Patch

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

r=me with issues addressed.

> Source/JavaScriptCore/ChangeLog:8
> +        1. Use FixedVector more in bytecode/ directory's long-living data structures.[

Accidental '['.

> Source/JavaScriptCore/ChangeLog:9
> +        2. Use FixedVector in SimpleJumpTable. This involves LLInt changes because we need to access to FixedVector data from LLInt.

/access to/access/

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h:61
> +        result.m_data->m_vector = FixedVector<ObjectPropertyCondition>(WTFMove(vector));

Shouldn't this be:
    result.m_data->m_vector = WTFMove(FixedVector<ObjectPropertyCondition>(vector));

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:616
> +const RefCountedArrayStorageNonNullRefCountOffset = -(constexpr (RefCountedArray::Header::size())) + RefCountedArray::Header::refCount

Is this needed?

> Source/WTF/wtf/RefCountedArray.h:280
> +        static ptrdiff_t offsetOfRefCount() { return OBJECT_OFFSETOF(Header, refCount); }
> +        static ptrdiff_t offsetOfLength() { return OBJECT_OFFSETOF(Header, length); }

Are these used anywhere?
Comment 4 Yusuke Suzuki 2021-04-07 13:13:49 PDT
Comment on attachment 425372 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h:61
>> +        result.m_data->m_vector = FixedVector<ObjectPropertyCondition>(WTFMove(vector));
> 
> Shouldn't this be:
>     result.m_data->m_vector = WTFMove(FixedVector<ObjectPropertyCondition>(vector));

This becomes move-assignment without WTFMove.
Comment 5 Yusuke Suzuki 2021-04-07 13:15:05 PDT
Comment on attachment 425372 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        1. Use FixedVector more in bytecode/ directory's long-living data structures.[
> 
> Accidental '['.

Fixed.

>> Source/JavaScriptCore/ChangeLog:9
>> +        2. Use FixedVector in SimpleJumpTable. This involves LLInt changes because we need to access to FixedVector data from LLInt.
> 
> /access to/access/

Fixed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:616
>> +const RefCountedArrayStorageNonNullRefCountOffset = -(constexpr (RefCountedArray::Header::size())) + RefCountedArray::Header::refCount
> 
> Is this needed?

I added for the completeness, but maybe we never access it. Removed.

>> Source/WTF/wtf/RefCountedArray.h:280
>> +        static ptrdiff_t offsetOfLength() { return OBJECT_OFFSETOF(Header, length); }
> 
> Are these used anywhere?

Not used. But I think having offsetOfLength() is good since we could touch it from JIT in the future, and we know that this field is accessed by LLInt by having this function too.
Comment 6 Yusuke Suzuki 2021-04-07 14:15:10 PDT
Committed r275626 (236271@main): <https://commits.webkit.org/236271@main>
Comment 7 Radar WebKit Bug Importer 2021-04-07 14:16:16 PDT
<rdar://problem/76365012>