Skip to content

Commit 054c94b

Browse files
obastemurobastemur
committed
[MERGE #4123 @obastemur] Equals: minor improvements
Merge pull request #4123 from obastemur:lesscmp See issue details #4110 - Fix unnecessary string comparison with PropertyRecord/Equal (acme-air hits often) - Improve BuiltInPropertyRecord string comparison (acme-air hits 3 times per request) This PR needs #3875 to be merged first. Results from micro-`test/benchmark` are flat.
2 parents 2ea7a7c + 3ca3051 commit 054c94b

13 files changed

+115
-90
lines changed

‎lib/Runtime/Base/PropertyRecord.h

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,46 @@ namespace Js
121121

122122
bool Equals(JsUtil::CharacterBuffer<WCHAR> const & str) const
123123
{
124-
return (LEN - 1 == str.GetLength() &&
125-
JsUtil::CharacterBuffer<WCHAR>::StaticEquals(buffer, str.GetBuffer(), LEN - 1));
124+
#ifndef _NTBUILD
125+
AssertMsg(false, "Do you really have to use this interface?");
126+
#endif
127+
return Equals(str.GetBuffer(), str.GetLength());
128+
}
129+
130+
bool Equals(JavascriptString * str) const
131+
{
132+
PropertyString * propString = PropertyString::TryFromVar(str);
133+
const PropertyRecord * propRecord = nullptr;
134+
if (propString == nullptr)
135+
{
136+
LiteralStringWithPropertyStringPtr * lstr = LiteralStringWithPropertyStringPtr::TryFromVar(str);
137+
propRecord = lstr->GetPropertyRecord();
138+
}
139+
else
140+
{
141+
propRecord = propString->GetPropertyRecord();
142+
}
143+
144+
145+
if (propRecord == nullptr)
146+
{
147+
return Equals(str->GetString(), str->GetLength());
148+
}
149+
else
150+
{
151+
return Equals(propRecord->GetPropertyId());
152+
}
153+
}
154+
155+
bool Equals(const PropertyId & propertyId) const
156+
{
157+
return propertyId == propertyRecord.GetPropertyId();
158+
}
159+
160+
bool Equals(const WCHAR * str, const charcount_t length) const
161+
{
162+
return (LEN - 1 == length &&
163+
JsUtil::CharacterBuffer<WCHAR>::StaticEquals(buffer, str, LEN - 1));
126164
}
127165
};
128166

@@ -169,8 +207,7 @@ namespace Js
169207
{
170208
inline static bool Equals(PropertyRecord const * str1, PropertyRecord const * str2)
171209
{
172-
return (str1->GetLength() == str2->GetLength() &&
173-
JsUtil::CharacterBuffer<WCHAR>::StaticEquals(str1->GetBuffer(), str2->GetBuffer(), str1->GetLength()));
210+
return str1->GetPropertyId() == str2->GetPropertyId();
174211
}
175212

176213
inline static bool Equals(PropertyRecord const * str1, JsUtil::CharacterBuffer<WCHAR> const * str2)

‎lib/Runtime/Language/JavascriptOperators.cpp

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1351,7 +1351,7 @@ namespace Js
13511351
{
13521352
return TRUE;
13531353
}
1354-
1354+
13551355
JavascriptProxy* proxy = JavascriptOperators::TryFromVar<JavascriptProxy>(instance);
13561356
if (proxy)
13571357
{
@@ -4466,45 +4466,39 @@ namespace Js
44664466

44674467
if (indexType == IndexType_Number)
44684468
{
4469+
SetElementIHelper_INDEX_TYPE_IS_NUMBER:
44694470
return JavascriptOperators::SetItem(receiver, object, indexVal, value, scriptContext, flags);
44704471
}
44714472
else if (indexType == IndexType_JavascriptString)
44724473
{
44734474
Assert(propertyNameString);
4474-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
44754475

4476-
if (BuiltInPropertyRecords::NaN.Equals(propertyName))
4477-
{
4478-
// Follow SetProperty convention for NaN
4479-
return JavascriptOperators::SetProperty(receiver, object, PropertyIds::NaN, value, scriptContext, flags);
4480-
}
4481-
else if (BuiltInPropertyRecords::Infinity.Equals(propertyName))
4482-
{
4483-
// Follow SetProperty convention for Infinity
4484-
return JavascriptOperators::SetProperty(receiver, object, PropertyIds::Infinity, value, scriptContext, flags);
4485-
}
4486-
#ifdef ENABLE_DEBUG_CONFIG_OPTIONS
4487-
if (PHASE_TRACE1(PropertyStringCachePhase))
4476+
// At this point, we know that the propertyNameString is neither PropertyString
4477+
// or LiteralStringWithPropertyStringPtr.. Get PropertyRecord!
4478+
// we will get it anyways otherwise. (Also, 1:1 string comparison for Builtin types will be expensive.)
4479+
4480+
if (propertyRecord == nullptr)
44884481
{
4489-
Output::Print(_u("PropertyCache: SetElem No property string for '%s'\n"), propertyNameString->GetString());
4482+
scriptContext->GetOrAddPropertyRecord(propertyNameString, &propertyRecord);
4483+
if (propertyRecord->IsNumeric())
4484+
{
4485+
indexVal = propertyRecord->GetNumericValue();
4486+
goto SetElementIHelper_INDEX_TYPE_IS_NUMBER;
4487+
}
44904488
}
4491-
#endif
4492-
return SetPropertyWPCache(receiver, object, propertyNameString, value, scriptContext, flags, &propertyValueInfo);
44934489
}
4494-
else
4490+
4491+
Assert(indexType == IndexType_PropertyId || indexType == IndexType_JavascriptString);
4492+
Assert(propertyRecord);
4493+
PropertyId propId = propertyRecord->GetPropertyId();
4494+
if (propId == PropertyIds::NaN || propId == PropertyIds::Infinity)
44954495
{
4496-
Assert(indexType == IndexType_PropertyId);
4497-
Assert(propertyRecord);
4498-
PropertyId propId = propertyRecord->GetPropertyId();
4499-
if (propId == PropertyIds::NaN || propId == PropertyIds::Infinity)
4500-
{
4501-
// As we no longer convert o[x] into o.x for NaN and Infinity, we need to follow SetProperty convention for these,
4502-
// which would check for read-only properties, strict mode, etc.
4503-
// Note that "-Infinity" does not qualify as property name, so we don't have to take care of it.
4504-
return JavascriptOperators::SetProperty(receiver, object, propId, value, scriptContext, flags);
4505-
}
4506-
return SetPropertyWPCache(receiver, object, propId, value, scriptContext, flags, &propertyValueInfo);
4496+
// As we no longer convert o[x] into o.x for NaN and Infinity, we need to follow SetProperty convention for these,
4497+
// which would check for read-only properties, strict mode, etc.
4498+
// Note that "-Infinity" does not qualify as property name, so we don't have to take care of it.
4499+
return JavascriptOperators::SetProperty(receiver, object, propId, value, scriptContext, flags);
45074500
}
4501+
return SetPropertyWPCache(receiver, object, propId, value, scriptContext, flags, &propertyValueInfo);
45084502
}
45094503

45104504
BOOL JavascriptOperators::OP_SetNativeIntElementI(

‎lib/Runtime/Library/BoundFunction.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -438,8 +438,7 @@ namespace Js
438438

439439
BOOL BoundFunction::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
440440
{
441-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
442-
if (BuiltInPropertyRecords::length.Equals(propertyName))
441+
if (BuiltInPropertyRecords::length.Equals(propertyNameString))
443442
{
444443
return false;
445444
}

‎lib/Runtime/Library/JavascriptArray.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3140,7 +3140,7 @@ namespace Js
31403140
JS_REENTRANT_NO_MUTATE(jsReentLock, CopyNativeIntArrayElementsToVar(pDestArray, BigIndex(idxDest).GetSmallIndex(), pIntItemArray));
31413141
idxDest = idxDest + pIntItemArray->length;
31423142
}
3143-
else
3143+
else
31443144
{
31453145
JavascriptNativeFloatArray *pFloatItemArray = JavascriptOperators::TryFromVar<JavascriptNativeFloatArray>(aItem);
31463146
if (pFloatItemArray)
@@ -3390,7 +3390,7 @@ namespace Js
33903390

33913391
idxDest = idxDest + pIntItemArray->length;
33923392
}
3393-
else
3393+
else
33943394
{
33953395
JavascriptNativeFloatArray * pFloatItemArray = JavascriptOperators::TryFromVar<JavascriptNativeFloatArray>(aItem);
33963396
if (pFloatItemArray && !isFillFromPrototypes)
@@ -5380,7 +5380,7 @@ namespace Js
53805380
{
53815381
RecyclableObject* protoObj = prototype;
53825382

5383-
if (!(DynamicObject::IsAnyArray(protoObj) || JavascriptOperators::IsObject(protoObj))
5383+
if (!(DynamicObject::IsAnyArray(protoObj) || JavascriptOperators::IsObject(protoObj))
53845384
|| JavascriptProxy::Is(protoObj)
53855385
|| protoObj->IsExternal())
53865386
{
@@ -6095,7 +6095,7 @@ namespace Js
60956095
*isIntArray = true;
60966096
#endif
60976097
}
6098-
else
6098+
else
60996099
{
61006100
JavascriptNativeFloatArray* nativeFloatArray = JavascriptOperators::TryFromVar<JavascriptNativeFloatArray>(this);
61016101
if (nativeFloatArray)
@@ -12162,8 +12162,7 @@ namespace Js
1216212162

1216312163
BOOL JavascriptArray::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
1216412164
{
12165-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
12166-
if (BuiltInPropertyRecords::length.Equals(propertyName))
12165+
if (BuiltInPropertyRecords::length.Equals(propertyNameString))
1216712166
{
1216812167
return false;
1216912168
}

‎lib/Runtime/Library/JavascriptFunction.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3071,16 +3071,15 @@ void __cdecl _alloca_probe_16()
30713071

30723072
BOOL JavascriptFunction::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
30733073
{
3074-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
3075-
if (BuiltInPropertyRecords::caller.Equals(propertyName) || BuiltInPropertyRecords::arguments.Equals(propertyName))
3074+
if (BuiltInPropertyRecords::caller.Equals(propertyNameString) || BuiltInPropertyRecords::arguments.Equals(propertyNameString))
30763075
{
30773076
if (this->HasRestrictedProperties())
30783077
{
30793078
JavascriptError::ThrowCantDeleteIfStrictMode(flags, this->GetScriptContext(), propertyNameString->GetString());
30803079
return false;
30813080
}
30823081
}
3083-
else if (BuiltInPropertyRecords::length.Equals(propertyName))
3082+
else if (BuiltInPropertyRecords::length.Equals(propertyNameString))
30843083
{
30853084
if (this->IsScriptFunction())
30863085
{
@@ -3091,7 +3090,7 @@ void __cdecl _alloca_probe_16()
30913090

30923091
BOOL result = DynamicObject::DeleteProperty(propertyNameString, flags);
30933092

3094-
if (result && (BuiltInPropertyRecords::prototype.Equals(propertyName) || BuiltInPropertyRecords::_symbolHasInstance.Equals(propertyName)))
3093+
if (result && (BuiltInPropertyRecords::prototype.Equals(propertyNameString) || BuiltInPropertyRecords::_symbolHasInstance.Equals(propertyNameString)))
30953094
{
30963095
InvalidateConstructorCacheOnPrototypeChange();
30973096
this->GetScriptContext()->GetThreadContext()->InvalidateIsInstInlineCachesForFunction(this);

‎lib/Runtime/Library/JavascriptGeneratorFunction.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -479,13 +479,12 @@ namespace Js
479479

480480
BOOL JavascriptGeneratorFunction::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
481481
{
482-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
483-
if (BuiltInPropertyRecords::length.Equals(propertyName))
482+
if (BuiltInPropertyRecords::length.Equals(propertyNameString))
484483
{
485484
return false;
486485
}
487486

488-
if (BuiltInPropertyRecords::caller.Equals(propertyName) || BuiltInPropertyRecords::arguments.Equals(propertyName))
487+
if (BuiltInPropertyRecords::caller.Equals(propertyNameString) || BuiltInPropertyRecords::arguments.Equals(propertyNameString))
489488
{
490489
// JavascriptFunction has special case for caller and arguments; call DynamicObject:: virtual directly to skip that.
491490
return DynamicObject::DeleteProperty(propertyNameString, flags);

‎lib/Runtime/Library/JavascriptRegExpConstructor.cpp

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -391,27 +391,30 @@ namespace Js
391391

392392
BOOL JavascriptRegExpConstructor::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
393393
{
394-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
395-
if (BuiltInPropertyRecords::input.Equals(propertyName)
396-
|| BuiltInPropertyRecords::$_.Equals(propertyName)
397-
|| BuiltInPropertyRecords::lastMatch.Equals(propertyName)
398-
|| BuiltInPropertyRecords::$Ampersand.Equals(propertyName)
399-
|| BuiltInPropertyRecords::lastParen.Equals(propertyName)
400-
|| BuiltInPropertyRecords::$Plus.Equals(propertyName)
401-
|| BuiltInPropertyRecords::leftContext.Equals(propertyName)
402-
|| BuiltInPropertyRecords::$BackTick.Equals(propertyName)
403-
|| BuiltInPropertyRecords::rightContext.Equals(propertyName)
404-
|| BuiltInPropertyRecords::$Tick.Equals(propertyName)
405-
|| BuiltInPropertyRecords::$1.Equals(propertyName)
406-
|| BuiltInPropertyRecords::$2.Equals(propertyName)
407-
|| BuiltInPropertyRecords::$3.Equals(propertyName)
408-
|| BuiltInPropertyRecords::$4.Equals(propertyName)
409-
|| BuiltInPropertyRecords::$5.Equals(propertyName)
410-
|| BuiltInPropertyRecords::$6.Equals(propertyName)
411-
|| BuiltInPropertyRecords::$7.Equals(propertyName)
412-
|| BuiltInPropertyRecords::$8.Equals(propertyName)
413-
|| BuiltInPropertyRecords::$9.Equals(propertyName)
414-
|| BuiltInPropertyRecords::index.Equals(propertyName))
394+
PropertyRecord const * propertyRecord = nullptr;
395+
propertyNameString->GetScriptContext()->GetOrAddPropertyRecord(propertyNameString, &propertyRecord);
396+
PropertyId propertyId = propertyRecord->GetPropertyId();
397+
398+
if (BuiltInPropertyRecords::input.Equals(propertyId)
399+
|| BuiltInPropertyRecords::$_.Equals(propertyId)
400+
|| BuiltInPropertyRecords::lastMatch.Equals(propertyId)
401+
|| BuiltInPropertyRecords::$Ampersand.Equals(propertyId)
402+
|| BuiltInPropertyRecords::lastParen.Equals(propertyId)
403+
|| BuiltInPropertyRecords::$Plus.Equals(propertyId)
404+
|| BuiltInPropertyRecords::leftContext.Equals(propertyId)
405+
|| BuiltInPropertyRecords::$BackTick.Equals(propertyId)
406+
|| BuiltInPropertyRecords::rightContext.Equals(propertyId)
407+
|| BuiltInPropertyRecords::$Tick.Equals(propertyId)
408+
|| BuiltInPropertyRecords::$1.Equals(propertyId)
409+
|| BuiltInPropertyRecords::$2.Equals(propertyId)
410+
|| BuiltInPropertyRecords::$3.Equals(propertyId)
411+
|| BuiltInPropertyRecords::$4.Equals(propertyId)
412+
|| BuiltInPropertyRecords::$5.Equals(propertyId)
413+
|| BuiltInPropertyRecords::$6.Equals(propertyId)
414+
|| BuiltInPropertyRecords::$7.Equals(propertyId)
415+
|| BuiltInPropertyRecords::$8.Equals(propertyId)
416+
|| BuiltInPropertyRecords::$9.Equals(propertyId)
417+
|| BuiltInPropertyRecords::index.Equals(propertyId))
415418
{
416419
JavascriptError::ThrowCantDeleteIfStrictMode(flags, GetScriptContext(), propertyNameString->GetString());
417420
return false;

‎lib/Runtime/Library/JavascriptRegularExpression.cpp

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1321,7 +1321,6 @@ namespace Js
13211321
BOOL JavascriptRegExp::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags flags)
13221322
{
13231323
const ScriptConfiguration* scriptConfig = this->GetScriptContext()->GetConfig();
1324-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
13251324

13261325
#define DELETE_PROPERTY(ownProperty) \
13271326
if (ownProperty) \
@@ -1331,23 +1330,23 @@ namespace Js
13311330
} \
13321331
return DynamicObject::DeleteProperty(propertyNameString, flags);
13331332

1334-
if (BuiltInPropertyRecords::lastIndex.Equals(propertyName))
1333+
if (BuiltInPropertyRecords::lastIndex.Equals(propertyNameString))
13351334
{
13361335
DELETE_PROPERTY(true);
13371336
}
1338-
else if (BuiltInPropertyRecords::global.Equals(propertyName)
1339-
|| BuiltInPropertyRecords::multiline.Equals(propertyName)
1340-
|| BuiltInPropertyRecords::ignoreCase.Equals(propertyName)
1341-
|| BuiltInPropertyRecords::source.Equals(propertyName)
1342-
|| BuiltInPropertyRecords::options.Equals(propertyName))
1337+
else if (BuiltInPropertyRecords::global.Equals(propertyNameString)
1338+
|| BuiltInPropertyRecords::multiline.Equals(propertyNameString)
1339+
|| BuiltInPropertyRecords::ignoreCase.Equals(propertyNameString)
1340+
|| BuiltInPropertyRecords::source.Equals(propertyNameString)
1341+
|| BuiltInPropertyRecords::options.Equals(propertyNameString))
13431342
{
13441343
DELETE_PROPERTY(!scriptConfig->IsES6RegExPrototypePropertiesEnabled());
13451344
}
1346-
else if (BuiltInPropertyRecords::unicode.Equals(propertyName))
1345+
else if (BuiltInPropertyRecords::unicode.Equals(propertyNameString))
13471346
{
13481347
DELETE_PROPERTY(scriptConfig->IsES6UnicodeExtensionsEnabled() && !scriptConfig->IsES6RegExPrototypePropertiesEnabled());
13491348
}
1350-
else if (BuiltInPropertyRecords::sticky.Equals(propertyName))
1349+
else if (BuiltInPropertyRecords::sticky.Equals(propertyNameString))
13511350
{
13521351
DELETE_PROPERTY(scriptConfig->IsES6RegExStickyEnabled() && !scriptConfig->IsES6RegExPrototypePropertiesEnabled());
13531352
}

‎lib/Runtime/Library/JavascriptString.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3863,8 +3863,7 @@ namespace Js
38633863

38643864
BOOL JavascriptString::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags propertyOperationFlags)
38653865
{
3866-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
3867-
if (BuiltInPropertyRecords::length.Equals(propertyName))
3866+
if (BuiltInPropertyRecords::length.Equals(propertyNameString))
38683867
{
38693868
JavascriptError::ThrowCantDeleteIfStrictMode(propertyOperationFlags, this->GetScriptContext(), propertyNameString->GetString());
38703869

‎lib/Runtime/Library/JavascriptStringObject.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,8 +342,7 @@ namespace Js
342342

343343
BOOL JavascriptStringObject::DeleteProperty(JavascriptString *propertyNameString, PropertyOperationFlags propertyOperationFlags)
344344
{
345-
JsUtil::CharacterBuffer<WCHAR> propertyName(propertyNameString->GetString(), propertyNameString->GetLength());
346-
if (BuiltInPropertyRecords::length.Equals(propertyName))
345+
if (BuiltInPropertyRecords::length.Equals(propertyNameString))
347346
{
348347
JavascriptError::ThrowCantDeleteIfStrictMode(propertyOperationFlags, this->GetScriptContext(), propertyNameString->GetString());
349348

0 commit comments

Comments
 (0)