1261720 - Reduce static data sizes by separating js::ClassExtension from js::Class

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1259194 and bug 1260984 moved ObjectOps and ClassSpec out of js::Class. This bug is about doing the same with ClassExtension.

The win is smaller (about 27 KiB) but it's not hard and makes js::Class more consistent, so it seems worth doing.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Separate js::ClassExtension from js::Class

js::ClassExtension is often all null. When it's not all null, it's often
duplicated among classes. By pulling it out into its own struct, and using a
(possibly null) pointer in js::Class, we can save 17 KiB per process on
64-bit, and half that on 32-bit.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move ClassExtension::isWrappedNative into js::Class::flags

Among the JSCLASS_* constants, 1<<2 was unused so I snaffled it. (1<<5 was
recently taken similarly in bug 1254968.)

Please let me know if this might cause any problems.

Comment 3

[Nicholas Nethercote [inactive]]

I considered merging ClassSpec and ClassExtension, but in practice they are mutually exclusive -- we have no classes in the tree for which they are both non-null -- and so merging them would have increased our data sizes.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8737653 [details] [diff] [review]
(part 1) - Separate js::ClassExtension from js::Class

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

Oh, and r?bz for the dom/ parts, please.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8737653 [details] [diff] [review]
(part 1) - Separate js::ClassExtension from js::Class

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

Thanks, this really is better in a few places.

::: js/xpconnect/src/Sandbox.cpp
@@ +572,5 @@
> +static const js::ClassExtension SandboxWriteToProtoClassExtension = {
> +    false,        /* isWrappedNative */
> +    nullptr,      /* weakmapKeyDelegateOp */
> +    sandbox_moved /* objectMovedOp */
> +};

This is the same as SandboxClassExtension.

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +1256,5 @@
>                                   true, nullptr, nullptr, si,
>                                   JSPROP_ENUMERATE, resolvep);
>  }
>  
> +static const js::ClassExtension XPC_WN_SHARED_PROTO_CLASS_EXT = {

Style nit: Please change the name to mixed case.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8737653 [details] [diff] [review]
(part 1) - Separate js::ClassExtension from js::Class

It might have made some sense to keep the PROXY_CLASS_WITH_EXT macro... anyway, not that big a deal.

>+++ b/dom/workers/WorkerScope.cpp

This code is gone on inbound as of this morning.  It's over in dom/bindings/SimpleGlobalObject.cpp now.  Should be a pretty straightforward merge...

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8737653 [details] [diff] [review]
(part 1) - Separate js::ClassExtension from js::Class

>+static const js::ClassExtension XPC_WN_NoHelper_JSClassExtension = {

Given that it's used for the helper case too, probably better to call this XPC_WN_JSClassExtension.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8737654 [details] [diff] [review]
(part 2) - Move ClassExtension::isWrappedNative into js::Class::flags

>+#define JSCLASS_IS_WRAPPED_NATIVE       (1<<2)  // class is an XPCWrappedNative

This should copy more of the docs from the old location than it currently does.

r=me

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f349b8151b9bc165318bda2eaed0ede209d50e
Bug 1261720 (part 1) - Separate js::ClassExtension from js::Class. r=jorendorff,bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/32598dbf8f233575c5dd492eb05dd324b53d1d6a
Bug 1261720 (part 2) - Move ClassExtension::isWrappedNative into js::Class::flags. r=jorendorff.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9f349b8151b
https://hg.mozilla.org/mozilla-central/rev/32598dbf8f23