Clean-up some code involving ObjectGroup

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P2
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

  • JSObject::splicePrototype and SetClassAndProto are passing a Class* around, even though the object using that class is also available.
  • There is still some code to process preliminary objects for array objects, which shouldn't be needed anymore because of bug 1398768.
  • TypedArray creation always checks useSingletonForAllocationSite, but the result is always GenericObject.

SetClassAndProto is called with obj and obj->getClass(), but it doesn't look like it's required to pass the Class* separately, because we can always call obj->getClass() to retrieve it ourselves. So remove the Class* argument and rename the function to SetProto because there's no longer Class which gets changed.

A similar change is possible for JSObject::splicePrototype, which is also always called with an object and its corresponding class.

Drive-by change:
Move JSObject::shouldSplicePrototype to GlobalObject::shouldSplicePrototype, because the method is only called for global objects nowadays. And also remove the singleton check, because global objects are always singletons.

Attachment #9037601 - Flags: review?(jdemooij)

ObjectGroup::maybePreliminaryObjectsDontCheckGeneration() always returns null when called for ObjectGroups of ArrayObjects, so we can change a couple of tests to assertions.

Attachment #9037603 - Flags: review?(jdemooij)

ObjectGroup::useSingletonForAllocationSite always returns GenericObject for TypedArrays, so there's currently no point calling it in the TypedArray constructor. This should help a bit for bug 1520286, where the profile shows we spend about 5% of the time of TypedArrayObjectTemplate::class_constructor in ObjectGroup::useSingletonForAllocationSite.

And a couple more drive-by changes:

  • Change the one remaining caller of useSingletonForAllocationSite(JSScript*, jsbytecode*, const Class*) to use the JSProtoKey version.
  • Use the new TypedArrayObjectTemplate::protoKey() method in TypedArray code instead of using JSCLASS_CACHED_PROTO_KEY(Class*).
  • Also change one JSCLASS_CACHED_PROTO_KEY with a constant Class* to directly use the JSProtoKey.
  • Remove a few unnecessary switch-cases in GetClassForProtoKey in ObjectGroup.cpp.
  • Directly use the ArrayObject class in ObjectGroupRealm::getStringSplitStringGroup and remove the unnecessary initialFlags parameter.
  • And finally add a helper to call NewBuiltinClassInstance to TypedArrayObjectTemplate to avoid some code duplication.
Attachment #9037607 - Flags: review?(jdemooij)
Attachment #9037601 - Flags: review?(jdemooij) → review+
Comment on attachment 9037603 [details] [diff] [review]
bug1521127-part2-array-preliminary-object-checks.patch

Review of attachment 9037603 [details] [diff] [review]:
-----------------------------------------------------------------

Good find, these checks are unnecessary now that unboxed arrays are gone.

::: js/src/jit/BaselineIC.cpp
@@ +3317,5 @@
>            ObjectGroup::callingAllocationSiteGroup(cx, JSProto_Array);
>        if (!group) {
>          return false;
>        }
> +      MOZ_ASSERT(!group->maybePreliminaryObjectsDontCheckGeneration());

ObjectGroup::callingAllocationSiteGroup is pretty slow and now it's only used for this assertion (since unboxed array removal). I think we should just remove the callingAllocationSiteGroup call and this assertion.

(I'd also be OK with removing the assertions elsewhere. Preliminary objects are PlainObject-specific now I think and we don't assert this for other object types.)

::: js/src/jit/CacheIR.cpp
@@ +4982,5 @@
>    RootedArrayObject thisarray(cx_, &thisobj->as<ArrayObject>());
>  
>    // And the object group for the array is not collecting preliminary objects.
>    AutoSweepObjectGroup sweep(thisobj->group());
> +  MOZ_ASSERT(!thisobj->group()->maybePreliminaryObjects(sweep));

We could change this to maybePreliminaryObjectsDontCheckGeneration and remove |sweep|.

::: js/src/jit/MCallOptimize.cpp
@@ +1841,5 @@
>    if (!group) {
>      return InliningStatus_NotInlined;
>    }
>    AutoSweepObjectGroup sweep(group);
> +  MOZ_ASSERT(!group->maybePreliminaryObjects(sweep));

Same here.
Attachment #9037603 - Flags: review?(jdemooij) → review+
Blocks: 1521526
Comment on attachment 9037607 [details] [diff] [review]
bug1521127-part3-typedarray-singleton-allocation-site.patch

Review of attachment 9037607 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice.
Attachment #9037607 - Flags: review?(jdemooij) → review+

(In reply to Jan de Mooij [:jandem] from comment #4)

ObjectGroup::callingAllocationSiteGroup is pretty slow and now it's only
used for this assertion (since unboxed array removal). I think we should
just remove the callingAllocationSiteGroup call and this assertion.

(I'd also be OK with removing the assertions elsewhere. Preliminary objects
are PlainObject-specific now I think and we don't assert this for other
object types.)

I've updated the patch to simply remove all callers to |maybePreliminaryObjects| and |maybePreliminaryObjectsDontCheckGeneration| for ArrayObjects. Is that what you had in mind?

Attachment #9037603 - Attachment is obsolete: true
Attachment #9038220 - Flags: review?(jdemooij)
Priority: -- → P2
Comment on attachment 9038220 [details] [diff] [review]
bug1521127-part2-array-preliminary-object-checks.patch

Review of attachment 9038220 [details] [diff] [review]:
-----------------------------------------------------------------

Yes. Thanks!
Attachment #9038220 - Flags: review?(jdemooij) → review+

Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac126ad6e18
Part 3: Don't check for singleton allocation sites when creating typed arrays. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/232668044fbf
Part 2: Remove more preliminary objects tracking for array objects. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/10f6f2e55f09
Part 1: Remove js::Class* argument from SetClassAndProto and splicePrototype. r=jandem

Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c244b0217fb
Part 1: Remove js::Class* argument from SetClassAndProto and splicePrototype. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ba74734504
Part 2: Remove more preliminary objects tracking for array objects. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/f052949e151b
Part 3: Don't check for singleton allocation sites when creating typed arrays. r=jandem

Relanded with correct patch order.

Flags: needinfo?(andrebargull)

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #12)

Relanded with correct patch order.

Danke schön! :-)

You need to log in before you can comment on or make changes to this bug.