Skip to content

Commit f088a6d

Browse files
kunalspathakkunalspathak
committed
Allocate more elements for new Array()
With chakra-core#1363, I changed the allocation buckets to allocate space for 2 elements if user calls `new Array()` or size passed to Array ctor is 0. However there can be cases where user creates array using `new Array()` but then set elements to indices beyond size 2. With my change, we will go through slow path to set these elements. As part of the fix, revert the `new Array()` case to allocate same amount of space before my chakra-core#1363 changes. This doesn't change the `new Array(0) or (foo = 0; new Array(foo);` case and will continue to allocate space for just 2 elements. I have opened chakra-core#2324 to gather profile data of array's length which will help us specify the right length for such scenarios during array allocation in JIT. Removed dead code
1 parent 6a8a129 commit f088a6d

5 files changed

Lines changed: 51 additions & 64 deletions

File tree

‎lib/Backend/Lower.cpp‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3901,7 +3901,7 @@ Lowerer::GenerateProfiledNewScArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteI
39013901
}
39023902
GenerateArrayInfoIsNativeIntArrayTest(instr, arrayInfo, arrayInfoAddr, helperLabel);
39033903
Assert(Js::JavascriptNativeIntArray::GetOffsetOfArrayFlags() + sizeof(uint16) == Js::JavascriptNativeIntArray::GetOffsetOfArrayCallSiteIndex());
3904-
headOpnd = GenerateArrayAlloc<Js::JavascriptNativeIntArray>(instr, &size, arrayInfo, &isZeroed);
3904+
headOpnd = GenerateArrayLiteralsAlloc<Js::JavascriptNativeIntArray>(instr, &size, arrayInfo, &isZeroed);
39053905
const IR::AutoReuseOpnd autoReuseHeadOpnd(headOpnd, func);
39063906

39073907
GenerateMemInit(dstOpnd, Js::JavascriptNativeIntArray::GetOffsetOfWeakFuncRef(), IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicFunctionBodyWeakRef, m_func), instr, isZeroed);
@@ -3919,7 +3919,7 @@ Lowerer::GenerateProfiledNewScArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteI
39193919
}
39203920
GenerateArrayInfoIsNativeFloatAndNotIntArrayTest(instr, arrayInfo, arrayInfoAddr, helperLabel);
39213921
Assert(Js::JavascriptNativeFloatArray::GetOffsetOfArrayFlags() + sizeof(uint16) == Js::JavascriptNativeFloatArray::GetOffsetOfArrayCallSiteIndex());
3922-
headOpnd = GenerateArrayAlloc<Js::JavascriptNativeFloatArray>(instr, &size, arrayInfo, &isZeroed);
3922+
headOpnd = GenerateArrayLiteralsAlloc<Js::JavascriptNativeFloatArray>(instr, &size, arrayInfo, &isZeroed);
39233923
const IR::AutoReuseOpnd autoReuseHeadOpnd(headOpnd, func);
39243924

39253925
GenerateMemInit(dstOpnd, Js::JavascriptNativeFloatArray::GetOffsetOfWeakFuncRef(), IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicFunctionBodyWeakRef, m_func), instr, isZeroed);
@@ -3942,7 +3942,7 @@ Lowerer::GenerateProfiledNewScArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteI
39423942
return;
39433943
}
39443944
uint const offsetStart = sizeof(Js::SparseArraySegmentBase);
3945-
headOpnd = GenerateArrayAlloc<Js::JavascriptArray>(instr, &size, arrayInfo, &isZeroed);
3945+
headOpnd = GenerateArrayLiteralsAlloc<Js::JavascriptArray>(instr, &size, arrayInfo, &isZeroed);
39463946
const IR::AutoReuseOpnd autoReuseHeadOpnd(headOpnd, func);
39473947
for (; i < size; i++)
39483948
{
@@ -3997,7 +3997,22 @@ IR::JnHelperMethod GetArrayAllocMemHelper<Js::JavascriptNativeFloatArray>()
39973997

39983998
template <typename ArrayType>
39993999
IR::RegOpnd *
4000-
Lowerer::GenerateArrayAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed, bool isArrayObjCtor /* = false */)
4000+
Lowerer::GenerateArrayLiteralsAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed)
4001+
{
4002+
return GenerateArrayAllocHelper<ArrayType>(instr, psize, arrayInfo, pIsHeadSegmentZeroed, false /* isArrayObjCtor */, false /* isNoArgs */);
4003+
}
4004+
4005+
template <typename ArrayType>
4006+
IR::RegOpnd *
4007+
Lowerer::GenerateArrayObjectsAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed, bool isNoArgs)
4008+
{
4009+
return GenerateArrayAllocHelper<ArrayType>(instr, psize, arrayInfo, pIsHeadSegmentZeroed, true /* isArrayObjCtor */, isNoArgs);
4010+
}
4011+
4012+
4013+
template <typename ArrayType>
4014+
IR::RegOpnd *
4015+
Lowerer::GenerateArrayAllocHelper(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed, bool isArrayObjCtor, bool isNoArgs)
40014016
{
40024017
Func * func = this->m_func;
40034018
IR::RegOpnd * dstOpnd = instr->GetDst()->AsRegOpnd();
@@ -4016,7 +4031,8 @@ Lowerer::GenerateArrayAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteI
40164031
{
40174032
if (isArrayObjCtor)
40184033
{
4019-
arrayAllocSize = Js::JavascriptArray::DetermineAllocationSizeForArrayObjects<ArrayType, 0>(count, nullptr, &alignedHeadSegmentSize);
4034+
uint32 allocCount = isNoArgs ? Js::SparseArraySegmentBase::SMALL_CHUNK_SIZE : count;
4035+
arrayAllocSize = Js::JavascriptArray::DetermineAllocationSizeForArrayObjects<ArrayType, 0>(allocCount, nullptr, &alignedHeadSegmentSize);
40204036
}
40214037
else
40224038
{
@@ -4205,7 +4221,7 @@ Lowerer::GenerateArrayAlloc(IR::Instr *instr, IR::Opnd * arrayLenOpnd, Js::Array
42054221

42064222

42074223
void
4208-
Lowerer::GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteInfo * arrayInfo, intptr_t arrayInfoAddr, intptr_t weakFuncRef, uint32 length, IR::LabelInstr* labelDone)
4224+
Lowerer::GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteInfo * arrayInfo, intptr_t arrayInfoAddr, intptr_t weakFuncRef, uint32 length, IR::LabelInstr* labelDone, bool isNoArgs)
42094225
{
42104226
if (PHASE_OFF(Js::ArrayCtorFastPathPhase, m_func))
42114227
{
@@ -4223,7 +4239,7 @@ Lowerer::GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSi
42234239
{
42244240
GenerateArrayInfoIsNativeIntArrayTest(instr, arrayInfo, arrayInfoAddr, helperLabel);
42254241
Assert(Js::JavascriptNativeIntArray::GetOffsetOfArrayFlags() + sizeof(uint16) == Js::JavascriptNativeIntArray::GetOffsetOfArrayCallSiteIndex());
4226-
headOpnd = GenerateArrayAlloc<Js::JavascriptNativeIntArray>(instr, &size, arrayInfo, &isZeroed, true);
4242+
headOpnd = GenerateArrayObjectsAlloc<Js::JavascriptNativeIntArray>(instr, &size, arrayInfo, &isZeroed, isNoArgs);
42274243

42284244
GenerateMemInit(dstOpnd, Js::JavascriptNativeIntArray::GetOffsetOfArrayCallSiteIndex(), IR::IntConstOpnd::New(profileId, TyUint16, func, true), instr, isZeroed);
42294245
GenerateMemInit(dstOpnd, Js::JavascriptNativeIntArray::GetOffsetOfWeakFuncRef(), IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicFunctionBodyWeakRef, m_func), instr, isZeroed);
@@ -4237,7 +4253,7 @@ Lowerer::GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSi
42374253
{
42384254
GenerateArrayInfoIsNativeFloatAndNotIntArrayTest(instr, arrayInfo, arrayInfoAddr, helperLabel);
42394255
Assert(Js::JavascriptNativeFloatArray::GetOffsetOfArrayFlags() + sizeof(uint16) == Js::JavascriptNativeFloatArray::GetOffsetOfArrayCallSiteIndex());
4240-
headOpnd = GenerateArrayAlloc<Js::JavascriptNativeFloatArray>(instr, &size, arrayInfo, &isZeroed, true);
4256+
headOpnd = GenerateArrayObjectsAlloc<Js::JavascriptNativeFloatArray>(instr, &size, arrayInfo, &isZeroed, isNoArgs);
42414257

42424258
GenerateMemInit(dstOpnd, Js::JavascriptNativeFloatArray::GetOffsetOfArrayCallSiteIndex(), IR::IntConstOpnd::New(profileId, TyUint16, func, true), instr, isZeroed);
42434259
GenerateMemInit(dstOpnd, Js::JavascriptNativeFloatArray::GetOffsetOfWeakFuncRef(), IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicFunctionBodyWeakRef, m_func), instr, isZeroed);
@@ -4256,7 +4272,7 @@ Lowerer::GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSi
42564272
else
42574273
{
42584274
uint const offsetStart = sizeof(Js::SparseArraySegmentBase);
4259-
headOpnd = GenerateArrayAlloc<Js::JavascriptArray>(instr, &size, arrayInfo, &isZeroed, true);
4275+
headOpnd = GenerateArrayObjectsAlloc<Js::JavascriptArray>(instr, &size, arrayInfo, &isZeroed, isNoArgs);
42604276
for (uint i = 0; i < size; i++)
42614277
{
42624278
GenerateMemInit(
@@ -4407,7 +4423,7 @@ Lowerer::GenerateProfiledNewScIntArrayFastPath(IR::Instr *instr, Js::ArrayCallSi
44074423
bool isHeadSegmentZeroed;
44084424
IR::RegOpnd * dstOpnd = instr->GetDst()->AsRegOpnd();
44094425
Assert(Js::JavascriptNativeIntArray::GetOffsetOfArrayFlags() + sizeof(uint16) == Js::JavascriptNativeIntArray::GetOffsetOfArrayCallSiteIndex());
4410-
IR::RegOpnd * headOpnd = GenerateArrayAlloc<Js::JavascriptNativeIntArray>(instr, &size, arrayInfo, &isHeadSegmentZeroed);
4426+
IR::RegOpnd * headOpnd = GenerateArrayLiteralsAlloc<Js::JavascriptNativeIntArray>(instr, &size, arrayInfo, &isHeadSegmentZeroed);
44114427
const IR::AutoReuseOpnd autoReuseHeadOpnd(headOpnd, func);
44124428

44134429
GenerateMemInit(dstOpnd, Js::JavascriptNativeIntArray::GetOffsetOfWeakFuncRef(), IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicMisc, m_func), instr, isHeadSegmentZeroed);
@@ -4479,7 +4495,7 @@ Lowerer::GenerateProfiledNewScFloatArrayFastPath(IR::Instr *instr, Js::ArrayCall
44794495
bool isHeadSegmentZeroed;
44804496
IR::RegOpnd * dstOpnd = instr->GetDst()->AsRegOpnd();
44814497
Assert(Js::JavascriptNativeFloatArray::GetOffsetOfArrayFlags() + sizeof(uint16) == Js::JavascriptNativeFloatArray::GetOffsetOfArrayCallSiteIndex());
4482-
IR::RegOpnd * headOpnd = GenerateArrayAlloc<Js::JavascriptNativeFloatArray>(instr, &size, arrayInfo, &isHeadSegmentZeroed);
4498+
IR::RegOpnd * headOpnd = GenerateArrayLiteralsAlloc<Js::JavascriptNativeFloatArray>(instr, &size, arrayInfo, &isHeadSegmentZeroed);
44834499
const IR::AutoReuseOpnd autoReuseHeadOpnd(headOpnd, func);
44844500

44854501
GenerateMemInit(dstOpnd, Js::JavascriptNativeFloatArray::GetOffsetOfWeakFuncRef(), IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicFunctionBodyWeakRef, m_func), instr, isHeadSegmentZeroed);
@@ -5478,7 +5494,7 @@ Lowerer::LowerNewScObjArray(IR::Instr *newObjInstr)
54785494
int32 length = linkSym->GetIntConstValue();
54795495
if (length >= 0 && length <= upperBoundValue)
54805496
{
5481-
GenerateProfiledNewScObjArrayFastPath(newObjInstr, arrayInfo, arrayInfoAddr, weakFuncRef, (uint32)length, labelDone);
5497+
GenerateProfiledNewScObjArrayFastPath(newObjInstr, arrayInfo, arrayInfoAddr, weakFuncRef, (uint32)length, labelDone, false);
54825498
}
54835499
else
54845500
{
@@ -5596,7 +5612,7 @@ Lowerer::LowerNewScObjArrayNoArg(IR::Instr *newObjInstr)
55965612
}
55975613

55985614
IR::LabelInstr *labelDone = IR::LabelInstr::New(Js::OpCode::Label, func);
5599-
GenerateProfiledNewScObjArrayFastPath(newObjInstr, arrayInfo, arrayInfoAddr, weakFuncRef, 0, labelDone);
5615+
GenerateProfiledNewScObjArrayFastPath(newObjInstr, arrayInfo, arrayInfoAddr, weakFuncRef, 0, labelDone, true);
56005616
newObjInstr->InsertAfter(labelDone);
56015617

56025618
m_lowererMD.LoadHelperArgument(newObjInstr, IR::AddrOpnd::New(weakFuncRef, IR::AddrOpndKindDynamicFunctionBodyWeakRef, func));

‎lib/Backend/Lower.h‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -520,11 +520,16 @@ class Lowerer
520520
void GenerateRecyclerAlloc(IR::JnHelperMethod allocHelper, size_t allocSize, IR::RegOpnd* newObjDst, IR::Instr* insertionPointInstr, bool inOpHelper = false);
521521

522522
template <typename ArrayType>
523-
IR::RegOpnd * GenerateArrayAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed, bool isArrayObjCtor = false);
523+
IR::RegOpnd * GenerateArrayAllocHelper(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed, bool isArrayObjCtor, bool isNoArgs);
524+
template <typename ArrayType>
525+
IR::RegOpnd * GenerateArrayLiteralsAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed);
526+
template <typename ArrayType>
527+
IR::RegOpnd * GenerateArrayObjectsAlloc(IR::Instr *instr, uint32 * psize, Js::ArrayCallSiteInfo * arrayInfo, bool * pIsHeadSegmentZeroed, bool isNoArgs);
528+
524529
template <typename ArrayType>
525530
IR::RegOpnd * GenerateArrayAlloc(IR::Instr *instr, IR::Opnd * sizeOpnd, Js::ArrayCallSiteInfo * arrayInfo);
526531

527-
void GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteInfo * arrayInfo, intptr_t arrayInfoAddr, intptr_t weakFuncRef, uint32 length, IR::LabelInstr* labelDone);
532+
void GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteInfo * arrayInfo, intptr_t arrayInfoAddr, intptr_t weakFuncRef, uint32 length, IR::LabelInstr* labelDone, bool isNoArgs);
528533

529534
template <typename ArrayType>
530535
void GenerateProfiledNewScObjArrayFastPath(IR::Instr *instr, Js::ArrayCallSiteInfo * arrayInfo, intptr_t arrayInfoAddr, intptr_t weakFuncRef, IR::LabelInstr* helperLabel, IR::LabelInstr* labelDone, IR::Opnd* lengthOpnd, uint32 offsetOfCallSiteIndex, uint32 offsetOfWeakFuncRef);

‎lib/Runtime/Library/JavascriptArray.cpp‎

Lines changed: 13 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,15 +1109,8 @@ namespace Js
11091109

11101110
if (callInfo.Count < 2)
11111111
{
1112-
if (pNew == nullptr)
1113-
{
1114-
// No arguments passed to Array(), so create with the default size (0).
1115-
pNew = CreateArrayFromConstructor(function, 0, scriptContext);
1116-
}
1117-
else
1118-
{
1119-
pNew->SetLength((uint32)0);
1120-
}
1112+
// No arguments passed to Array(), so create with the default size (0).
1113+
pNew = CreateArrayFromConstructorNoArg(function, scriptContext);
11211114

11221115
return isCtorSuperCall ?
11231116
JavascriptOperators::OrdinaryCreateFromConstructor(RecyclableObject::FromVar(newTarget), pNew, nullptr, scriptContext) :
@@ -1138,14 +1131,7 @@ namespace Js
11381131
JavascriptError::ThrowRangeError(scriptContext, JSERR_ArrayLengthConstructIncorrect);
11391132
}
11401133

1141-
if (pNew == nullptr)
1142-
{
1143-
pNew = CreateArrayFromConstructor(function, elementCount, scriptContext);
1144-
}
1145-
else
1146-
{
1147-
pNew->SetLength(elementCount);
1148-
}
1134+
pNew = CreateArrayFromConstructor(function, elementCount, scriptContext);
11491135
}
11501136
else if (JavascriptNumber::Is_NoTaggedIntCheck(firstArgument))
11511137
{
@@ -1157,14 +1143,7 @@ namespace Js
11571143
JavascriptError::ThrowRangeError(scriptContext, JSERR_ArrayLengthConstructIncorrect);
11581144
}
11591145

1160-
if (pNew == nullptr)
1161-
{
1162-
pNew = CreateArrayFromConstructor(function, uvalue, scriptContext);
1163-
}
1164-
else
1165-
{
1166-
pNew->SetLength(uvalue);
1167-
}
1146+
pNew = CreateArrayFromConstructor(function, uvalue, scriptContext);
11681147
}
11691148
else
11701149
{
@@ -1174,10 +1153,7 @@ namespace Js
11741153
// Set first element as the passed Var
11751154
//
11761155

1177-
if (pNew == nullptr)
1178-
{
1179-
pNew = CreateArrayFromConstructor(function, 1, scriptContext);
1180-
}
1156+
pNew = CreateArrayFromConstructor(function, 1, scriptContext);
11811157

11821158
JavascriptOperators::SetItem(pNew, pNew, 0u, firstArgument, scriptContext, PropertyOperation_ThrowIfNotExtensible);
11831159

@@ -1193,22 +1169,7 @@ namespace Js
11931169
{
11941170
// Called with a list of initial element values.
11951171
// Create an array of the appropriate length and walk the list.
1196-
1197-
if (pNew == nullptr)
1198-
{
1199-
pNew = CreateArrayFromConstructor(function, callInfo.Count - 1, scriptContext);
1200-
}
1201-
else
1202-
{
1203-
// If we were passed an uninitialized JavascriptArray as the this argument,
1204-
// we need to set the length. We should do this _after_ setting the
1205-
// elements as the array may have side effects such as a setter for property
1206-
// named '0' which would make the previous length of the array observable.
1207-
// Note: We don't support this case now as the DirectSetItemAt calls in FillFromArgs
1208-
// will not call the setter. Need to refactor that method.
1209-
pNew->SetLength(callInfo.Count - 1);
1210-
}
1211-
1172+
pNew = CreateArrayFromConstructor(function, callInfo.Count - 1, scriptContext);
12121173
pNew->JavascriptArray::FillFromArgs(callInfo.Count - 1, 0, args.Values);
12131174
}
12141175

@@ -1228,10 +1189,14 @@ namespace Js
12281189
// Note: We need to use the library from the ScriptContext of the constructor, not the currently executing function.
12291190
// This is for the case where a built-in @@create method from a different JavascriptLibrary is installed on
12301191
// constructor.
1231-
JavascriptArray* arr = library->CreateArray(length);
1192+
return library->CreateArray(length);
1193+
}
12321194

1233-
return arr;
1234-
}
1195+
JavascriptArray* JavascriptArray::CreateArrayFromConstructorNoArg(RecyclableObject* constructor, ScriptContext* scriptContext)
1196+
{
1197+
JavascriptLibrary* library = constructor->GetLibrary();
1198+
return library->CreateArray();
1199+
}
12351200

12361201
#if ENABLE_PROFILE_INFO
12371202
Var JavascriptArray::ProfiledNewInstanceNoArg(RecyclableObject *function, ScriptContext *scriptContext, ArrayCallSiteInfo *arrayInfo, RecyclerWeakReference<FunctionBody> *weakFuncRef)

‎lib/Runtime/Library/JavascriptArray.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,7 @@ namespace Js
379379
}
380380

381381
static JavascriptArray* CreateArrayFromConstructor(RecyclableObject* constructor, uint32 length, ScriptContext* scriptContext);
382+
static JavascriptArray* CreateArrayFromConstructorNoArg(RecyclableObject* constructor, ScriptContext* scriptContext);
382383

383384
template<typename unitType, typename className>
384385
static className* New(Recycler* recycler, DynamicType* arrayType);

‎lib/Runtime/Library/JavascriptArray.inl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ namespace Js
158158
size_t allocationPlusSize;
159159
uint alignedInlineElementSlots;
160160
DetermineAllocationSizeForArrayObjects<className, 0>(
161-
0,
161+
SparseArraySegmentBase::SMALL_CHUNK_SIZE,
162162
&allocationPlusSize,
163163
&alignedInlineElementSlots);
164164
return RecyclerNewPlusZ(recycler, allocationPlusSize, className, type, alignedInlineElementSlots);

0 commit comments

Comments
 (0)