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

Update PropertyRecord interface to preserve the record life time #4580

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Update PropertyRecord interface to preserve the record life time #4580

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
2 changes: 1 addition & 1 deletion lib/Runtime/Base/ScriptContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,7 +877,7 @@ namespace Js

void ScriptContext::GetOrAddPropertyRecord(_In_ Js::JavascriptString * propertyString, _Out_ PropertyRecord const** propertyRecord)
{
*propertyRecord = propertyString->GetPropertyRecord();
propertyString->GetPropertyRecord(propertyRecord);
}

void ScriptContext::GetOrAddPropertyRecord(JsUtil::CharacterBuffer<WCHAR> const& propertyName, PropertyRecord const ** propertyRecord)
Expand Down
5 changes: 2 additions & 3 deletions lib/Runtime/Base/ThreadContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,10 +887,9 @@ ThreadContext::GetPropertyNameImpl(Js::PropertyId propertyId)
void
ThreadContext::FindPropertyRecord(Js::JavascriptString *pstName, Js::PropertyRecord const ** propertyRecord)
{
const Js::PropertyRecord * propRecord = pstName->GetPropertyRecord(true);
if (propRecord != nullptr)
pstName->GetPropertyRecord(propertyRecord, true);
if (*propertyRecord != nullptr)
{
*propertyRecord = propRecord;
return;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Language/JavascriptConversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ namespace Js
{
// For all other types, convert the key into a string and use that as the property name
JavascriptString * propName = JavascriptConversion::ToString(key, scriptContext);
*propertyRecord = propName->GetPropertyRecord();
propName->GetPropertyRecord(propertyRecord);
}

if (propString)
Expand Down
32 changes: 18 additions & 14 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1914,7 +1914,7 @@ namespace Js
{
if (value && !WithScopeObject::Is(object) && info->GetPropertyString())
{
PropertyId propertyId = info->GetPropertyString()->GetPropertyRecord()->GetPropertyId();
PropertyId propertyId = info->GetPropertyString()->GetPropertyId();
CacheOperators::CachePropertyRead(instance, object, false, propertyId, false, info, requestContext);
}
return JavascriptConversion::PropertyQueryFlagsToBoolean(result);
Expand All @@ -1928,7 +1928,7 @@ namespace Js
if (!PHASE_OFF1(MissingPropertyCachePhase) && info->GetPropertyString() && DynamicObject::Is(instance) && ((DynamicObject*)instance)->GetDynamicType()->GetTypeHandler()->IsPathTypeHandler())
{
PropertyValueInfo::Set(info, requestContext->GetLibrary()->GetMissingPropertyHolder(), 0);
CacheOperators::CachePropertyRead(instance, requestContext->GetLibrary()->GetMissingPropertyHolder(), false, info->GetPropertyString()->GetPropertyRecord()->GetPropertyId(), true, info, requestContext);
CacheOperators::CachePropertyRead(instance, requestContext->GetLibrary()->GetMissingPropertyHolder(), false, info->GetPropertyString()->GetPropertyId(), true, info, requestContext);
}

*value = requestContext->GetMissingPropertyResult();
Expand Down Expand Up @@ -2302,7 +2302,7 @@ namespace Js
{
if (!WithScopeObject::Is(receiver) && info->GetPropertyString())
{
CacheOperators::CachePropertyWrite(RecyclableObject::FromVar(receiver), false, object->GetType(), info->GetPropertyString()->GetPropertyRecord()->GetPropertyId(), info, requestContext);
CacheOperators::CachePropertyWrite(RecyclableObject::FromVar(receiver), false, object->GetType(), info->GetPropertyString()->GetPropertyId(), info, requestContext);
}
receiver = (RecyclableObject::FromVar(receiver))->GetThisObjectOrUnWrap();
RecyclableObject* func = RecyclableObject::FromVar(setterValueOrProxy);
Expand Down Expand Up @@ -2358,12 +2358,12 @@ namespace Js
{
if (!JavascriptProxy::Is(receiver) && info->GetPropertyString() && info->GetFlags() != InlineCacheSetterFlag && !object->CanHaveInterceptors())
{
CacheOperators::CachePropertyWrite(RecyclableObject::FromVar(receiver), false, typeWithoutProperty, info->GetPropertyString()->GetPropertyRecord()->GetPropertyId(), info, requestContext);
CacheOperators::CachePropertyWrite(RecyclableObject::FromVar(receiver), false, typeWithoutProperty, info->GetPropertyString()->GetPropertyId(), info, requestContext);

if (info->GetInstance() == receiverObject)
{
PropertyValueInfo::SetCacheInfo(info, info->GetPropertyString(), info->GetPropertyString()->GetLdElemInlineCache(), info->AllowResizingPolymorphicInlineCache());
CacheOperators::CachePropertyRead(object, receiverObject, false, info->GetPropertyString()->GetPropertyRecord()->GetPropertyId(), false, info, requestContext);
CacheOperators::CachePropertyRead(object, receiverObject, false, info->GetPropertyString()->GetPropertyId(), false, info, requestContext);
}
}
return TRUE;
Expand Down Expand Up @@ -3778,7 +3778,8 @@ namespace Js
LiteralStringWithPropertyStringPtr * strWithPtr = (LiteralStringWithPropertyStringPtr *)temp;
if (!strWithPtr->HasPropertyRecord())
{
strWithPtr->GetPropertyRecord(); // lookup-cache propertyRecord
PropertyRecord const * propertyRecord;
strWithPtr->GetPropertyRecord(&propertyRecord); // lookup-cache propertyRecord
}
else
{
Expand All @@ -3798,7 +3799,8 @@ namespace Js
JavascriptString::FromVar(index)->GetSz());
}

PropertyRecord const * propertyRecord = propertyString->GetPropertyRecord();
PropertyRecord const * propertyRecord;
propertyString->GetPropertyRecord(&propertyRecord);
Var value;

if (propertyRecord->IsNumeric())
Expand Down Expand Up @@ -4430,13 +4432,13 @@ namespace Js
LiteralStringWithPropertyStringPtr * strWithPtr = LiteralStringWithPropertyStringPtr::TryFromVar(index);
if (strWithPtr != nullptr)
{
propertyString = strWithPtr->GetPropertyString(); // do not force create the PropertyString,
// if it wasn't there, it won't be efficient for now.
propertyRecord = strWithPtr->GetPropertyRecord(true /* dontLookupFromDictionary */);
propertyString = strWithPtr->GetPropertyString(); // do not force create the PropertyString,
// if it wasn't there, it won't be efficient for now.
strWithPtr->GetPropertyRecord(&propertyRecord, true /* dontLookupFromDictionary */);
if (propertyRecord == nullptr)
{
propertyRecord = strWithPtr->GetPropertyRecord(); // lookup-cache propertyRecord
// later this call, there will be a lookup anyways!
strWithPtr->GetPropertyRecord(&propertyRecord); // lookup-cache propertyRecord
// later this call, there will be a lookup anyways!
}
else if (propertyString == nullptr)
{
Expand All @@ -4448,7 +4450,7 @@ namespace Js
}
else
{
propertyRecord = propertyString->GetPropertyRecord();
propertyString->GetPropertyRecord(&propertyRecord);
}

if (propertyRecord != nullptr)
Expand Down Expand Up @@ -10535,7 +10537,9 @@ namespace Js

BOOL JavascriptOperators::CheckPrototypesForAccessorOrNonWritableProperty(RecyclableObject* instance, JavascriptString* propertyNameString, Var* setterValue, DescriptorFlags* flags, PropertyValueInfo* info, ScriptContext* scriptContext)
{
PropertyId propertyId = propertyNameString->GetPropertyRecord()->GetPropertyId();
Js::PropertyRecord const * localPropertyRecord;
propertyNameString->GetPropertyRecord(&localPropertyRecord);
PropertyId propertyId = localPropertyRecord->GetPropertyId();
return CheckPrototypesForAccessorOrNonWritableProperty(instance, propertyId, setterValue, flags, info, scriptContext);
}

Expand Down
29 changes: 20 additions & 9 deletions lib/Runtime/Library/ConcatString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ namespace Js
this->propertyString = propStr;
if (propStr != nullptr)
{
this->propertyRecord = propStr->GetPropertyRecord();
Js::PropertyRecord const * localPropertyRecord;
propStr->GetPropertyRecord(&localPropertyRecord);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the local variable here, rather than passing in &(this->propertyRecord)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That wouldn't trigger SWB operator

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, makes sense. Could you please add a comment there so it doesn't get "cleaned up" later?

this->propertyRecord = localPropertyRecord;
}
}

Expand All @@ -159,20 +161,29 @@ namespace Js
return RecyclableObject::Is(var) && LiteralStringWithPropertyStringPtr::Is(RecyclableObject::UnsafeFromVar(var));
}

Js::PropertyRecord const * LiteralStringWithPropertyStringPtr::GetPropertyRecord(bool dontLookupFromDictionary)
void LiteralStringWithPropertyStringPtr::GetPropertyRecord(_Out_ PropertyRecord const** propRecord, bool dontLookupFromDictionary)
{
*propRecord = nullptr;
ScriptContext * scriptContext = this->GetScriptContext();

if (this->propertyRecord == nullptr && !dontLookupFromDictionary)
if (this->propertyRecord == nullptr)
{
Js::PropertyRecord const * localPropertyRecord;
scriptContext->GetOrAddPropertyRecord(this->GetSz(),
static_cast<int>(this->GetLength()),
&localPropertyRecord);
this->propertyRecord = localPropertyRecord;
if (!dontLookupFromDictionary)
{
// cache PropertyRecord
Js::PropertyRecord const * localPropertyRecord;
scriptContext->GetOrAddPropertyRecord(this->GetSz(),
static_cast<int>(this->GetLength()),
&localPropertyRecord);
this->propertyRecord = localPropertyRecord;
}
else
{
return;
}
}

return this->propertyRecord;
*propRecord = this->propertyRecord;
}

/////////////////////// ConcatStringBase //////////////////////////
Expand Down
3 changes: 2 additions & 1 deletion lib/Runtime/Library/ConcatString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ namespace Js
Field(const Js::PropertyRecord*) propertyRecord;

public:
virtual Js::PropertyRecord const * GetPropertyRecord(bool dontLookupFromDictionary = false) override;
virtual void GetPropertyRecord(_Out_ PropertyRecord const** propRecord, bool dontLookupFromDictionary = false) override;

bool HasPropertyRecord() const { return propertyRecord != nullptr; }

PropertyString * GetPropertyString() const;
Expand Down
4 changes: 2 additions & 2 deletions lib/Runtime/Library/ForInObjectEnumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,10 @@ namespace Js
// Property Id does not exist.
if (propertyId == Constants::NoProperty)
{
propRecord = currentIndex->GetPropertyRecord(true);
currentIndex->GetPropertyRecord(&propRecord, true);
if (propRecord == nullptr)
{
propRecord = currentIndex->GetPropertyRecord(false); // will create
currentIndex->GetPropertyRecord(&propRecord, false); // will create
// We keep the track of what is enumerated using a bit vector of propertyID.
// so the propertyId can't be collected until the end of the for in enumerator
// Keep a list of the property string.
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JSONStringifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ JSONStringifier::ReadValue(_In_ JavascriptString* key, _In_opt_ const PropertyRe

if (propertyRecord == nullptr)
{
propertyRecord = key->GetPropertyRecord();
key->GetPropertyRecord(&propertyRecord);
}
JavascriptOperators::GetProperty(holder, propertyRecord->GetPropertyId(), &value, this->scriptContext, &info);
return value;
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JavascriptObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1733,7 +1733,7 @@ namespace Js
}
else
{
propertyRecord = ((PropertyString*)propertyName)->GetPropertyRecord();
propertyName->GetPropertyRecord(&propertyRecord);
}

if (descCount == descSize)
Expand Down
10 changes: 4 additions & 6 deletions lib/Runtime/Library/JavascriptString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,17 +224,15 @@ namespace Js
return JavascriptOperators::GetTypeId(aValue) == TypeIds_String;
}

Js::PropertyRecord const * JavascriptString::GetPropertyRecord(bool dontLookupFromDictionary)
void JavascriptString::GetPropertyRecord(_Out_ Js::PropertyRecord const ** propertyRecord, bool dontLookupFromDictionary)
{
*propertyRecord = nullptr;
if (dontLookupFromDictionary)
{
return nullptr;
return;
}

Js::PropertyRecord const * propertyRecord;
GetScriptContext()->GetOrAddPropertyRecord(GetString(), GetLength(), &propertyRecord);

return propertyRecord;
GetScriptContext()->GetOrAddPropertyRecord(GetString(), GetLength(), propertyRecord);
}

JavascriptString* JavascriptString::FromVar(Var aValue)
Expand Down
6 changes: 3 additions & 3 deletions lib/Runtime/Library/JavascriptString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ namespace Js
BOOL GetItemAt(charcount_t idxChar, Var* value);
char16 GetItem(charcount_t index);

virtual Js::PropertyRecord const * GetPropertyRecord(bool dontLookupFromDictionary = false);
virtual void GetPropertyRecord(_Out_ PropertyRecord const** propertyRecord, bool dontLookupFromDictionary = false);

_Ret_range_(m_charLength, m_charLength) charcount_t GetLength() const;
virtual size_t GetAllocatedByteCount() const;
Expand Down Expand Up @@ -354,8 +354,8 @@ namespace Js
{
inline static bool Equals(JavascriptString * str1, JavascriptString * str2)
{
// We want to pin the strings str1 and str2 because flattening of any of these strings could cause a GC and result in the other string getting collected if it was optimized
// away by the compiler. We would normally have called the EnterPinnedScope/LeavePinnedScope methods here but it adds extra call instructions to the assembly code. As Equals
// We want to pin the strings str1 and str2 because flattening of any of these strings could cause a GC and result in the other string getting collected if it was optimized
// away by the compiler. We would normally have called the EnterPinnedScope/LeavePinnedScope methods here but it adds extra call instructions to the assembly code. As Equals
// methods could get called a lot of times this can show up as regressions in benchmarks.
volatile Js::JavascriptString** keepAliveString1 = (volatile Js::JavascriptString**)& str1;
volatile Js::JavascriptString** keepAliveString2 = (volatile Js::JavascriptString**)& str2;
Expand Down
6 changes: 4 additions & 2 deletions lib/Runtime/Library/LazyJSONString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ LazyJSONString::ReconstructObject(_In_ JSONObject* valueList) const
PropertyValueInfo info;
if (!propertyString || !propertyString->TrySetPropertyFromCache(obj, propertyValue, this->GetScriptContext(), PropertyOperation_None, &info))
{
const PropertyRecord* propertyRecord = propertyName->GetPropertyRecord();
JavascriptOperators::SetProperty(obj, obj, propertyRecord->GetPropertyId(), propertyValue, &info, this->GetScriptContext());
Js::PropertyRecord const * localPropertyRecord;
propertyName->GetPropertyRecord(&localPropertyRecord);
JavascriptOperators::SetProperty(obj, obj, localPropertyRecord->GetPropertyId(),
propertyValue, &info, this->GetScriptContext());
}
}
NEXT_SLISTCOUNTED_ENTRY;
Expand Down
9 changes: 7 additions & 2 deletions lib/Runtime/Library/PropertyString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,14 @@ class PropertyString : public JavascriptString

PropertyString(StaticType* type, const Js::PropertyRecord* propertyRecord);
public:
virtual Js::PropertyRecord const * GetPropertyRecord(bool dontLookupFromDictionary = false) override
virtual void GetPropertyRecord(_Out_ PropertyRecord const** propertyRecord, bool dontLookupFromDictionary = false) override
{
return this->propertyRecord;
*propertyRecord = this->propertyRecord;
}

Js::PropertyId GetPropertyId()
{
return this->propertyRecord->GetPropertyId();
}

PolymorphicInlineCache * GetLdElemInlineCache() const;
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Types/DynamicObjectPropertyEnumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ namespace Js
{
PropertyString * propertyString = cachedData->strings[enumeratedCount];
propertyStringName = propertyString;
propertyId = propertyString->GetPropertyRecord()->GetPropertyId();
propertyId = propertyString->GetPropertyId();

#if DBG
PropertyId tempPropertyId;
Expand Down
5 changes: 3 additions & 2 deletions lib/Runtime/Types/SimpleDictionaryTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ namespace Js
PropertyString * propertyString = PropertyString::TryFromVar(key);
if (propertyString != nullptr)
{
propertyRecord = propertyString->GetPropertyRecord();
propertyString->GetPropertyRecord(&propertyRecord);
}
else
{
Expand All @@ -104,7 +104,8 @@ namespace Js
bool TPropertyKey_IsInternalPropertyId(JavascriptString* key)
{
// WARNING: This will return false for PropertyStrings that are actually InternalPropertyIds
Assert(!PropertyString::Is(key) || !IsInternalPropertyId(((PropertyString*)key)->GetPropertyRecord()->GetPropertyId()));
Assert(!PropertyString::Is(key) || !IsInternalPropertyId(((PropertyString*)key)->GetPropertyId()));

return false;
}

Expand Down