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.
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
[JSC] Make species constructor for TypedArray fast #2694
[JSC] Make species constructor for TypedArray fast #2694
Conversation
b28ac40
to
50fff08
Compare
50fff08
to
6d9f674
Compare
6d9f674
to
d9579ba
Compare
present -> presence
|
void JSGlobalObject::installTypedArrayConstructorSpeciesWatchpoint(JSTypedArrayViewConstructor* constructor) | ||
{ | ||
VM& vm = this->vm(); | ||
PropertySlot slot(constructor, PropertySlot::InternalMethodType::VMInquiry, &vm); | ||
constructor->getOwnPropertySlot(constructor, this, vm.propertyNames->speciesSymbol.impl(), slot); | ||
constructor->structure()->startWatchingPropertyForReplacements(vm, slot.cachedOffset()); | ||
ObjectPropertyCondition speciesCondition = ObjectPropertyCondition::equivalence(vm, nullptr, constructor, vm.propertyNames->speciesSymbol.impl(), speciesGetterSetter()); | ||
m_typedArrayConstructorSpeciesWatchpoint = makeUnique<ObjectPropertyChangeAdaptiveWatchpoint<InlineWatchpointSet>>(this, speciesCondition, m_typedArrayConstructorSpeciesWatchpointSet); | ||
m_typedArrayConstructorSpeciesWatchpoint->install(vm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we assert somehow that this succeeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can assert it since this function is called when TypedArrayConstructor is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it is already asserted in ObjectPropertyChangeAdaptiveWatchpoint::install.
@@ -49,6 +49,7 @@ void JSTypedArrayViewConstructor::finishCreation(VM& vm, JSGlobalObject* globalO | |||
|
|||
JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->of, typedArrayConstructorOfCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum)); | |||
JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->from, typedArrayConstructorFromCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum)); | |||
globalObject->installTypedArrayConstructorSpeciesWatchpoint(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we normally put these calls in JSGlobalObject itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are managing these watchpoints in JSGlobalObject (1) to suppress size increase of these constructors and (2) these constructors are only created per JSGlobalObject. And we already have this kind of methods for different types in JSGlobalObject. So I think JSGlobalObject is the right place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking if we should put this call inside JSGlobalObject instead of JSTypedArrayViewConstructor (just from a stylistic perspective)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here is fine since (1) Set / Map prototypes are already doing the same thing. And (2) for Set and Map, it is hard to call it nicely since they are batch-initialized via FOR_EACH_LAZY_BUILTIN_TYPE
macro. So, for consistency, I think putting here is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
auto* prototype = globalObject->typedArrayPrototype(ViewClass::TypedArrayStorageType); | ||
|
||
if (globalObject->typedArraySpeciesWatchpointSet(ViewClass::TypedArrayStorageType).state() == ClearWatchpoint) { | ||
globalObject->tryInstallTypedArraySpeciesWatchpoint(ViewClass::TypedArrayStorageType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only do this once, how do we guarantee that the __proto__
of something like Uint8Array is TypedArray?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tryInstallTypedArraySpeciesWatchpoint
is checking this exact thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And PropertyCondition::absence
keeps guaranteeing this thing (see commit message for more detail) :)
|
||
protected: | ||
void tryInstallSpeciesWatchpoint(JSObject* prototype, JSObject* constructor, std::unique_ptr<ObjectPropertyChangeAdaptiveWatchpoint<InlineWatchpointSet>>& constructorWatchpoint, std::unique_ptr<ObjectPropertyChangeAdaptiveWatchpoint<InlineWatchpointSet>>& speciesWatchpoint, InlineWatchpointSet& speciesWatchpointSet); | ||
enum class SpeciesEmpty : bool { Yes, No }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think I'd name this "HasSpecies" or "HasSpeciesProperty", and invert the uses of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
speciesCondition = ObjectPropertyCondition::equivalence(vm, prototype, constructor, vm.propertyNames->speciesSymbol.impl(), speciesGetterSetter()); | ||
break; | ||
case SpeciesEmpty::Yes: | ||
speciesCondition = ObjectPropertyCondition::absence(vm, prototype, constructor, vm.propertyNames->speciesSymbol.impl(), jsDynamicCast<JSObject*>(constructor->getPrototypeDirect())); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the prototype
the owner
? Shouldn't it be the global object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is not so important since we do not need write-barrier. For now, just using this
, but in the future, it should be nullptr
(But I don't change it in this patch since I'm not planning to change all the other things too).
FOR_EACH_TYPED_ARRAY_TYPE(DECLARE_TYPED_ARRAY_TYPE_WATCHPOINT) | ||
#undef DECLARE_TYPED_ARRAY_TYPE_WATCHPOINT | ||
|
||
std::unique_ptr<ObjectAdaptiveStructureWatchpoint>& typedArrayConstructorSpeciesWatchpoint(TypedArrayType type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's a bit verbose, but can we name this so it's clear it's the absence watchpoint? Same with ObjectAdaptiveStructureWatchpoint fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually checking 2 things (species absence and constructor equivalence), and only one of that is absence. If we use absence here, it is a bit misleading since this watchpoint-set is also ensuring that constructor
property's equivalence too.
I'll rename it to typedArrayConstructorSpeciesFastPathWatchpoint
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no, this is absence one, I'll change it to include absence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is renamed to typedArrayConstructorSpeciesAbsenceWatchpoint
tryInstallSpeciesWatchpoint(arrayBufferPrototype(sharingMode), arrayBufferConstructor(sharingMode), m_arrayBufferPrototypeConstructorWatchpoints[index], m_arrayBufferConstructorSpeciesWatchpoints[index], arrayBufferSpeciesWatchpointSet(sharingMode), SpeciesEmpty::No); | ||
} | ||
|
||
void JSGlobalObject::tryInstallTypedArraySpeciesWatchpoint(TypedArrayType type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also put absence in the name here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is not exclusive to absence. So renaming it to FastPath etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, maybe, for now, it is good name for this function, since it is both ensuring things to make species construct fast. Including absence etc. is misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is kept as is since it is not exclusive to absence one.
d9579ba
to
2b33d61
Compare
206a1cc
to
5f2b963
Compare
https://bugs.webkit.org/show_bug.cgi?id=243148 Reviewed by Saam Barati. This patch accelerates TypedArray species-construct operation since it is consuming much time in TypedArray#slice. The concept is that we should do the same optimization to ArraySpeciesCreate. But difficulty is that TypedArray constructor is a bit awkwardly designed. So we need to carefully ensure the invariants. TypedArray constructor's inheritance chain is that, Uint8Array (It does *not* have @@species getter) | v TypedArray (This *has* @@species getter) | v Function | v Object And prototype is, Uint8Array.prototype (which has "constructor" property to Uint8Array) | v TypedArray.prototype (which has various TypedArray functions) | v ... And in species construct, we first get "constructor" property of the instance, which looks up Uint8Array.prototype.constructor. Then, from this constructor, we get @@species field (invoking a getter). It looks up TypedArray[@@species] and it returns |this|, in this case, Uint8Array. And this is how species constructor is obtained. In this patch, we have two WatchpointSet. 1. typedArraySpeciesWatchpointSet. It ensures that (1) Uint8Array constructor does not have @@species getter, (2) Uint8Array constructor's __proto__ is TypedArray constructor, (3) and Uint8Array.prototype.constructor is Uint8Array constructor. 2. typedArrayConstructorSpeciesWatchpointSet. It ensures that TypedArray constructor has the default @@species getter. For 1's (1) and (2), we track Uint8Array's @@species absence and its __proto__ via ObjectPropertyCondition::absence. We introduce ObjectAdaptiveStructureWatchpoint, which looks similar to ObjectPropertyChangeAdaptiveWatchpoint, but it is usable for absence and presence. And 1's (3) is ensured by using ObjectPropertyChangeAdaptiveWatchpoint. For 2, we use ObjectPropertyChangeAdaptiveWatchpoint. If these conditions are met, we can skip "constructor" and @@species property lookups if (1) the instance does not have properties (which can shadow "constructor") and (2) the instance's __proto__ is Uint8Array.prototype. speciesWatchpointIsValid checks them. Attached microbenchmark showed 2x better score. ToT Patched typed-array-slice-hot 239.3897+-0.3691 ^ 118.0493+-0.2077 ^ definitely 2.0279x faster * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj: * Source/JavaScriptCore/bytecode/Watchpoint.cpp: * Source/JavaScriptCore/bytecode/Watchpoint.h: * Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h: (JSC::speciesWatchpointIsValid): (JSC::speciesConstruct): * Source/JavaScriptCore/runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::JSGlobalObject): (JSC::JSGlobalObject::tryInstallSpeciesWatchpoint): (JSC::JSGlobalObject::tryInstallArraySpeciesWatchpoint): (JSC::JSGlobalObject::tryInstallArrayBufferSpeciesWatchpoint): (JSC::JSGlobalObject::tryInstallTypedArraySpeciesWatchpoint): (JSC::JSGlobalObject::installTypedArrayConstructorSpeciesWatchpoint): * Source/JavaScriptCore/runtime/JSGlobalObject.h: (JSC::JSGlobalObject::typedArrayConstructorSpeciesWatchpoint): (JSC::JSGlobalObject::typedArrayPrototypeConstructorWatchpoint): (JSC::JSGlobalObject::typedArraySpeciesWatchpointSet): (JSC::JSGlobalObject::typedArrayConstructorSpeciesWatchpointSet): (JSC::JSGlobalObject::typedArrayPrototype const): * Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.cpp: (JSC::JSTypedArrayViewConstructor::finishCreation): * Source/JavaScriptCore/runtime/ObjectAdaptiveStructureWatchpoint.h: Added. (JSC::ObjectAdaptiveStructureWatchpoint::install): (JSC::ObjectAdaptiveStructureWatchpoint::fireInternal): * JSTests/stress/typed-array-slice-constructor-replace.js: Added. (shouldBe): * JSTests/stress/typed-array-slice-own-extend-but-fast.js: Added. (shouldBe): * JSTests/stress/typed-array-slice-parent-replace.js: Added. (shouldBe): (Middle): * JSTests/stress/typed-array-slice-parent-species-replace.js: Added. (shouldBe): * JSTests/stress/typed-array-slice-proto-constructor-replace.js: Added. (shouldBe): * JSTests/stress/typed-array-slice-proto-replace.js: Added. (shouldBe): * JSTests/stress/typed-array-slice-species-define.js: Added. (shouldBe): Canonical link: https://commits.webkit.org/252847@main
5f2b963
to
b4e6f93
Compare
Committed 252847@main (b4e6f93): https://commits.webkit.org/252847@main Reviewed commits have been landed. Closing PR #2694 and removing active labels. |
b4e6f93