From 648ba1f7dd7a41151983ec93220803ffd84685aa Mon Sep 17 00:00:00 2001 From: Mythri Date: Thu, 7 Feb 2019 13:19:57 +0000 Subject: [PATCH] Defer inferring language mode for PropertyCallbackInfo This cl: https://chromium-review.googlesource.com/c/v8/v8/+/1421077 changed the implementation of SetProperty to infer the language mode. Language mode is only required when there is an error to decide if we have to throw an error or not. However we used to compute language mode eagerly for PropertyCallbackInfo. This causes regressions in some benchmarks. This cl changes it by deferring it further by computing it only when it is actually required. BUG: v8:8580, chromium:925289 Change-Id: Iba70ec5f9bb3deec16414a1ec418b3963f2144f9 Reviewed-on: https://chromium-review.googlesource.com/c/1454608 Reviewed-by: Adam Klein Reviewed-by: Yang Guo Reviewed-by: Toon Verwaest Commit-Queue: Mythri Alle Cr-Commit-Position: refs/heads/master@{#59450} --- include/v8-internal.h | 11 +++++++++++ include/v8.h | 8 ++++++-- src/api-arguments.cc | 14 ++++++++------ src/api-arguments.h | 2 +- src/api.cc | 5 +++++ src/globals.h | 5 ++++- src/ic/ic.cc | 11 +++++------ src/keys.cc | 4 ++-- src/objects.cc | 8 ++++---- src/objects/js-objects.cc | 14 ++++++-------- 10 files changed, 52 insertions(+), 30 deletions(-) diff --git a/include/v8-internal.h b/include/v8-internal.h index 7f9c27ebb9..b67924381e 100644 --- a/include/v8-internal.h +++ b/include/v8-internal.h @@ -182,6 +182,12 @@ class Internals { static const int kUndefinedOddballKind = 5; static const int kNullOddballKind = 3; + // Constants used by PropertyCallbackInfo to check if we should throw when an + // error occurs. + static const int kThrowOnError = 0; + static const int kDontThrow = 1; + static const int kInferShouldThrowMode = 2; + // Soft limit for AdjustAmountofExternalAllocatedMemory. Trigger an // incremental GC once the external memory reaches this limit. static constexpr int kExternalAllocationSoftLimit = 64 * 1024 * 1024; @@ -367,6 +373,11 @@ V8_INLINE void PerformCastCheck(T* data) { // that's guaranteed to never be in ReadOnlySpace. V8_EXPORT internal::Isolate* IsolateFromNeverReadOnlySpaceObject(Address obj); +// Returns if we need to throw when an error occurs. This infers the language +// mode based on the current context and the closure. This returns true if the +// language mode is strict. +V8_EXPORT bool ShouldThrowOnError(v8::internal::Isolate* isolate); + } // namespace internal } // namespace v8 diff --git a/include/v8.h b/include/v8.h index 5586298e04..075098440b 100644 --- a/include/v8.h +++ b/include/v8.h @@ -10723,10 +10723,14 @@ ReturnValue PropertyCallbackInfo::GetReturnValue() const { template bool PropertyCallbackInfo::ShouldThrowOnError() const { typedef internal::Internals I; - return args_[kShouldThrowOnErrorIndex] != I::IntToSmi(0); + if (args_[kShouldThrowOnErrorIndex] != + I::IntToSmi(I::kInferShouldThrowMode)) { + return args_[kShouldThrowOnErrorIndex] != I::IntToSmi(I::kDontThrow); + } + return v8::internal::ShouldThrowOnError( + reinterpret_cast(GetIsolate())); } - Local Undefined(Isolate* isolate) { typedef internal::Address S; typedef internal::Internals I; diff --git a/src/api-arguments.cc b/src/api-arguments.cc index b706050b30..76e821cad7 100644 --- a/src/api-arguments.cc +++ b/src/api-arguments.cc @@ -9,17 +9,19 @@ namespace v8 { namespace internal { -PropertyCallbackArguments::PropertyCallbackArguments(Isolate* isolate, - Object data, Object self, - JSObject holder, - ShouldThrow should_throw) +PropertyCallbackArguments::PropertyCallbackArguments( + Isolate* isolate, Object data, Object self, JSObject holder, + Maybe should_throw) : Super(isolate) { slot_at(T::kThisIndex).store(self); slot_at(T::kHolderIndex).store(holder); slot_at(T::kDataIndex).store(data); slot_at(T::kIsolateIndex).store(Object(reinterpret_cast
(isolate))); - slot_at(T::kShouldThrowOnErrorIndex) - .store(Smi::FromInt(should_throw == kThrowOnError ? 1 : 0)); + int value = Internals::kInferShouldThrowMode; + if (should_throw.IsJust()) { + value = should_throw.FromJust(); + } + slot_at(T::kShouldThrowOnErrorIndex).store(Smi::FromInt(value)); // Here the hole is set as default value. // It cannot escape into js as it's removed in Call below. diff --git a/src/api-arguments.h b/src/api-arguments.h index 6b025bdbb3..4f1ea8c85a 100644 --- a/src/api-arguments.h +++ b/src/api-arguments.h @@ -72,7 +72,7 @@ class PropertyCallbackArguments static const int kShouldThrowOnErrorIndex = T::kShouldThrowOnErrorIndex; PropertyCallbackArguments(Isolate* isolate, Object data, Object self, - JSObject holder, ShouldThrow should_throw); + JSObject holder, Maybe should_throw); // ------------------------------------------------------------------------- // Accessor Callbacks diff --git a/src/api.cc b/src/api.cc index ec48391827..b016038836 100644 --- a/src/api.cc +++ b/src/api.cc @@ -3660,6 +3660,11 @@ i::Isolate* i::IsolateFromNeverReadOnlySpaceObject(i::Address obj) { i::HeapObject::cast(i::Object(obj))); } +bool i::ShouldThrowOnError(i::Isolate* isolate) { + return i::GetShouldThrow(isolate, Nothing()) == + i::ShouldThrow::kThrowOnError; +} + void i::Internals::CheckInitializedImpl(v8::Isolate* external_isolate) { i::Isolate* isolate = reinterpret_cast(external_isolate); Utils::ApiCheck(isolate != nullptr && !isolate->IsDead(), diff --git a/src/globals.h b/src/globals.h index 91903b582d..402a280166 100644 --- a/src/globals.h +++ b/src/globals.h @@ -800,7 +800,10 @@ enum WhereToStart { kStartAtReceiver, kStartAtPrototype }; enum ResultSentinel { kNotFound = -1, kUnsupported = -2 }; -enum ShouldThrow { kThrowOnError, kDontThrow }; +enum ShouldThrow { + kThrowOnError = Internals::kThrowOnError, + kDontThrow = Internals::kDontThrow +}; // The Store Buffer (GC). typedef enum { diff --git a/src/ic/ic.cc b/src/ic/ic.cc index 7d1e5b5cd1..d131df409e 100644 --- a/src/ic/ic.cc +++ b/src/ic/ic.cc @@ -2710,9 +2710,8 @@ RUNTIME_FUNCTION(Runtime_StoreCallbackProperty) { DCHECK(info->IsCompatibleReceiver(*receiver)); - ShouldThrow should_throw = GetShouldThrow(isolate, Nothing()); PropertyCallbackArguments arguments(isolate, info->data(), *receiver, *holder, - should_throw); + Nothing()); arguments.CallAccessorSetter(info, name, value); RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate); return *value; @@ -2728,7 +2727,7 @@ RUNTIME_FUNCTION(Runtime_LoadCallbackProperty) { DCHECK(info->IsCompatibleReceiver(*receiver)); PropertyCallbackArguments custom_args(isolate, info->data(), *receiver, - *holder, kThrowOnError); + *holder, Just(kThrowOnError)); Handle result = custom_args.CallAccessorGetter(info, name); RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate); if (result.is_null()) return ReadOnlyRoots(isolate).undefined_value(); @@ -2776,7 +2775,7 @@ RUNTIME_FUNCTION(Runtime_LoadPropertyWithInterceptor) { Handle interceptor(holder->GetNamedInterceptor(), isolate); PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver, - *holder, kDontThrow); + *holder, Just(kDontThrow)); Handle result = arguments.CallNamedGetter(interceptor, name); RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate); @@ -2838,7 +2837,7 @@ RUNTIME_FUNCTION(Runtime_StorePropertyWithInterceptor) { DCHECK(!interceptor->non_masking()); PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver, - *receiver, kDontThrow); + *receiver, Just(kDontThrow)); Handle result = arguments.CallNamedSetter(interceptor, name, value); RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate); @@ -2870,7 +2869,7 @@ RUNTIME_FUNCTION(Runtime_LoadElementWithInterceptor) { Handle interceptor(receiver->GetIndexedInterceptor(), isolate); PropertyCallbackArguments arguments(isolate, interceptor->data(), *receiver, - *receiver, kDontThrow); + *receiver, Just(kDontThrow)); Handle result = arguments.CallIndexedGetter(interceptor, index); RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate); diff --git a/src/keys.cc b/src/keys.cc index 4e12e73f72..640fb76f45 100644 --- a/src/keys.cc +++ b/src/keys.cc @@ -489,7 +489,7 @@ void FilterForEnumerableProperties(Handle receiver, // args are invalid after args.Call(), create a new one in every iteration. PropertyCallbackArguments args(accumulator->isolate(), interceptor->data(), - *receiver, *object, kDontThrow); + *receiver, *object, Just(kDontThrow)); Handle element = accessor->Get(result, i); Handle attributes; @@ -521,7 +521,7 @@ Maybe CollectInterceptorKeysInternal(Handle receiver, IndexedOrNamed type) { Isolate* isolate = accumulator->isolate(); PropertyCallbackArguments enum_args(isolate, interceptor->data(), *receiver, - *object, kDontThrow); + *object, Just(kDontThrow)); Handle result; if (!interceptor->enumerator()->IsUndefined(isolate)) { diff --git a/src/objects.cc b/src/objects.cc index a8204efad5..46c65238ea 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -1379,7 +1379,7 @@ MaybeHandle Object::GetPropertyWithAccessor(LookupIterator* it) { } PropertyCallbackArguments args(isolate, info->data(), *receiver, *holder, - kDontThrow); + Just(kDontThrow)); Handle result = args.CallAccessorGetter(info, name); RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object); if (result.is_null()) return isolate->factory()->undefined_value(); @@ -1489,9 +1489,8 @@ Maybe Object::SetPropertyWithAccessor( // AccessorInfo was created by the API or internally (see accessors.cc). // Here we handle both cases using GenericNamedPropertySetterCallback and // its Call method. - ShouldThrow should_throw = GetShouldThrow(isolate, maybe_should_throw); PropertyCallbackArguments args(isolate, info->data(), *receiver, *holder, - should_throw); + maybe_should_throw); Handle result = args.CallAccessorSetter(info, name, value); // In the case of AccessorNameSetterCallback, we know that the result value // cannot have been set, so the result of Call will be null. In the case of @@ -1499,7 +1498,8 @@ Maybe Object::SetPropertyWithAccessor( // (signalling an exception) or a boolean Oddball. RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing()); if (result.is_null()) return Just(true); - DCHECK(result->BooleanValue(isolate) || should_throw == kDontThrow); + DCHECK(result->BooleanValue(isolate) || + GetShouldThrow(isolate, maybe_should_throw) == kDontThrow); return Just(result->BooleanValue(isolate)); } diff --git a/src/objects/js-objects.cc b/src/objects/js-objects.cc index 344c294fbd..9908ec267a 100644 --- a/src/objects/js-objects.cc +++ b/src/objects/js-objects.cc @@ -973,7 +973,7 @@ MaybeHandle GetPropertyWithInterceptorInternal( isolate, receiver, Object::ConvertReceiver(isolate, receiver), Object); } PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, kDontThrow); + *holder, Just(kDontThrow)); if (it->IsElement()) { result = args.CallIndexedGetter(interceptor, it->index()); @@ -1006,7 +1006,7 @@ Maybe GetPropertyAttributesWithInterceptorInternal( Nothing()); } PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, kDontThrow); + *holder, Just(kDontThrow)); if (!interceptor->query()->IsUndefined(isolate)) { Handle result; if (it->IsElement()) { @@ -1053,8 +1053,7 @@ Maybe SetPropertyWithInterceptorInternal( Nothing()); } PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, - GetShouldThrow(isolate, should_throw)); + *holder, should_throw); if (it->IsElement()) { // TODO(neis): In the future, we may want to actually return the @@ -1087,8 +1086,7 @@ Maybe DefinePropertyWithInterceptorInternal( Nothing()); } PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, - GetShouldThrow(isolate, should_throw)); + *holder, should_throw); std::unique_ptr descriptor( new v8::PropertyDescriptor()); @@ -1512,7 +1510,7 @@ Maybe GetPropertyDescriptorWithInterceptor(LookupIterator* it, } PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, kDontThrow); + *holder, Just(kDontThrow)); if (it->IsElement()) { result = args.CallIndexedDescriptor(interceptor, it->index()); } else { @@ -3538,7 +3536,7 @@ Maybe JSObject::DeletePropertyWithInterceptor(LookupIterator* it, } PropertyCallbackArguments args(isolate, interceptor->data(), *receiver, - *holder, should_throw); + *holder, Just(should_throw)); Handle result; if (it->IsElement()) { result = args.CallIndexedDeleter(interceptor, it->index());