Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable ES6IsConcatSpreadable under experimental #1198

Merged
merged 1 commit into from
Jun 28, 2016
Merged

Enable ES6IsConcatSpreadable under experimental #1198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions lib/Common/ConfigFlagsList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ PHASE(All)
PHASE(InlineCandidate)
PHASE(InlineHostCandidate)
PHASE(ScriptFunctionWithInlineCache)
PHASE(IsConcatSpreadableCache)
PHASE(Arena)
PHASE(ApplyUsage)
PHASE(ObjectHeaderInlining)
Expand Down Expand Up @@ -499,7 +500,12 @@ PHASE(All)
#define DEFAULT_CONFIG_ES6FunctionNameFull (false)
#endif
#define DEFAULT_CONFIG_ES6Generators (true)
#define DEFAULT_CONFIG_ES6IsConcatSpreadable (false)
#ifdef COMPILE_DISABLE_ES6IsConcatSpreadable
// If ES6IsConcatSpreadable needs to be disabled by compile flag, COMPILE_DISABLE_ES6IsConcatSpreadable should be false
#define DEFAULT_CONFIG_ES6IsConcatSpreadable (false)
#else
#define DEFAULT_CONFIG_ES6IsConcatSpreadable (false)
#endif
#define DEFAULT_CONFIG_ES6Math (true)
#ifdef COMPILE_DISABLE_ES6Module
// If ES6Module needs to be disabled by compile flag, DEFAULT_CONFIG_ES6Module should be false
Expand Down Expand Up @@ -952,7 +958,10 @@ FLAGPR (Boolean, ES6, ES7ExponentiationOperator, "Enable ES7 exponenti
FLAGPR_REGOVR_EXP(Boolean, ES6, ES7Builtins , "Enable ES7 built-ins" , DEFAULT_CONFIG_ES7Builtins)
FLAGPR (Boolean, ES6, ES7ValuesEntries , "Enable ES7 Object.values and Object.entries" , DEFAULT_CONFIG_ES7ValuesEntries)
FLAGPR (Boolean, ES6, ES7TrailingComma , "Enable ES7 trailing comma in function" , DEFAULT_CONFIG_ES7TrailingComma)
FLAGPR (Boolean, ES6, ES6IsConcatSpreadable , "Enable ES6 isConcatSpreadable Symbol" , DEFAULT_CONFIG_ES6IsConcatSpreadable)
#ifndef COMPILE_DISABLE_ES6IsConcatSpreadable
#define COMPILE_DISABLE_ES6IsConcatSpreadable 0
#endif
FLAGPR_REGOVR_EXP(Boolean, ES6, ES6IsConcatSpreadable , "Enable ES6 isConcatSpreadable Symbol" , DEFAULT_CONFIG_ES6IsConcatSpreadable)
FLAGPR (Boolean, ES6, ES6Math , "Enable ES6 Math extensions" , DEFAULT_CONFIG_ES6Math)

#ifndef COMPILE_DISABLE_ES6Module
Expand Down
4 changes: 4 additions & 0 deletions lib/Runtime/Base/ThreadContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,8 @@ class ThreadContext sealed :
typedef JsUtil::BaseDictionary<Js::Var, Js::IsInstInlineCache*, ArenaAllocator> IsInstInlineCacheListMapByFunction;
IsInstInlineCacheListMapByFunction isInstInlineCacheByFunction;

Js::IsConcatSpreadableCache isConcatSpreadableCache;

ArenaAllocator prototypeChainEnsuredToHaveOnlyWritableDataPropertiesAllocator;
DListBase<Js::ScriptContext *> prototypeChainEnsuredToHaveOnlyWritableDataPropertiesScriptContext;

Expand Down Expand Up @@ -841,6 +843,8 @@ class ThreadContext sealed :

UCrtC99MathApis* GetUCrtC99MathApis() { return &ucrtC99MathApis; }

Js::IsConcatSpreadableCache* GetIsConcatSpreadableCache() { return &isConcatSpreadableCache; }

#ifdef ENABLE_GLOBALIZATION
Js::DelayLoadWinRtString *GetWinRTStringLibrary();
#ifdef ENABLE_PROJECTION
Expand Down
66 changes: 66 additions & 0 deletions lib/Runtime/Language/InlineCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -964,6 +964,72 @@ namespace Js
void Unregister(ScriptContext * scriptContext);
};

// Two-entry Type-indexed circular cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you do any experimentation with different cache sizes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Tried 1-, 2-, and 3-entry caches. From 1 to 2 saw significant perf gain; from 2 to 3 no gain. Therefore chose 2-entry for simplicity.

In reply to: 68804491 [](ancestors = 68804491)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, good to know.

// cache IsConcatSpreadable() result unless user-defined [@@isConcatSpreadable] exists
class IsConcatSpreadableCache
{
Type *type0, *type1;
int lastAccess;
BOOL result0, result1;

public:
IsConcatSpreadableCache() :
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialized these as initialization list.

type0(nullptr),
type1(nullptr),
result0(FALSE),
result1(FALSE),
lastAccess(1)
{
}

bool TryGetIsConcatSpreadable(Type *type, _Out_ BOOL *result)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: changed to bool

{
Assert(type != nullptr);
Assert(result != nullptr);

*result = FALSE;
if (type0 == type)
{
*result = result0;
lastAccess = 0;
return true;
}

if (type1 == type)
{
*result = result1;
lastAccess = 1;
return true;
}

return false;
}

void CacheIsConcatSpreadable(Type *type, BOOL result)
{
Assert(type != nullptr);

if (lastAccess == 0)
{
type1 = type;
result1 = result;
lastAccess = 1;
}
else
{
type0 = type;
result0 = result;
lastAccess = 0;
}
}

void Invalidate()
{
type0 = nullptr;
type1 = nullptr;
}
};

#if defined(_M_IX86_OR_ARM32)
CompileAssert(sizeof(IsInstInlineCache) == 0x10);
#else
Expand Down
38 changes: 33 additions & 5 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1357,17 +1357,45 @@ namespace Js

RecyclableObject* instance = RecyclableObject::FromVar(instanceVar);
ScriptContext* scriptContext = instance->GetScriptContext();

if (!PHASE_OFF1(IsConcatSpreadableCachePhase))
{
BOOL retVal = FALSE;
Type *instanceType = instance->GetType();
IsConcatSpreadableCache *isConcatSpreadableCache = scriptContext->GetThreadContext()->GetIsConcatSpreadableCache();

if (isConcatSpreadableCache->TryGetIsConcatSpreadable(instanceType, &retVal))
{
OUTPUT_TRACE(Phase::IsConcatSpreadableCachePhase, _u("IsConcatSpreadableCache hit: %p\n"), instanceType);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: changed to use OUTPUT_TRACE

return retVal;
}

Var spreadable = nullptr;
BOOL hasUserDefinedSpreadable = JavascriptOperators::GetProperty(instance, instance, PropertyIds::_symbolIsConcatSpreadable, &spreadable, scriptContext);

if (hasUserDefinedSpreadable && spreadable != scriptContext->GetLibrary()->GetUndefined())
{
return JavascriptConversion::ToBoolean(spreadable, scriptContext);
}

retVal = JavascriptOperators::IsArray(instance);

if (!hasUserDefinedSpreadable)
{
OUTPUT_TRACE(Phase::IsConcatSpreadableCachePhase, _u("IsConcatSpreadableCache saved: %p\n"), instanceType);
isConcatSpreadableCache->CacheIsConcatSpreadable(instanceType, retVal);
}

return retVal;
}

Var spreadable = JavascriptOperators::GetProperty(instance, PropertyIds::_symbolIsConcatSpreadable, scriptContext);
if (spreadable != scriptContext->GetLibrary()->GetUndefined())
{
return JavascriptConversion::ToBoolean(spreadable, scriptContext);
}
if (JavascriptOperators::IsArray(instance))
{
return true;
}
return false;

return JavascriptOperators::IsArray(instance);
}

Var JavascriptOperators::OP_LdCustomSpreadIteratorList(Var aRight, ScriptContext* scriptContext)
Expand Down
45 changes: 31 additions & 14 deletions lib/Runtime/Library/JavascriptArray.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2972,6 +2972,11 @@ namespace Js
return;
}

if (length + idxDest > FiftyThirdPowerOfTwoMinusOne) // 2^53-1: from ECMA 22.1.3.1 Array.prototype.concat(...arguments)
{
JavascriptError::ThrowTypeError(scriptContext, JSERR_IllegalArraySizeAndLength);
}

RecyclableObject* itemObject = RecyclableObject::FromVar(aItem);
Var subItem;
uint32 lengthToUin32Max = length.IsSmallIndex() ? length.GetSmallIndex() : MaxArrayLength;
Expand Down Expand Up @@ -3044,7 +3049,7 @@ namespace Js
return true;
}

void JavascriptArray::ConcatIntArgs(JavascriptNativeIntArray* pDestArray, TypeId *remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext)
JavascriptArray* JavascriptArray::ConcatIntArgs(JavascriptNativeIntArray* pDestArray, TypeId *remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext)
{
uint idxDest = 0u;
for (uint idxArg = 0; idxArg < args.Info.Count; idxArg++)
Expand All @@ -3058,7 +3063,7 @@ namespace Js
if (!JavascriptNativeIntArray::Is(pDestArray)) // SetItem could convert pDestArray to a var array if aItem is not an integer if so fall back
{
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
return;
return pDestArray;
}
continue;
}
Expand All @@ -3073,13 +3078,11 @@ namespace Js
// Copying the last array forced a conversion, so switch over to the var version
// to finish.
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
return;
return pDestArray;
}
}
else
else if (!JavascriptArray::IsAnyArray(aItem) && remoteTypeIds[idxArg] != TypeIds_Array)
{
Assert(!JavascriptArray::IsAnyArray(aItem) && remoteTypeIds[idxArg] != TypeIds_Array);

if (TaggedInt::Is(aItem))
{
pDestArray->DirectSetItemAt(idxDest, TaggedInt::ToInt32(aItem));
Expand All @@ -3096,14 +3099,21 @@ namespace Js
}
++idxDest;
}
else
{
JavascriptArray *pVarDestArray = JavascriptNativeIntArray::ConvertToVarArray(pDestArray);
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest);
return pVarDestArray;
}
}
if (pDestArray->GetLength() != idxDest)
{
pDestArray->SetLength(idxDest);
}
return pDestArray;
}

void JavascriptArray::ConcatFloatArgs(JavascriptNativeFloatArray* pDestArray, TypeId *remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext)
JavascriptArray* JavascriptArray::ConcatFloatArgs(JavascriptNativeFloatArray* pDestArray, TypeId *remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext)
{
uint idxDest = 0u;
for (uint idxArg = 0; idxArg < args.Info.Count; idxArg++)
Expand All @@ -3119,7 +3129,7 @@ namespace Js
if (!JavascriptNativeFloatArray::Is(pDestArray)) // SetItem could convert pDestArray to a var array if aItem is not an integer if so fall back
{
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
return;
return pDestArray;
}
continue;
}
Expand All @@ -3133,18 +3143,25 @@ namespace Js
converted = CopyNativeIntArrayElementsToFloat(pDestArray, idxDest, pIntArray);
idxDest = idxDest + pIntArray->length;
}
else
else if (JavascriptNativeFloatArray::Is(aItem))
{
JavascriptNativeFloatArray* pItemArray = JavascriptNativeFloatArray::FromVar(aItem);
converted = CopyNativeFloatArrayElements(pDestArray, idxDest, pItemArray);
idxDest = idxDest + pItemArray->length;
}
else
{
JavascriptArray *pVarDestArray = JavascriptNativeFloatArray::ConvertToVarArray(pDestArray);
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest);
return pVarDestArray;
}

if (converted)
{
// Copying the last array forced a conversion, so switch over to the var version
// to finish.
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
return;
return pDestArray;
}
}
else
Expand All @@ -3167,6 +3184,8 @@ namespace Js
{
pDestArray->SetLength(idxDest);
}

return pDestArray;
}

bool JavascriptArray::BoxConcatItem(Var aItem, uint idxArg, ScriptContext *scriptContext)
Expand Down Expand Up @@ -3318,15 +3337,13 @@ namespace Js
{
JavascriptNativeIntArray *pIntArray = isArray ? JavascriptNativeIntArray::FromVar(pDestObj) : scriptContext->GetLibrary()->CreateNativeIntArray(cDestLength);
pIntArray->EnsureHead<int32>();
ConcatIntArgs(pIntArray, remoteTypeIds, args, scriptContext);
pDestArray = pIntArray;
pDestArray = ConcatIntArgs(pIntArray, remoteTypeIds, args, scriptContext);
}
else if (isFloat)
{
JavascriptNativeFloatArray *pFArray = isArray ? JavascriptNativeFloatArray::FromVar(pDestObj) : scriptContext->GetLibrary()->CreateNativeFloatArray(cDestLength);
pFArray->EnsureHead<double>();
ConcatFloatArgs(pFArray, remoteTypeIds, args, scriptContext);
pDestArray = pFArray;
pDestArray = ConcatFloatArgs(pFArray, remoteTypeIds, args, scriptContext);
}
else
{
Expand Down
5 changes: 3 additions & 2 deletions lib/Runtime/Library/JavascriptArray.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ namespace Js
static uint32 const MaxArrayLength = InvalidIndex;
static uint32 const MaxInitialDenseLength=1<<18;
static ushort const MergeSegmentsLengthHeuristics = 128; // If the length is less than MergeSegmentsLengthHeuristics then try to merge the segments
static uint64 const FiftyThirdPowerOfTwoMinusOne = 0x1FFFFFFFFFFFFF; // 2^53-1

static const Var MissingItem;
template<typename T> static T GetMissingItem();
Expand Down Expand Up @@ -759,10 +760,10 @@ namespace Js
static void ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start, BigIndex startIdxDest, BOOL firstPromotedItemIsSpreadable, BigIndex firstPromotedItemLength);
template<typename T>
static void ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start = 0, uint startIdxDest = 0u, BOOL FirstPromotedItemIsSpreadable = false, BigIndex FirstPromotedItemLength = 0u);
static void ConcatIntArgs(JavascriptNativeIntArray* pDestArray, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext);
static JavascriptArray* ConcatIntArgs(JavascriptNativeIntArray* pDestArray, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext);
static bool PromoteToBigIndex(BigIndex lhs, BigIndex rhs);
static bool PromoteToBigIndex(BigIndex lhs, uint32 rhs);
static void ConcatFloatArgs(JavascriptNativeFloatArray* pDestArray, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext);
static JavascriptArray* ConcatFloatArgs(JavascriptNativeFloatArray* pDestArray, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext);
private:
template<typename T=uint32>
static RecyclableObject* ArraySpeciesCreate(Var pThisArray, T length, ScriptContext* scriptContext, bool* pIsIntArray = nullptr, bool* pIsFloatArray = nullptr);
Expand Down
2 changes: 2 additions & 0 deletions lib/Runtime/Library/JavascriptLibrary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,8 @@ namespace Js
RecyclableObject* TryGetStringTemplateCallsiteObject(ParseNodePtr pnode);
RecyclableObject* TryGetStringTemplateCallsiteObject(RecyclableObject* callsite);

static void CheckAndInvalidateIsConcatSpreadableCache(PropertyId propertyId, ScriptContext *scriptContext);

#if DBG_DUMP
static const char16* GetStringTemplateCallsiteObjectKey(Var callsite);
#endif
Expand Down
9 changes: 9 additions & 0 deletions lib/Runtime/Library/JavascriptLibrary.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ namespace Js
return AddFunctionToLibraryObject(object, scriptContext->GetOrAddPropertyIdTracked(propertyName), functionInfo, length);
}

inline void JavascriptLibrary::CheckAndInvalidateIsConcatSpreadableCache(PropertyId propertyId, ScriptContext *scriptContext)
{
if (!PHASE_OFF1(IsConcatSpreadableCachePhase) && propertyId == PropertyIds::_symbolIsConcatSpreadable)
{
OUTPUT_TRACE(Phase::IsConcatSpreadableCachePhase, _u("IsConcatSpreadableCache invalidated\n"));
scriptContext->GetThreadContext()->GetIsConcatSpreadableCache()->Invalidate();
}
}

#if ENABLE_COPYONACCESS_ARRAY
template <>
inline void JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(const Var instance)
Expand Down
3 changes: 3 additions & 0 deletions lib/Runtime/Library/JavascriptProxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,9 @@ namespace Js
return FALSE;
JavascriptError::ThrowTypeError(scriptContext, JSERR_ErrorOnRevokedProxy, _u("get"));
}

RecyclableObject *target = this->target;

JavascriptFunction* getGetMethod = GetMethodHelper(PropertyIds::get, scriptContext);
Var getGetResult;
if (nullptr == getGetMethod || scriptContext->IsHeapEnumInProgress())
Expand Down
2 changes: 2 additions & 0 deletions lib/Runtime/Types/DictionaryTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,8 @@ namespace Js
bool throwIfNotExtensible = (flags & (PropertyOperation_ThrowIfNotExtensible | PropertyOperation_StrictMode)) != 0;
bool isForce = (flags & PropertyOperation_Force) != 0;

JavascriptLibrary::CheckAndInvalidateIsConcatSpreadableCache(propertyId, scriptContext);

Assert(propertyId != Constants::NoProperty);
PropertyRecord const* propertyRecord = scriptContext->GetPropertyName(propertyId);
if (propertyMap->TryGetReference(propertyRecord, &descriptor))
Expand Down
2 changes: 2 additions & 0 deletions lib/Runtime/Types/PathTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,8 @@ namespace Js
Assert(value != nullptr || IsInternalPropertyId(propertyId));
PropertyIndex index = PathTypeHandlerBase::GetPropertyIndex(propertyId);

JavascriptLibrary::CheckAndInvalidateIsConcatSpreadableCache(propertyId, instance->GetScriptContext());

if (index != Constants::NoSlot)
{
// If type is shared then the handler must be shared as well. This is a weaker invariant than in AddPropertyInternal,
Expand Down
2 changes: 2 additions & 0 deletions lib/Runtime/Types/SimpleDictionaryTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1302,6 +1302,8 @@ namespace Js
Assert(propertyId != Constants::NoProperty);
PropertyRecord const* propertyRecord = instance->GetScriptContext()->GetPropertyName(propertyId);

JavascriptLibrary::CheckAndInvalidateIsConcatSpreadableCache(propertyId, instance->GetScriptContext());

// It can be the case that propertyRecord is a symbol property and it has the same string description as
// another property which is in the propertyMap. If we are in a string-keyed type handler, the propertyMap
// will find that normal property when we call TryGetReference with the symbol propertyRecord. However,
Expand Down
Loading