1259194 - Reduce static data sizes by separating js::ObjectOps from js::Class

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

js::Class is big -- 208 bytes on 64-bit platforms. And it is sparse -- in many cases a lot of the fields are null, and if not null, the same across many instances. And we have lots instances -- more than 2400 in Firefox, the majority from DOM bindings. That adds up to about 0.5 MiB of static data which is replicated in every process.

One fairly easy thing to do is to replace the js::ObjectOps sub-struct with a js::ObjectOps*, which reduces sizeof(js::Class) by 11 words. In all the cases where we use JS_NULL_OBJECT_OPS, this can just become nullptr. And then in lots of other cases we can share one js::ObjectOps between multiple js::Class instances.

On my Linux64 machine this reduces static data size (a cost incurred for each process) by 208 KiB. The downsides are (a) accesses of non-null ObjectOps involve an indirection, and (b) it's an API breakage.

There is potential for doing similar changes for the other sections of js::Class, but this seems like a good place to start.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add 's' prefixes to some statics generated for dom bindings

This makes Class, PrototypeClass and InterfaceObjectClass match existing cases
like sAttributes{,_ids,specs}, sNativeProperties, sNativePropertyHooks.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove XPCWrappedNativeJSClass

It's a trivial and pointless wrapper for js::Class.

This patch also fixes some ridiculous formatting in XPCNativeScriptableShared.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8734055 [details] [diff] [review]
(part 1) - Add 's' prefixes to some statics generated for dom bindings

r=me

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove PopulateJSClass()

PopulateJSClass() is only called immediately after the constructor, so it can
be merged into the constructor. (This helps with the next patch.)

The patch also removes an unnecessary |new| check.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Separate js::ObjectOps from js::Class

bz: please review the dom/ changes.

efaust: please review the js/src/ and js/public/ changes.

mrbkap: please review the js/xpconnect/ changes.

Thank you.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8734246 [details] [diff] [review]
(part 4) - Separate js::ObjectOps from js::Class

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

js/ changes lgtm, with comments below addressed.

I could maybe argue that I prefer get$(OPNAME)Op() to getOps$(OPNAME)(), but it's too weak to make you go change all of those spots.

::: js/public/Class.h
@@ +630,5 @@
>  struct ObjectOps
>  {
> +    const LookupPropertyOp lookupProperty;
> +    const DefinePropertyOp defineProperty;
> +    const HasPropertyOp    hasProperty;

why are these marked const? I'm all for doing it, but it seems like the accessors below ignore that. I'm not sure what mutation this protects against. Did I miss something?

::: js/src/jsfriendapi.h
@@ +327,5 @@
>          js::proxy_WeakmapKeyDelegate,                                   \
>          objectMoved                                                     \
>      }
>  
> +#define PROXY_OBJECT_OPS_DEF \

I don't think we need this. We can just inline it into Proxy.cpp. The reason we have a bunch of PROXY_* macros is so that people can roll their own Class*s in the embedding, with the proper constants. In this case, that's &js::ProxyObjectOps, which you've already provided them.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8734246 [details] [diff] [review]
(part 4) - Separate js::ObjectOps from js::Class

r=me on the dom/ and xpconnect/ bits.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a875129c6f35701ecfa2866fbcd319d2fb7d726
Bug 1259194 (part 1) - Add 's' prefixes to some statics generated for dom bindings. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1af522c298bfc1cc383bcf5f6b9bae25aff7bef
Bug 1259194 (part 2) - Remove XPCWrappedNativeJSClass. r=mrbkap.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ec435d22564e64fdd03a71604ebf003b40a48a
Bug 1259194 (part 3) - Remove PopulateJSClass(). r=mrbkap.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a875129c6f3
https://hg.mozilla.org/mozilla-central/rev/d1af522c298b
https://hg.mozilla.org/mozilla-central/rev/a8ec435d2256

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43d8b2d94db52eb484bfda5e0ee00819b48a4ef
Bug 1259194 (part 4) - Separate js::ObjectOps from js::Class. r=efaust,mrbkap,bz.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a43d8b2d94db52eb484bfda5e0ee00819b48a4ef
> Bug 1259194 (part 4) - Separate js::ObjectOps from js::Class. r=efaust,mrbkap,bz.

This reduced the size of libxul.so on Android by only KiB, according to PerfHerder:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=%5Bmozilla-inbound,7517209843c92900e48c1a8f72252b633110a8f1,1%5D&selected=%5Bmozilla-inbound,7517209843c92900e48c1a8f72252b633110a8f1,29297,24817668%5D

I expected something more like 100 KiB, i.e. half of the 208 KiB reduction I saw on libxul.so size on my Linux64 machine. Hmm. Does Fennec have all the DOM bindings code in it?

On Linux64 the change was clearer, though the reduction looks more like ~160 KiB rather than 208 KiB:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=172800&series=%5Bmozilla-inbound,3676ab14b4eed79031e4bf1049ac19e9ce1398c9,1%5D&selected=%5Bmozilla-inbound,3676ab14b4eed79031e4bf1049ac19e9ce1398c9,29297,24817650%5D

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Does Fennec have all the DOM bindings code in it?
It should have almost all of the same stuff, yes.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a43d8b2d94db