Bug 224162 – [JSC] Shrink some of Vectors in JSC

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Shrink some of Vectors in JSC

[Yusuke Suzuki]

Created attachment 425110 [details]
Patch

[Yusuke Suzuki]

Created attachment 425111 [details]
Patch

[Yusuke Suzuki]

Created attachment 425113 [details]
Patch

[Sam Weinig]

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

[Yusuke Suzuki]

Created attachment 425115 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 425117 [details]
Patch

[Yusuke Suzuki]

Created attachment 425119 [details]
Patch

[Yusuke Suzuki]

Created attachment 425120 [details]
Patch

[Yusuke Suzuki]

Created attachment 425121 [details]
Patch

[Simon Fraser (smfr)]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 425122 [details]
Patch

[Yusuke Suzuki]

Created attachment 425124 [details]
Patch

[Mark Lam]

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?

[Mark Lam]

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

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Yusuke Suzuki]

Committed r275490 (236147@main): <https://commits.webkit.org/236147@main>

[Radar WebKit Bug Importer]

<rdar://problem/76251180>