Closed
Bug 1260653
Opened 8 years ago
Closed 8 years ago
Shrink NativeProperties
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file)
32.66 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
NativeDescriptors contains seven pointer trios. Typically 4 or 5 or 6 of the trios are all null. We can represent things more compactly.
Assignee | ||
Comment 1•8 years ago
|
||
This patch implements a variable-length NativeProperties. It reduces static data by 110,336 bytes. The implementation ended up a bit uglier than I had hoped for; see what you think. Look at the changes in DOMJSClass.h first, especially the big comment. Everything else follows from there. Here's an example. Old generated code: > static const NativeProperties sNativeProperties = { > nullptr, nullptr, nullptr, > nullptr, nullptr, nullptr, > sMethods, sMethods_ids, sMethods_specs, > sAttributes, sAttributes_ids, sAttributes_specs, > nullptr, nullptr, nullptr, > nullptr, nullptr, nullptr, > sConstants, sConstants_ids, sConstants_specs, > -1 > }; New generated code: > static const NativePropertiesN<3> sNativeProperties = { > -1, > false, 0, > false, 0, > true, 0 /* sMethods */, > true, 1 /* sAttributes */, > false, 0, > false, 0, > true, 2 /* sConstants */, > { > { sMethods, sMethods_ids, sMethods_specs }, > { sAttributes, sAttributes_ids, sAttributes_specs }, > { sConstants, sConstants_ids, sConstants_specs } > } > };
Assignee | ||
Updated•8 years ago
|
Attachment #8736181 -
Flags: review?(bzbarsky)
Comment 2•8 years ago
|
||
Comment on attachment 8736181 [details] [diff] [review] Shrink NativeProperties >+++ b/dom/bindings/Codegen.py > class CGNativeProperties(CGList): >+ nativeProps1 = [] How about calling this nativePropsIntBits? >+ nativeProps2 = [] And this one nativePropsPointerBits? >+ props = ("true, %d /* %s */" % (offset, varName)) This doesn't really need the outermost params. >+ props = "{ %(name)s, " + ids + ", %(name)s_specs }" I'm not so happy about reusing "props" here. Maybe call this "pointers"? >+++ b/dom/bindings/DOMJSClass.h >+ MOZ_ASSERT(mHas##FieldName##s); \ Please MOZ_ASSERT(Has##FieldName##s()); instead. >+// Ensure the struct has the expected size. The 8 is for the >+// iteratorAliasMethodIndex plus the bitfields; If we cared, I think we could cut this down to 4 instead of 8 on 32-bit builds. Specifically, since we know that we have at most 7 Trios, we know our offset is always in the range 0-6. Thus we could use an offset value of 7 to indicate "not present", which would mean only 3 bits needed per trio type, leaving 11 bits to represent values of iteratorAliasMethodIndex (where we'd also use 4095 to represent "not present", so it can be unsigned). That would fit all the non-pointer state into 32 bits. Not sure how much we care. r=me
Attachment #8736181 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 3•8 years ago
|
||
> If we cared, I think we could cut this down to 4 instead of 8 on 32-bit
> builds.
>
> Specifically, since we know that we have at most 7 Trios, we know our offset
> is always in the range 0-6. Thus we could use an offset value of 7 to
> indicate "not present", which would mean only 3 bits needed per trio type,
> leaving 11 bits to represent values of iteratorAliasMethodIndex (where we'd
> also use 4095 to represent "not present", so it can be unsigned). That
> would fit all the non-pointer state into 32 bits. Not sure how much we care.
That would be some truly impressive packing :) But it would save 32 bits per instance on 32-bit, which would only be about 3 KiB. I figure this type is already ugly enough, so I won't do this change. Thank you for the suggestion.
Assignee | ||
Comment 4•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae0a75858a8169197d9fc40b94504d7e45bfc72 Bug 1260653 - Shrink NativeProperties. r=bz.
Comment 5•8 years ago
|
||
Looking at bug 1186064 comment 8, it seems like we shouldn't be using standard constexpr yet. Nicholas, can you push a follow-up to switch to MOZ_CONSTEXPR? Thanks!
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/456df967efef1a9c621c9ededd6bf5530baedd7f Bug 1260653 (follow-up) - Use |MOZ_CONSTEXPR| instead of |constexpr| to unbreak builds with VS 2013. r=birtles.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(n.nethercote)
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5ae0a75858a8 https://hg.mozilla.org/mozilla-central/rev/456df967efef
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•