Closed Bug 1431726 Opened 3 years ago Closed 3 years ago

Refactor GetBuiltinPrototype and friends


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox59 --- fixed


(Reporter: jandem, Assigned: jandem)



(5 files)

I've noticed GetBuiltinPrototype in profiles a few times. We can make it faster and remove some code duplication.
* We can use GlobalObject::ensureConstructor instead of MaybeResolveConstructor, as they do the same thing.

* We can move GlobalObject::ensureConstructor to the header file: it's 3 lines of code and it's called a lot.

* After that GlobalObject::resolveConstructor can be made private.

* We can use Handle<GlobalObject*> instead of Rooted<GlobalObject*> in a few places.
Attachment #8943931 - Flags: review?(andrebargull)
This uses GlobalObject::getOrCreateConstructor instead of GetBuiltinConstructor.

I went with this getOrCreateConstructor name for consistency with getOrCreateTypedArrayConstructor etc.

I also changed it to return JSObject* instead of bool + outparam.
Attachment #8943934 - Flags: review?(andrebargull)
Like the previous patch but for prototypes. getOrCreatePrototype is consistent with getOrCreateArrayPrototype etc.
Attachment #8943935 - Flags: review?(andrebargull)
This matches maybeGetArrayPrototype.

Also, this function is never called with JSProto_Null, so I changed the assert to check for that and removed the if-statement.
Attachment #8943936 - Flags: review?(andrebargull)
And finally, IsStandardPrototype can be a static function since it's only used in jsobj.cpp
Attachment #8943937 - Flags: review?(andrebargull)
Attachment #8943931 - Flags: review?(andrebargull) → review+
Attachment #8943934 - Flags: review?(andrebargull) → review+
Comment on attachment 8943935 [details] [diff] [review]
Part 3 - Use GlobalObject method instead of GetBuiltinPrototype

Review of attachment 8943935 [details] [diff] [review]:

::: js/src/vm/ObjectGroup.cpp
@@ +718,5 @@
>  /* static */ ObjectGroup*
>  ObjectGroup::defaultNewGroup(JSContext* cx, JSProtoKey key)
>  {
>      RootedObject proto(cx);

Do we still need to root |proto| here?

@@ +846,5 @@
>      RootedObjectGroup group(cx);
>      if (p) {
>          group = p->value();
>      } else {
> +        RootedObject proto(cx, GlobalObject::getOrCreateArrayPrototype(cx, cx->global()));

Rooting |proto| shouldn't be necessary anymore, right? (Because |proto| is later rooted through |taggedProto|.)

@@ +1169,5 @@
>      if (!p) {
>          if (!CanShareObjectGroup(properties, nproperties))
>              return NewPlainObjectWithProperties(cx, properties, nproperties, newKind);
> +        RootedObject proto(cx, GlobalObject::getOrCreatePrototype(cx, JSProto_Object));

Same here.

@@ +1670,5 @@
>      // for ObjectGroup::allocationSiteGroup().
>      const Class* clasp = GetClassForProtoKey(JSProto_Array);
> +    RootedObject proto(cx, GlobalObject::getOrCreateArrayPrototype(cx, cx->global()));

And here, too.
Attachment #8943935 - Flags: review?(andrebargull) → review+
Attachment #8943936 - Flags: review?(andrebargull) → review+
Comment on attachment 8943937 [details] [diff] [review]
Part 5 - Make IsStandardPrototype static

Review of attachment 8943937 [details] [diff] [review]:

Nice to see all this code duplication go away. I know that in the past I was more than once confused why we had these various constructor/prototype resolving functions which more or less did all the same! :-)
Attachment #8943937 - Flags: review?(andrebargull) → review+
You're right, we can definitely get rid of some roots. Some of these functions are pretty hot as well so it's nice :)
Pushed by
part 1 - Remove MaybeResolveConstructor, use GlobalObject::ensureConstructor instead. r=anba
part 2 - Replace GetBuiltinConstructor with GlobalObject::getOrCreateConstructor. r=anba
part 3 - Replace GetBuiltinPrototype with GlobalObject::getOrCreatePrototype. r=anba
part 4 - Replace GetBuiltinPrototypePure with GlobalObject::maybeGetPrototype. r=anba
part 5 - Make IsStandardPrototype static. r=anba
You need to log in before you can comment on or make changes to this bug.