224162 – [JSC] Shrink some of Vectors in JSC

RESOLVED FIXED 224162

[JSC] Shrink some of Vectors in JSC

https://bugs.webkit.org/show_bug.cgi?id=224162

Summary [JSC] Shrink some of Vectors in JSC

Yusuke Suzuki

Reported 2021-04-03 15:03:00 PDT

[JSC] Shrink some of Vectors in JSC

Attachments
Patch (29.45 KB, patch) 2021-04-03 15:06 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (29.58 KB, patch) 2021-04-03 15:09 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (30.37 KB, patch) 2021-04-03 16:14 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (34.65 KB, patch) 2021-04-03 16:40 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (34.67 KB, patch) 2021-04-03 17:32 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (63.07 KB, patch) 2021-04-03 18:21 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (75.80 KB, patch) 2021-04-03 18:44 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (77.82 KB, patch) 2021-04-03 18:51 PDT, Yusuke Suzuki	simon.fraser: review+ ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (77.86 KB, patch) 2021-04-03 19:57 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (77.86 KB, patch) 2021-04-03 21:52 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2021-04-03 15:06:55 PDT

Created attachment 425110 [details] Patch

Yusuke Suzuki

Comment 2 2021-04-03 15:09:27 PDT

Created attachment 425111 [details] Patch

Yusuke Suzuki

Comment 3 2021-04-03 16:14:55 PDT

Created attachment 425113 [details] Patch

Sam Weinig

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

Yusuke Suzuki

Comment 5 2021-04-03 16:40:03 PDT

Created attachment 425115 [details] Patch

Yusuke Suzuki

Comment 6 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.

Yusuke Suzuki

Comment 7 2021-04-03 17:32:34 PDT

Created attachment 425117 [details] Patch

Yusuke Suzuki

Comment 8 2021-04-03 18:21:46 PDT

Created attachment 425119 [details] Patch

Yusuke Suzuki

Comment 9 2021-04-03 18:44:43 PDT

Created attachment 425120 [details] Patch

Yusuke Suzuki

Comment 10 2021-04-03 18:51:55 PDT

Created attachment 425121 [details] Patch

Simon Fraser (smfr)

Comment 11 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

Yusuke Suzuki

Comment 12 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.

Yusuke Suzuki

Comment 13 2021-04-03 19:57:22 PDT

Created attachment 425122 [details] Patch

Yusuke Suzuki

Comment 14 2021-04-03 21:52:08 PDT

Created attachment 425124 [details] Patch

Mark Lam

Comment 15 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?

Mark Lam

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

Yusuke Suzuki

Comment 17 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.

Mark Lam

Comment 18 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.

Yusuke Suzuki

Comment 19 2021-04-05 21:25:42 PDT

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

Radar WebKit Bug Importer

Comment 20 2021-04-05 21:26:16 PDT

<rdar://problem/76251180>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component JavaScriptCore

Assignee

Yusuke Suzuki

Reported

2021-04-03 15:03 PDT

Modified

2021-04-05 21:26 PDT History

CC List

12 users Show

URL

Keywords InRadar

Depends on

Blocks