From a3d55afcca3c692eea42cded728405790f1c1457 Mon Sep 17 00:00:00 2001 From: Jianchun Xu Date: Thu, 16 Jul 2015 15:37:03 -0700 Subject: [PATCH] chakrashim: some performance improvements IsTypeOf(): Replace built-in type check by name with by cached global constructor function. This gets rid of repeated propertyId lookup and getting property of global object. Optimize likely int/double/bool conversions: In Value::Int32Value(), try JsNumberToInt directly before performing JS value conversion, since the underlying value is very likely an int already. Similarly for Uint32Value(), BoolValue(). IsExternal(): Call JsHasProperty with a private symbol. Remove IsObject() and propertyId lookup calls. HandleScope: Use stack to save small number of refs. Skips slow JS array code path for many scenarios. Reviewed-by: @kunalspathak, @Cellule --- deps/chakrashim/include/v8.h | 7 +- .../src/jsrtcachedpropertyidref.inc | 12 +++ deps/chakrashim/src/jsrtcontextcachedobj.inc | 9 ++- deps/chakrashim/src/jsrtisolateshim.cc | 53 +++++++------ deps/chakrashim/src/jsrtisolateshim.h | 12 ++- deps/chakrashim/src/jsrtutils.cc | 14 +--- deps/chakrashim/src/jsrtutils.h | 46 ++++++++++++ deps/chakrashim/src/v8external.cc | 23 +++--- deps/chakrashim/src/v8handlescope.cc | 8 +- deps/chakrashim/src/v8value.cc | 74 +++++++------------ 10 files changed, 153 insertions(+), 105 deletions(-) diff --git a/deps/chakrashim/include/v8.h b/deps/chakrashim/include/v8.h index 2d595400b3f..70856106bfd 100644 --- a/deps/chakrashim/include/v8.h +++ b/deps/chakrashim/include/v8.h @@ -422,11 +422,12 @@ class EXPORT Eternal : private Persistent { // values created will then be added to that array. So the GC will see the array // on the stack and then keep those local references alive. class EXPORT HandleScope { - template - friend class Local; + template friend class Local; + static const int kOnStackLocals = 5; // Arbitrary number of refs on stack private: - JsValueRef _refs; + JsValueRef _locals[kOnStackLocals]; // Save some refs on stack + JsValueRef _refs; // More refs go to a JS array int _count; HandleScope *_prev; JsContextRef _contextRef; diff --git a/deps/chakrashim/src/jsrtcachedpropertyidref.inc b/deps/chakrashim/src/jsrtcachedpropertyidref.inc index d6feb47f912..710728d2901 100644 --- a/deps/chakrashim/src/jsrtcachedpropertyidref.inc +++ b/deps/chakrashim/src/jsrtcachedpropertyidref.inc @@ -18,6 +18,14 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS // IN THE SOFTWARE. +#ifndef DEF +#define DEF(x) +#endif + +#ifndef DEFSYMBOL +#define DEFSYMBOL(x) +#endif + DEF(apply) DEF(construct) DEF(defineProperty) @@ -63,4 +71,8 @@ DEF(buffer) DEF(byteOffset) DEF(byteLength) +DEFSYMBOL(self) +DEFSYMBOL(__isexternal__) + #undef DEF +#undef DEFSYMBOL diff --git a/deps/chakrashim/src/jsrtcontextcachedobj.inc b/deps/chakrashim/src/jsrtcontextcachedobj.inc index edfe068ad11..b9dc5e015f6 100644 --- a/deps/chakrashim/src/jsrtcontextcachedobj.inc +++ b/deps/chakrashim/src/jsrtcontextcachedobj.inc @@ -30,14 +30,21 @@ DEFTYPE(ArrayBuffer) DEFTYPE(Boolean) DEFTYPE(Date) +DEFTYPE(Error) +DEFTYPE(EvalError) DEFTYPE(Function) DEFTYPE(Number) DEFTYPE(Object) DEFTYPE(Proxy) +DEFTYPE(RangeError) +DEFTYPE(ReferenceError) DEFTYPE(RegExp) DEFTYPE(String) -DEFTYPE(Uint8Array) +DEFTYPE(SyntaxError) +DEFTYPE(TypeError) DEFTYPE(Uint32Array) +DEFTYPE(Uint8Array) +DEFTYPE(URIError) // These prototype functions will be cached/shimmed diff --git a/deps/chakrashim/src/jsrtisolateshim.cc b/deps/chakrashim/src/jsrtisolateshim.cc index b74ec678a3a..8a0e1157188 100644 --- a/deps/chakrashim/src/jsrtisolateshim.cc +++ b/deps/chakrashim/src/jsrtisolateshim.cc @@ -32,15 +32,15 @@ namespace jsrt { IsolateShim::IsolateShim(JsRuntimeHandle runtime) : runtime(runtime), contextScopeStack(nullptr), - selfSymbolPropertyIdRef(JS_INVALID_REFERENCE), + symbolPropertyIdRefs(), cachedPropertyIdRefs(), + embeddedData(), isDisposing(false), tryCatchStackTop(nullptr) { // CHAKRA-TODO: multithread locking for s_isolateList? this->prevnext = &s_isolateList; this->next = s_isolateList; s_isolateList = this; - memset(embeddedData, 0, sizeof(embeddedData)); } IsolateShim::~IsolateShim() { @@ -241,7 +241,7 @@ ContextShim * IsolateShim::GetCurrentContextShim() { } JsPropertyIdRef IsolateShim::GetSelfSymbolPropertyIdRef() { - return EnsurePrivateSymbol(&selfSymbolPropertyIdRef); + return GetCachedSymbolPropertyIdRef(CachedSymbolPropertyIdRef::self); } JsPropertyIdRef IsolateShim::GetKeepAliveObjectSymbolPropertyIdRef() { @@ -250,20 +250,29 @@ JsPropertyIdRef IsolateShim::GetKeepAliveObjectSymbolPropertyIdRef() { // return EnsurePrivateSymbol(&keepAliveObjectSymbolPropertyIdRef); } -JsPropertyIdRef IsolateShim::EnsurePrivateSymbol( - JsPropertyIdRef * propertyIdRefPtr) { - assert(this->GetCurrentContextShim() != nullptr); - JsPropertyIdRef propertyIdRef = *propertyIdRefPtr; - if (propertyIdRef == JS_INVALID_REFERENCE) { - JsValueRef newSymbol; - if (JsCreateSymbol(JS_INVALID_REFERENCE, &newSymbol) == JsNoError) { - if (JsGetPropertyIdFromSymbol(newSymbol, &propertyIdRef) == JsNoError) { - JsAddRef(propertyIdRef, nullptr); - *propertyIdRefPtr = propertyIdRef; - } +template +JsPropertyIdRef GetCachedPropertyId( + JsPropertyIdRef cache[], Index index, + const CreatePropertyIdFunc& createPropertyId) { + JsPropertyIdRef propIdRef = cache[index]; + if (propIdRef == JS_INVALID_REFERENCE) { + if (createPropertyId(index, &propIdRef)) { + JsAddRef(propIdRef, nullptr); + cache[index] = propIdRef; } } - return propertyIdRef; + return propIdRef; +} + +JsPropertyIdRef IsolateShim::GetCachedSymbolPropertyIdRef( + CachedSymbolPropertyIdRef cachedSymbolPropertyIdRef) { + CHAKRA_ASSERT(this->GetCurrentContextShim() != nullptr); + return GetCachedPropertyId(symbolPropertyIdRefs, cachedSymbolPropertyIdRef, + [](CachedSymbolPropertyIdRef, JsPropertyIdRef* propIdRef) { + JsValueRef newSymbol; + return JsCreateSymbol(JS_INVALID_REFERENCE, &newSymbol) == JsNoError && + JsGetPropertyIdFromSymbol(newSymbol, propIdRef) == JsNoError; + }); } static wchar_t const * @@ -274,15 +283,11 @@ const s_cachedPropertyIdRefNames[CachedPropertyIdRef::Count] = { JsPropertyIdRef IsolateShim::GetCachedPropertyIdRef( CachedPropertyIdRef cachedPropertyIdRef) { - JsPropertyIdRef propertyIdRef = cachedPropertyIdRefs[cachedPropertyIdRef]; - if (propertyIdRef == JS_INVALID_REFERENCE) { - if (JsGetPropertyIdFromName(s_cachedPropertyIdRefNames[cachedPropertyIdRef], - &propertyIdRef) == JsNoError) { - JsAddRef(propertyIdRef, nullptr); - cachedPropertyIdRefs[cachedPropertyIdRef] = propertyIdRef; - } - } - return propertyIdRef; + return GetCachedPropertyId(cachedPropertyIdRefs, cachedPropertyIdRef, + [](CachedPropertyIdRef index, JsPropertyIdRef* propIdRef) { + return JsGetPropertyIdFromName(s_cachedPropertyIdRefNames[index], + propIdRef) == JsNoError; + }); } JsPropertyIdRef IsolateShim::GetProxyTrapPropertyIdRef(ProxyTraps trap) { diff --git a/deps/chakrashim/src/jsrtisolateshim.h b/deps/chakrashim/src/jsrtisolateshim.h index cfd01eaafe8..e1a54b73644 100644 --- a/deps/chakrashim/src/jsrtisolateshim.h +++ b/deps/chakrashim/src/jsrtisolateshim.h @@ -36,6 +36,12 @@ enum CachedPropertyIdRef { Count }; +enum CachedSymbolPropertyIdRef { +#define DEFSYMBOL(x, ...) x, +#include "jsrtcachedpropertyidref.inc" + SymbolCount +}; + class IsolateShim { public: @@ -67,6 +73,8 @@ class IsolateShim { // Symbols propertyIdRef JsPropertyIdRef GetSelfSymbolPropertyIdRef(); JsPropertyIdRef GetKeepAliveObjectSymbolPropertyIdRef(); + JsPropertyIdRef GetCachedSymbolPropertyIdRef( + CachedSymbolPropertyIdRef cachedSymbolPropertyIdRef); // String propertyIdRef JsPropertyIdRef GetProxyTrapPropertyIdRef(ProxyTraps trap); @@ -97,10 +105,8 @@ class IsolateShim { static void CALLBACK JsContextBeforeCollectCallback( _In_ JsRef contextRef, _In_opt_ void *data); - JsPropertyIdRef EnsurePrivateSymbol(JsPropertyIdRef * propertyIdRefPtr); - JsRuntimeHandle runtime; - JsPropertyIdRef selfSymbolPropertyIdRef; + JsPropertyIdRef symbolPropertyIdRefs[CachedSymbolPropertyIdRef::SymbolCount]; JsPropertyIdRef cachedPropertyIdRefs[CachedPropertyIdRef::Count]; bool isDisposing; diff --git a/deps/chakrashim/src/jsrtutils.cc b/deps/chakrashim/src/jsrtutils.cc index 6a44556597c..35ad621ed7a 100644 --- a/deps/chakrashim/src/jsrtutils.cc +++ b/deps/chakrashim/src/jsrtutils.cc @@ -76,12 +76,7 @@ JsErrorCode GetProperty(_In_ JsValueRef ref, return error; } - error = JsConvertValueToNumber(value, &value); - if (error != JsNoError) { - return error; - } - - return JsNumberToInt(value, intValue); + return ValueToIntLikely(value, intValue); } JsErrorCode SetProperty(_In_ JsValueRef ref, @@ -200,12 +195,7 @@ JsErrorCode CallGetter( return error; } - error = JsConvertValueToNumber(value, &value); - if (error != JsNoError) { - return error; - } - - return JsNumberToInt(value, result); + return ValueToIntLikely(value, result); } JsErrorCode GetPropertyOfGlobal(_In_ const wchar_t *propertyName, diff --git a/deps/chakrashim/src/jsrtutils.h b/deps/chakrashim/src/jsrtutils.h index 494f366097c..3a0bc72b23c 100644 --- a/deps/chakrashim/src/jsrtutils.h +++ b/deps/chakrashim/src/jsrtutils.h @@ -363,4 +363,50 @@ class JsArguments { // create and set the exception on the context void SetOutOfMemoryErrorIfExist(_In_ JsErrorCode errorCode); + +template +JsErrorCode ValueToNative(const JsConvertToValueFunc& JsConvertToValue, + const JsValueToNativeFunc& JsValueToNative, + JsValueRef value, T* nativeValue) { + JsErrorCode error; + + // If LIKELY, try JsValueToNative first. Likely to succeed. + if (LIKELY) { + error = JsValueToNative(value, nativeValue); + if (error != JsErrorInvalidArgument) { + return error; + } + } + + // Perform JS conversion first, then to native. + error = JsConvertToValue(value, &value); + if (error != JsNoError) { + return error; + } + return JsValueToNative(value, nativeValue); +} + +inline JsErrorCode ValueToInt(JsValueRef value, int* intValue) { + return ValueToNative( + JsConvertValueToNumber, JsNumberToInt, value, intValue); +} + +inline JsErrorCode ValueToIntLikely(JsValueRef value, int* intValue) { + return ValueToNative( + JsConvertValueToNumber, JsNumberToInt, value, intValue); +} + +inline JsErrorCode ValueToDouble(JsValueRef value, double* dblValue) { + return ValueToNative( + JsConvertValueToNumber, JsNumberToDouble, value, dblValue); +} + +inline JsErrorCode ValueToDoubleLikely(JsValueRef value, double* dblValue) { + return ValueToNative( + JsConvertValueToNumber, JsNumberToDouble, value, dblValue); +} + } // namespace jsrt diff --git a/deps/chakrashim/src/v8external.cc b/deps/chakrashim/src/v8external.cc index 46a725ad646..33c10a87a11 100644 --- a/deps/chakrashim/src/v8external.cc +++ b/deps/chakrashim/src/v8external.cc @@ -25,8 +25,6 @@ namespace v8 { using jsrt::IsolateShim; -const wchar_t * EXTERNAL_PROP_NAME = L"__isexternal__"; - Local External::Wrap(void* data) { return External::New(Isolate::GetCurrent(), data); } @@ -46,14 +44,15 @@ Local External::New(Isolate* isolate, void* value) { return Local(); } - JsValueRef trueRef = - IsolateShim::FromIsolate(isolate)->GetCurrentContextShim()->GetTrue(); + IsolateShim* iso = IsolateShim::FromIsolate(isolate); + JsValueRef trueRef = iso->GetCurrentContextShim()->GetTrue(); if (JsGetTrueValue(&trueRef) != JsNoError) { return Local(); } if (jsrt::DefineProperty(externalRef, - EXTERNAL_PROP_NAME, + iso->GetCachedSymbolPropertyIdRef( + jsrt::CachedSymbolPropertyIdRef::__isexternal__), jsrt::PropertyDescriptorOptionValues::False, jsrt::PropertyDescriptorOptionValues::False, jsrt::PropertyDescriptorOptionValues::False, @@ -67,17 +66,13 @@ Local External::New(Isolate* isolate, void* value) { } bool External::IsExternal(const v8::Value* value) { - if (!value->IsObject()) { - return false; - } - - JsPropertyIdRef propIdRef; - if (JsGetPropertyIdFromName(EXTERNAL_PROP_NAME, &propIdRef) != JsNoError) { - return false; - } + IsolateShim* iso = IsolateShim::GetCurrent(); bool hasProp; - if (JsHasProperty((JsValueRef)value, propIdRef, &hasProp) != JsNoError) { + if (JsHasProperty((JsValueRef)value, + iso->GetCachedSymbolPropertyIdRef( + jsrt::CachedSymbolPropertyIdRef::__isexternal__), + &hasProp) != JsNoError) { return false; } diff --git a/deps/chakrashim/src/v8handlescope.cc b/deps/chakrashim/src/v8handlescope.cc index 0765f31b3e2..78aa65fdd99 100644 --- a/deps/chakrashim/src/v8handlescope.cc +++ b/deps/chakrashim/src/v8handlescope.cc @@ -27,6 +27,7 @@ __declspec(thread) HandleScope *current = nullptr; HandleScope::HandleScope(Isolate* isolate) : _prev(current), + _locals(), _refs(JS_INVALID_REFERENCE), _count(0), _contextRef(JS_INVALID_REFERENCE), @@ -52,6 +53,11 @@ HandleScope *HandleScope::GetCurrent() { } bool HandleScope::AddLocal(JsValueRef value) { + if (_count < _countof(_locals)) { + _locals[_count++] = value; + return true; + } + if (_refs == JS_INVALID_REFERENCE) { if (JsCreateArray(1, &_refs) != JsNoError) { return AddLocalAddRef(value); @@ -60,7 +66,7 @@ bool HandleScope::AddLocal(JsValueRef value) { JsValueRef index; - if (JsIntToNumber(_count, &index) != JsNoError) { + if (JsIntToNumber(_count - _countof(_locals), &index) != JsNoError) { return AddLocalAddRef(value); } diff --git a/deps/chakrashim/src/v8value.cc b/deps/chakrashim/src/v8value.cc index e7dee2bfca3..dfd30a301e6 100644 --- a/deps/chakrashim/src/v8value.cc +++ b/deps/chakrashim/src/v8value.cc @@ -25,6 +25,8 @@ namespace v8 { +using jsrt::ContextShim; + static bool IsOfType(const Value* ref, JsValueType type) { JsValueType valueType; if (JsGetValueType(const_cast(ref), &valueType) != JsNoError) { @@ -33,8 +35,11 @@ static bool IsOfType(const Value* ref, JsValueType type) { return valueType == type; } -static bool IsOfType(const Value* ref, const wchar_t* type) { - return jsrt::IsOfGlobalType(const_cast(ref), type); +static bool IsOfType(const Value* ref, ContextShim::GlobalType index) { + bool result; + return jsrt::InstanceOf(const_cast(ref), + ContextShim::GetCurrent()->GetGlobalType(index), + &result) == JsNoError && result; } bool Value::IsUndefined() const { @@ -97,7 +102,7 @@ bool Value::IsTypedArray() const { } bool Value::IsUint8Array() const { - return IsTypedArray() && IsOfType(this, L"Uint8Array"); + return IsOfType(this, ContextShim::GlobalType::Uint8Array); } bool Value::IsBoolean() const { @@ -144,33 +149,33 @@ bool Value::IsUint32() const { } bool Value::IsDate() const { - return IsOfType(this, L"Date"); + return IsOfType(this, ContextShim::GlobalType::Date); } bool Value::IsBooleanObject() const { - return IsOfType(this, L"Boolean"); + return IsOfType(this, ContextShim::GlobalType::Boolean); } bool Value::IsNumberObject() const { - return IsOfType(this, L"Number"); + return IsOfType(this, ContextShim::GlobalType::Number); } bool Value::IsStringObject() const { - return IsOfType(this, L"String"); + return IsOfType(this, ContextShim::GlobalType::String); } bool Value::IsNativeError() const { - return IsOfType(this, L"Error") - || IsOfType(this, L"EvalError") - || IsOfType(this, L"RangeError") - || IsOfType(this, L"ReferenceError") - || IsOfType(this, L"SyntaxError") - || IsOfType(this, L"TypeError") - || IsOfType(this, L"URIError"); + return IsOfType(this, ContextShim::GlobalType::Error) + || IsOfType(this, ContextShim::GlobalType::EvalError) + || IsOfType(this, ContextShim::GlobalType::RangeError) + || IsOfType(this, ContextShim::GlobalType::ReferenceError) + || IsOfType(this, ContextShim::GlobalType::SyntaxError) + || IsOfType(this, ContextShim::GlobalType::TypeError) + || IsOfType(this, ContextShim::GlobalType::URIError); } bool Value::IsRegExp() const { - return IsOfType(this, L"RegExp"); + return IsOfType(this, ContextShim::GlobalType::RegExp); } Local Value::ToBoolean(Isolate* isolate) const { @@ -235,40 +240,21 @@ Local Value::ToInt32(Isolate* isolate) const { } bool Value::BooleanValue() const { - JsValueRef ref; - if (IsBoolean()) { - ref = (JsValueRef)this; - } else { - if (JsConvertValueToBoolean((JsValueRef)this, &ref) != JsNoError) { - return false; - } - } - bool value; - - if (JsBooleanToBool((JsValueRef)this, &value) != JsNoError) { + if (jsrt::ValueToNative(JsConvertValueToBoolean, + JsBooleanToBool, + (JsValueRef)this, + &value) != JsNoError) { return false; } - return value; } double Value::NumberValue() const { - JsValueRef ref; - if (IsNumber()) { - ref = (JsValueRef)this; - } else { - if (JsConvertValueToNumber((JsValueRef)this, &ref) != JsNoError) { - return 0; - } - } - double value; - - if (JsNumberToDouble(ref, &value) != JsNoError) { - return false; + if (jsrt::ValueToDoubleLikely((JsValueRef)this, &value) != JsNoError) { + return 0; } - return value; } @@ -281,16 +267,10 @@ uint32_t Value::Uint32Value() const { } int32_t Value::Int32Value() const { - JsValueRef ref; - if (JsConvertValueToNumber((JsValueRef)this, &ref) != JsNoError) { - return 0; - } - int intValue; - if (JsNumberToInt(ref, &intValue) != JsNoError) { + if (jsrt::ValueToIntLikely((JsValueRef)this, &intValue) != JsNoError) { return 0; } - return intValue; }