Closed
Bug 1259194
Opened 8 years ago
Closed 8 years ago
Reduce static data sizes by separating js::ObjectOps from js::Class
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(4 files)
10.72 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
12.04 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
4.09 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
70.45 KB,
patch
|
bzbarsky
:
review+
efaust
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
This makes Class, PrototypeClass and InterfaceObjectClass match existing cases like sAttributes{,_ids,specs}, sNativeProperties, sNativePropertyHooks.
Attachment #8734055 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
It's a trivial and pointless wrapper for js::Class. This patch also fixes some ridiculous formatting in XPCNativeScriptableShared.
Attachment #8734059 -
Flags: review?(mrbkap)
Comment 3•8 years ago
|
||
Comment on attachment 8734055 [details] [diff] [review] (part 1) - Add 's' prefixes to some statics generated for dom bindings r=me
Attachment #8734055 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Attachment #8734059 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 4•8 years ago
|
||
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.
Attachment #8734154 -
Flags: review?(mrbkap)
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 5•8 years ago
|
||
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.
Attachment #8734246 -
Flags: review?(mrbkap)
Attachment #8734246 -
Flags: review?(efaustbmo)
Attachment #8734246 -
Flags: review?(bzbarsky)
Comment 6•8 years ago
|
||
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.
Attachment #8734246 -
Flags: review?(efaustbmo) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8734246 [details] [diff] [review] (part 4) - Separate js::ObjectOps from js::Class r=me on the dom/ and xpconnect/ bits.
Attachment #8734246 -
Flags: review?(mrbkap)
Attachment #8734246 -
Flags: review?(bzbarsky)
Attachment #8734246 -
Flags: review+
Updated•8 years ago
|
Attachment #8734154 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2a875129c6f3 https://hg.mozilla.org/mozilla-central/rev/d1af522c298b https://hg.mozilla.org/mozilla-central/rev/a8ec435d2256
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a43d8b2d94db52eb484bfda5e0ee00819b48a4ef Bug 1259194 (part 4) - Separate js::ObjectOps from js::Class. r=efaust,mrbkap,bz.
Assignee | ||
Comment 11•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a43d8b2d94db
You need to log in
before you can comment on or make changes to this bug.
Description
•