1521127 - Clean-up some code involving ObjectGroup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[André Bargull [:anba]]

• 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.

Comment 1

[André Bargull [:anba]]

Attachment: bug1521127-part1-remove-class-argument.patch

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.

Comment 2

[André Bargull [:anba]]

Attachment: bug1521127-part2-array-preliminary-object-checks.patch

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

Comment 3

[André Bargull [:anba]]

Attachment: bug1521127-part3-typedarray-singleton-allocation-site.patch

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.

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 9037607 [details] [diff] [review]
bug1521127-part3-typedarray-singleton-allocation-site.patch

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

Very nice.

Comment 6

[André Bargull [:anba]]

Attachment: bug1521127-part2-array-preliminary-object-checks.patch

(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?

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9038220 [details] [diff] [review]
bug1521127-part2-array-preliminary-object-checks.patch

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

Yes. Thanks!

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb80152ba029fdc34a99a06dd154450348b9f8f2

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backed out 3 changesets (bug 1521127) for build bustages at builds/worker/workspace/build/src/js/src/vm/ObjectGroup.cpp

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f92e62a285c1e626ecb745857d6177bbaf660cc

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=10f6f2e55f095b42b71617da52b1c99245dbfb75

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223494657&repo=mozilla-inbound&lineNumber=11053

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Relanded with correct patch order.

Comment 13

[André Bargull [:anba]]

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

Relanded with correct patch order.

Danke schön! :-)

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8c244b0217fb
https://hg.mozilla.org/mozilla-central/rev/c9ba74734504
https://hg.mozilla.org/mozilla-central/rev/f052949e151b