Bug 224162

Summary: [JSC] Shrink some of Vectors in JSC
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, msaboff, saam, sam, simon.fraser, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+, ews-feeder: commit-queue-
Patch
none
Patch none

Description Yusuke Suzuki 2021-04-03 15:03:00 PDT 
[JSC] Shrink some of Vectors in JSC
Comment 1 Yusuke Suzuki 2021-04-03 15:06:55 PDT 
Created attachment 425110 [details]
Patch
Comment 2 Yusuke Suzuki 2021-04-03 15:09:27 PDT 
Created attachment 425111 [details]
Patch
Comment 3 Yusuke Suzuki 2021-04-03 16:14:55 PDT 
Created attachment 425113 [details]
Patch
Comment 4 Sam Weinig 2021-04-03 16:26:06 PDT 
Comment on attachment 425113 [details]
Patch

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

> Source/JavaScriptCore/bytecode/DeleteByStatus.cpp:137
> +        result.shrinkToFit();

For cases like this, might it make sense to have

> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:129
>      m_routines.shrink(dstIndex);
> +    m_routines.shrinkToFit();

Could these be combined into to just m_routines.shrinkCapacity(dstIndex)?
Comment 5 Yusuke Suzuki 2021-04-03 16:40:03 PDT 
Created attachment 425115 [details]
Patch
Comment 6 Yusuke Suzuki 2021-04-03 17:31:04 PDT 
Comment on attachment 425113 [details]
Patch

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

>> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:129
>> +    m_routines.shrinkToFit();
> 
> Could these be combined into to just m_routines.shrinkCapacity(dstIndex)?

Sounds good. Changed.
Comment 7 Yusuke Suzuki 2021-04-03 17:32:34 PDT 
Created attachment 425117 [details]
Patch
Comment 8 Yusuke Suzuki 2021-04-03 18:21:46 PDT 
Created attachment 425119 [details]
Patch
Comment 9 Yusuke Suzuki 2021-04-03 18:44:43 PDT 
Created attachment 425120 [details]
Patch
Comment 10 Yusuke Suzuki 2021-04-03 18:51:55 PDT 
Created attachment 425121 [details]
Patch
Comment 11 Simon Fraser (smfr) 2021-04-03 19:43:40 PDT 
Comment on attachment 425121 [details]
Patch

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

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:44
> +GetterSetterAccessCase::GetterSetterAccessCase(VM& vm, JSCell* owner, AccessType accessType, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, bool viaProxy, WatchpointSet* additionalSet, JSObject* customSlotBase, RefPtr<PolyProtoAccessChain> prototypeAccessChain)

Should that be a RefPtr<>&&?

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:59
> +        JSObject* customSlotBase, Optional<DOMAttributeAnnotation>, RefPtr<PolyProtoAccessChain>);

RefPtr<>&&?

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:62
> +        const ObjectPropertyConditionSet&, RefPtr<PolyProtoAccessChain>, bool viaProxy = false,

Ditto

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:73
> +    GetterSetterAccessCase(VM&, JSCell*, AccessType, CacheableIdentifier, PropertyOffset, Structure*, const ObjectPropertyConditionSet&, bool viaProxy, WatchpointSet* additionalSet, JSObject* customSlotBase, RefPtr<PolyProtoAccessChain>);

Ditto

> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.cpp:36
> +IntrinsicGetterAccessCase::IntrinsicGetterAccessCase(VM& vm, JSCell* owner, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain> prototypeAccessChain)

Ditto

> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.cpp:42
> +std::unique_ptr<AccessCase> IntrinsicGetterAccessCase::create(VM& vm, JSCell* owner, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain> prototypeAccessChain)

Ditto

> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.h:45
> +    static std::unique_ptr<AccessCase> create(VM&, JSCell*, CacheableIdentifier, PropertyOffset, Structure*, const ObjectPropertyConditionSet&, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain>);

Ditto

> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.h:52
> +    IntrinsicGetterAccessCase(VM&, JSCell*, CacheableIdentifier, PropertyOffset, Structure*, const ObjectPropertyConditionSet&, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain>);

Ditto

> Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:41
> +std::unique_ptr<AccessCase> ProxyableAccessCase::create(VM& vm, JSCell* owner, AccessType type, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, bool viaProxy, WatchpointSet* additionalSet, RefPtr<PolyProtoAccessChain> prototypeAccessChain)

Ditto
Comment 12 Yusuke Suzuki 2021-04-03 19:54:53 PDT 
Comment on attachment 425121 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:44
>> +GetterSetterAccessCase::GetterSetterAccessCase(VM& vm, JSCell* owner, AccessType accessType, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, bool viaProxy, WatchpointSet* additionalSet, JSObject* customSlotBase, RefPtr<PolyProtoAccessChain> prototypeAccessChain)
> 
> Should that be a RefPtr<>&&?

Fixed.

>> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:59
>> +        JSObject* customSlotBase, Optional<DOMAttributeAnnotation>, RefPtr<PolyProtoAccessChain>);
> 
> RefPtr<>&&?

Fixed.

>> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:62
>> +        const ObjectPropertyConditionSet&, RefPtr<PolyProtoAccessChain>, bool viaProxy = false,
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:73
>> +    GetterSetterAccessCase(VM&, JSCell*, AccessType, CacheableIdentifier, PropertyOffset, Structure*, const ObjectPropertyConditionSet&, bool viaProxy, WatchpointSet* additionalSet, JSObject* customSlotBase, RefPtr<PolyProtoAccessChain>);
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.cpp:36
>> +IntrinsicGetterAccessCase::IntrinsicGetterAccessCase(VM& vm, JSCell* owner, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain> prototypeAccessChain)
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.cpp:42
>> +std::unique_ptr<AccessCase> IntrinsicGetterAccessCase::create(VM& vm, JSCell* owner, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain> prototypeAccessChain)
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.h:45
>> +    static std::unique_ptr<AccessCase> create(VM&, JSCell*, CacheableIdentifier, PropertyOffset, Structure*, const ObjectPropertyConditionSet&, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain>);
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/IntrinsicGetterAccessCase.h:52
>> +    IntrinsicGetterAccessCase(VM&, JSCell*, CacheableIdentifier, PropertyOffset, Structure*, const ObjectPropertyConditionSet&, JSFunction* intrinsicFunction, RefPtr<PolyProtoAccessChain>);
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:41
>> +std::unique_ptr<AccessCase> ProxyableAccessCase::create(VM& vm, JSCell* owner, AccessType type, CacheableIdentifier identifier, PropertyOffset offset, Structure* structure, const ObjectPropertyConditionSet& conditionSet, bool viaProxy, WatchpointSet* additionalSet, RefPtr<PolyProtoAccessChain> prototypeAccessChain)
> 
> Ditto

Fixed.
Comment 13 Yusuke Suzuki 2021-04-03 19:57:22 PDT 
Created attachment 425122 [details]
Patch
Comment 14 Yusuke Suzuki 2021-04-03 21:52:08 PDT 
Created attachment 425124 [details]
Patch
Comment 15 Mark Lam 2021-04-05 10:15:45 PDT 
Comment on attachment 425121 [details]
Patch

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

> Source/JavaScriptCore/bytecode/PolyProtoAccessChain.cpp:80
> +    return adoptRef(*new PolyProtoAccessChain(WTFMove(chain)));

It's unfortunate that we are implementing a tryCreate() function here but we're going to crash if new fails.  Can we use a placement new using tryMalloc so that we can fail without crashing?
Comment 16 Mark Lam 2021-04-05 10:23:40 PDT 
(In reply to Mark Lam from comment #15)
> Comment on attachment 425121 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425121&action=review
> 
> > Source/JavaScriptCore/bytecode/PolyProtoAccessChain.cpp:80
> > +    return adoptRef(*new PolyProtoAccessChain(WTFMove(chain)));
> 
> It's unfortunate that we are implementing a tryCreate() function here but
> we're going to crash if new fails.  Can we use a placement new using
> tryMalloc so that we can fail without crashing?

Make that tryFastMalloc().
Comment 17 Yusuke Suzuki 2021-04-05 10:34:11 PDT 
Comment on attachment 425121 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/PolyProtoAccessChain.cpp:80
>>> +    return adoptRef(*new PolyProtoAccessChain(WTFMove(chain)));
>> 
>> It's unfortunate that we are implementing a tryCreate() function here but we're going to crash if new fails.  Can we use a placement new using tryMalloc so that we can fail without crashing?
> 
> Make that tryFastMalloc().

This tryCreate has meaning: it fails when the passed argument meets particular condition.
Returning nullptr when OOM will break our IC's semantics.
Comment 18 Mark Lam 2021-04-05 11:15:02 PDT 
Comment on attachment 425121 [details]
Patch

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

>>>> Source/JavaScriptCore/bytecode/PolyProtoAccessChain.cpp:80
>>>> +    return adoptRef(*new PolyProtoAccessChain(WTFMove(chain)));
>>> 
>>> It's unfortunate that we are implementing a tryCreate() function here but we're going to crash if new fails.  Can we use a placement new using tryMalloc so that we can fail without crashing?
>> 
>> Make that tryFastMalloc().
> 
> This tryCreate has meaning: it fails when the passed argument meets particular condition.
> Returning nullptr when OOM will break our IC's semantics.

ok, thanks for clarifying.
Comment 19 Yusuke Suzuki 2021-04-05 21:25:42 PDT 
Committed r275490 (236147@main): <https://commits.webkit.org/236147@main>
Comment 20 Radar WebKit Bug Importer 2021-04-05 21:26:16 PDT 
<rdar://problem/76251180>