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

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

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.
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.
Attachment #8737653 - Flags: review?(jorendorff)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8737654 - Flags: review?(jorendorff)
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.
Blocks: 1261723
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.
Attachment #8737653 - Flags: review?(bzbarsky)
Attachment #8737654 - Flags: review?(bzbarsky)
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.
Attachment #8737653 - Flags: review?(jorendorff) → review+
Attachment #8737654 - Flags: review?(jorendorff) → review+
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
Attachment #8737653 - Flags: review?(bzbarsky) → review+
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 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
Attachment #8737654 - Flags: review?(bzbarsky) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/b9f349b8151b
https://hg.mozilla.org/mozilla-central/rev/32598dbf8f23
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.