Skip to content

Commit d08f5fe

Browse files
pleathpleath
committed
[MERGE chakra-core#841] Fix optimized element access that incorrectly reports that the instance has no such property
Merge pull request chakra-core#841 from pleath:isjsnative2 Reimplement shortcut in element access on unknown property name. The optimization requires a prototype chain walk to confirm that the property being looked for can't be found on an object such as a CEO that may use names unknown to the JS engine, or that, like a proxy, may require that the lookup be done regardless. For now the prototype walk is done on every attempted access where no property record exists for the index name. This could be optimized with a scheme similar to the one we use for non-writable properties in the prototype chain, but I'd like to see a needful use case before I saddle us with another registration mechanism. Benchmarks are flat.
2 parents b9991bd + b2ce371 commit d08f5fe

3 files changed

Lines changed: 155 additions & 55 deletions

File tree

‎lib/Common/ConfigFlagsList.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,7 @@ PHASE(All)
289289
PHASE(ObjectHeaderInliningForConstructors)
290290
PHASE(ObjectHeaderInliningForObjectLiterals)
291291
PHASE(ObjectHeaderInliningForEmptyObjects)
292+
PHASE(OptUnknownElementName)
292293
#if DBG_DUMP
293294
PHASE(TypePropertyCache)
294295
PHASE(InlineSlots)

‎lib/Runtime/Language/JavascriptOperators.cpp‎

Lines changed: 150 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,8 @@ namespace Js
7676
}
7777
}
7878

79-
IndexType GetIndexType(Var indexVar, ScriptContext* scriptContext, uint32* index, PropertyRecord const ** propertyRecord, JavascriptString ** propertyNameString, bool createIfNotFound, bool preferJavascriptStringOverPropertyRecord)
79+
IndexType GetIndexTypeFromPrimitive(Var indexVar, ScriptContext* scriptContext, uint32* index, PropertyRecord const ** propertyRecord, JavascriptString ** propertyNameString, bool createIfNotFound, bool preferJavascriptStringOverPropertyRecord)
8080
{
81-
indexVar = JavascriptConversion::ToPrimitive(indexVar, JavascriptHint::HintString, scriptContext);
82-
8381
// CONSIDER: Only OP_SetElementI and OP_GetElementI use and take advantage of the
8482
// IndexType_JavascriptString result. Consider modifying other callers of GetIndexType to take
8583
// advantage of non-interned property strings where appropriate.
@@ -142,7 +140,18 @@ namespace Js
142140
}
143141
}
144142

145-
IndexType GetIndexType(Var indexVar, ScriptContext* scriptContext, uint32* index, PropertyRecord const ** propertyRecord, bool createIfNotFound)
143+
IndexType GetIndexTypeFromPrimitive(Var indexVar, ScriptContext* scriptContext, uint32* index, PropertyRecord const ** propertyRecord, bool createIfNotFound)
144+
{
145+
return GetIndexTypeFromPrimitive(indexVar, scriptContext, index, propertyRecord, nullptr, createIfNotFound, false);
146+
}
147+
148+
IndexType GetIndexType(Var& indexVar, ScriptContext* scriptContext, uint32* index, PropertyRecord const ** propertyRecord, JavascriptString ** propertyNameString, bool createIfNotFound, bool preferJavascriptStringOverPropertyRecord)
149+
{
150+
indexVar = JavascriptConversion::ToPrimitive(indexVar, JavascriptHint::HintString, scriptContext);
151+
return GetIndexTypeFromPrimitive(indexVar, scriptContext, index, propertyRecord, propertyNameString, createIfNotFound, preferJavascriptStringOverPropertyRecord);
152+
}
153+
154+
IndexType GetIndexType(Var& indexVar, ScriptContext* scriptContext, uint32* index, PropertyRecord const ** propertyRecord, bool createIfNotFound)
146155
{
147156
return GetIndexType(indexVar, scriptContext, index, propertyRecord, nullptr, createIfNotFound, false);
148157
}
@@ -434,11 +443,10 @@ namespace Js
434443
try
435444
{
436445
Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
446+
IndexType indexType = GetIndexType(index, scriptContext, &indexVal, &propertyRecord, false);
437447

438448
// For JS Objects, don't create the propertyId if not already added
439-
bool createIfNotFound = !IsJsNativeObject(object) ||
440-
(DynamicType::Is(object->GetTypeId()) && static_cast<DynamicObject*>(object)->GetTypeHandler()->IsStringTypeHandler()) || JavascriptProxy::Is(object);
441-
if (GetIndexType(index, scriptContext, &indexVal, &propertyRecord, createIfNotFound) == IndexType_Number)
449+
if (indexType == IndexType_Number)
442450
{
443451
// In edge mode, we don't need to worry about the special "unknown" behavior. If the item is not available from Get,
444452
// just return undefined.
@@ -450,26 +458,35 @@ namespace Js
450458
return scriptContext->GetLibrary()->GetUndefinedDisplayString();
451459
}
452460
}
453-
else if (propertyRecord == nullptr)
461+
else
454462
{
455-
Assert(IsJsNativeObject(object));
463+
Assert(indexType == IndexType_PropertyId);
464+
if (propertyRecord == nullptr && !JavascriptOperators::CanShortcutOnUnknownPropertyName(object))
465+
{
466+
indexType = GetIndexTypeFromPrimitive(index, scriptContext, &indexVal, &propertyRecord, true);
467+
Assert(indexType == IndexType_PropertyId);
468+
Assert(propertyRecord != nullptr);
469+
}
456470

471+
if (propertyRecord != nullptr)
472+
{
473+
if (!JavascriptOperators::GetProperty(instance, object, propertyRecord->GetPropertyId(), &member, scriptContext))
474+
{
475+
// If the instance doesn't have the property, typeof result is "undefined".
476+
threadContext->CheckAndResetImplicitCallAccessorFlag();
477+
threadContext->AddImplicitCallFlags(savedImplicitCallFlags);
478+
return scriptContext->GetLibrary()->GetUndefinedDisplayString();
479+
}
480+
}
481+
else
482+
{
457483
#if DBG
458-
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
459-
PropertyRecord const * debugPropertyRecord;
460-
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
461-
AssertMsg(!JavascriptOperators::GetProperty(instance, object, debugPropertyRecord->GetPropertyId(), &member, scriptContext), "how did this property come? See OS Bug 2727708 if you see this come from the web");
484+
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
485+
PropertyRecord const * debugPropertyRecord;
486+
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
487+
AssertMsg(!JavascriptOperators::GetProperty(instance, object, debugPropertyRecord->GetPropertyId(), &member, scriptContext), "how did this property come? See OS Bug 2727708 if you see this come from the web");
462488
#endif
463489

464-
// If the instance doesn't have the property, typeof result is "undefined".
465-
threadContext->CheckAndResetImplicitCallAccessorFlag();
466-
threadContext->AddImplicitCallFlags(savedImplicitCallFlags);
467-
return scriptContext->GetLibrary()->GetUndefinedDisplayString();
468-
}
469-
else
470-
{
471-
if (!JavascriptOperators::GetProperty(instance, object, propertyRecord->GetPropertyId(), &member, scriptContext))
472-
{
473490
// If the instance doesn't have the property, typeof result is "undefined".
474491
threadContext->CheckAndResetImplicitCallAccessorFlag();
475492
threadContext->AddImplicitCallFlags(savedImplicitCallFlags);
@@ -3118,29 +3135,37 @@ namespace Js
31183135

31193136
uint32 indexVal;
31203137
PropertyRecord const * propertyRecord;
3121-
bool createIfNotFound = (DynamicType::Is(object->GetTypeId()) &&
3122-
static_cast<DynamicObject*>(object)->GetTypeHandler()->IsStringTypeHandler()) ||
3123-
JavascriptProxy::Is(object);
3124-
if (GetIndexType(index, scriptContext, &indexVal, &propertyRecord, createIfNotFound) == IndexType_Number)
3138+
IndexType indexType = GetIndexType(index, scriptContext, &indexVal, &propertyRecord, false);
3139+
3140+
if (indexType == IndexType_Number)
31253141
{
31263142
return HasItem(object, indexVal);
31273143
}
3128-
else if (propertyRecord == nullptr)
3144+
else
31293145
{
3130-
Assert(IsJsNativeObject(object));
3146+
Assert(indexType == IndexType_PropertyId);
3147+
if (propertyRecord == nullptr && !JavascriptOperators::CanShortcutOnUnknownPropertyName(object))
3148+
{
3149+
indexType = GetIndexTypeFromPrimitive(index, scriptContext, &indexVal, &propertyRecord, true);
3150+
Assert(indexType == IndexType_PropertyId);
3151+
Assert(propertyRecord != nullptr);
3152+
}
31313153

3154+
if (propertyRecord != nullptr)
3155+
{
3156+
return HasProperty(object, propertyRecord->GetPropertyId());
3157+
}
3158+
else
3159+
{
31323160
#if DBG
3133-
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
3134-
PropertyRecord const * debugPropertyRecord;
3135-
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
3136-
AssertMsg(!JavascriptOperators::HasProperty(object, debugPropertyRecord->GetPropertyId()), "how did this property come? See OS Bug 2727708 if you see this come from the web");
3161+
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
3162+
PropertyRecord const * debugPropertyRecord;
3163+
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
3164+
AssertMsg(!JavascriptOperators::HasProperty(object, debugPropertyRecord->GetPropertyId()), "how did this property come? See OS Bug 2727708 if you see this come from the web");
31373165
#endif
31383166

3139-
return FALSE;
3140-
}
3141-
else
3142-
{
3143-
return HasProperty(object, propertyRecord->GetPropertyId());
3167+
return FALSE;
3168+
}
31443169
}
31453170
}
31463171

@@ -3700,9 +3725,7 @@ namespace Js
37003725
JavascriptString * propertyNameString;
37013726
Var value;
37023727

3703-
bool createIfNotFound = !IsJsNativeObject(object);
3704-
3705-
IndexType indexType = GetIndexType(index, scriptContext, &indexVal, &propertyRecord, &propertyNameString, createIfNotFound, true);
3728+
IndexType indexType = GetIndexType(index, scriptContext, &indexVal, &propertyRecord, &propertyNameString, false, true);
37063729

37073730
if (indexType == IndexType_Number)
37083731
{
@@ -3720,15 +3743,30 @@ namespace Js
37203743
}
37213744
else
37223745
{
3723-
// We called GetIndexType with preferJavascriptString as true, so we mush have a propertyRecord
37243746
Assert(indexType == IndexType_PropertyId);
3747+
if (propertyRecord == nullptr && !JavascriptOperators::CanShortcutOnUnknownPropertyName(object))
3748+
{
3749+
indexType = GetIndexTypeFromPrimitive(index, scriptContext, &indexVal, &propertyRecord, &propertyNameString, true, true);
3750+
Assert(indexType == IndexType_PropertyId);
3751+
Assert(propertyRecord != nullptr);
3752+
}
37253753

3726-
Assert(propertyRecord);
3727-
3728-
if (JavascriptOperators::GetPropertyWPCache(receiver, object, propertyRecord->GetPropertyId(), &value, scriptContext, nullptr))
3754+
if (propertyRecord != nullptr)
37293755
{
3730-
return value;
3756+
if (JavascriptOperators::GetPropertyWPCache(receiver, object, propertyRecord->GetPropertyId(), &value, scriptContext, nullptr))
3757+
{
3758+
return value;
3759+
}
3760+
}
3761+
#if DBG
3762+
else
3763+
{
3764+
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
3765+
PropertyRecord const * debugPropertyRecord;
3766+
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
3767+
AssertMsg(!JavascriptOperators::GetProperty(receiver, object, debugPropertyRecord->GetPropertyId(), &value, scriptContext), "how did this property come? See OS Bug 2727708 if you see this come from the web");
37313768
}
3769+
#endif
37323770
}
37333771

37343772
return scriptContext->GetMissingItemResult();
@@ -3911,23 +3949,30 @@ namespace Js
39113949
PropertyRecord const * propertyRecord;
39123950
Var value = NULL;
39133951
BOOL hasProperty = FALSE;
3914-
bool createIfNotFound = !IsJsNativeObject(object) ||
3915-
(DynamicType::Is(object->GetTypeId()) && static_cast<DynamicObject*>(object)->GetTypeHandler()->IsStringTypeHandler()) || JavascriptProxy::Is(object);
3952+
IndexType indexType = GetIndexType(index, scriptContext, &indexVal, &propertyRecord, false);
39163953

3917-
if (GetIndexType(index, scriptContext, &indexVal, &propertyRecord, createIfNotFound) == IndexType_Number)
3954+
if (indexType == IndexType_Number)
39183955
{
39193956
hasProperty = JavascriptOperators::GetItemReference(instance, object, indexVal, &value, scriptContext);
39203957
}
39213958
else
39223959
{
3960+
Assert(indexType == IndexType_PropertyId);
3961+
3962+
if (propertyRecord == nullptr && !JavascriptOperators::CanShortcutOnUnknownPropertyName(object))
3963+
{
3964+
indexType = GetIndexTypeFromPrimitive(index, scriptContext, &indexVal, &propertyRecord, true);
3965+
Assert(indexType == IndexType_PropertyId);
3966+
Assert(propertyRecord != nullptr);
3967+
}
3968+
39233969
if (propertyRecord != nullptr)
39243970
{
39253971
hasProperty = JavascriptOperators::GetPropertyReference(instance, object, propertyRecord->GetPropertyId(), &value, scriptContext, NULL);
39263972
}
39273973
#if DBG
39283974
else
39293975
{
3930-
Assert(IsJsNativeObject(object));
39313976
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
39323977
PropertyRecord const * debugPropertyRecord;
39333978
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
@@ -4765,24 +4810,30 @@ namespace Js
47654810
uint32 indexVal;
47664811
PropertyRecord const * propertyRecord;
47674812
BOOL result = TRUE;
4813+
IndexType indexType = GetIndexType(index, scriptContext, &indexVal, &propertyRecord, false);
47684814

4769-
bool createIfNotFound = !IsJsNativeObject(object) ||
4770-
(DynamicType::Is(object->GetTypeId()) && static_cast<DynamicObject*>(object)->GetTypeHandler()->IsStringTypeHandler()) || JavascriptProxy::Is(object);
4771-
4772-
if (GetIndexType(index, scriptContext, &indexVal, &propertyRecord, createIfNotFound) == IndexType_Number)
4815+
if (indexType == IndexType_Number)
47734816
{
47744817
result = JavascriptOperators::DeleteItem(object, indexVal, propertyOperationFlags);
47754818
}
47764819
else
47774820
{
4778-
if (propertyRecord)
4821+
Assert(indexType == IndexType_PropertyId);
4822+
4823+
if (propertyRecord == nullptr && !JavascriptOperators::CanShortcutOnUnknownPropertyName(object))
4824+
{
4825+
indexType = GetIndexTypeFromPrimitive(index, scriptContext, &indexVal, &propertyRecord, true);
4826+
Assert(indexType == IndexType_PropertyId);
4827+
Assert(propertyRecord != nullptr);
4828+
}
4829+
4830+
if (propertyRecord != nullptr)
47794831
{
47804832
result = JavascriptOperators::DeleteProperty(object, propertyRecord->GetPropertyId(), propertyOperationFlags);
47814833
}
47824834
#if DBG
47834835
else
47844836
{
4785-
Assert(IsJsNativeObject(object));
47864837
JavascriptString* indexStr = JavascriptConversion::ToString(index, scriptContext);
47874838
PropertyRecord const * debugPropertyRecord;
47884839
scriptContext->GetOrAddPropertyRecord(indexStr->GetString(), indexStr->GetLength(), &debugPropertyRecord);
@@ -4977,6 +5028,50 @@ namespace Js
49775028
}
49785029
}
49795030

5031+
bool JavascriptOperators::CanShortcutOnUnknownPropertyName(RecyclableObject *instance)
5032+
{
5033+
if (!CanShortcutInstanceOnUnknownPropertyName(instance))
5034+
{
5035+
return false;
5036+
}
5037+
return CanShortcutPrototypeChainOnUnknownPropertyName(instance->GetPrototype());
5038+
}
5039+
5040+
bool JavascriptOperators::CanShortcutInstanceOnUnknownPropertyName(RecyclableObject *instance)
5041+
{
5042+
if (PHASE_OFF1(Js::OptUnknownElementNamePhase))
5043+
{
5044+
return false;
5045+
}
5046+
5047+
if (JavascriptProxy::Is(instance) || instance->IsExternal())
5048+
{
5049+
return false;
5050+
}
5051+
if (DynamicType::Is(instance->GetTypeId()) &&
5052+
static_cast<DynamicObject*>(instance)->GetTypeHandler()->IsStringTypeHandler())
5053+
{
5054+
return false;
5055+
}
5056+
return !(instance->HasDeferredTypeHandler() &&
5057+
JavascriptFunction::Is(instance) &&
5058+
JavascriptFunction::FromVar(instance)->IsExternalFunction());
5059+
}
5060+
5061+
bool JavascriptOperators::CanShortcutPrototypeChainOnUnknownPropertyName(RecyclableObject *prototype)
5062+
{
5063+
Assert(prototype);
5064+
5065+
for (; prototype->GetTypeId() != TypeIds_Null; prototype = prototype->GetPrototype())
5066+
{
5067+
if (!CanShortcutInstanceOnUnknownPropertyName(prototype))
5068+
{
5069+
return false;
5070+
}
5071+
}
5072+
return true;
5073+
}
5074+
49805075
RecyclableObject* JavascriptOperators::GetPrototype(RecyclableObject* instance)
49815076
{
49825077
if (JavascriptOperators::GetTypeId(instance) == TypeIds_Null)

‎lib/Runtime/Language/JavascriptOperators.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,10 @@ namespace Js
215215
static BOOL IsAnyNumberValue(Var instance);
216216
static BOOL IsClassConstructor(Var instance);
217217

218+
static bool CanShortcutOnUnknownPropertyName(RecyclableObject * instance);
219+
static bool CanShortcutInstanceOnUnknownPropertyName(RecyclableObject *instance);
220+
static bool CanShortcutPrototypeChainOnUnknownPropertyName(RecyclableObject *instance);
221+
218222
static BOOL HasOwnItem(RecyclableObject* instance, uint32 index);
219223
static BOOL HasItem(RecyclableObject* instance, uint32 index);
220224
static BOOL HasItem(RecyclableObject* instance, uint64 index);

0 commit comments

Comments
 (0)