Improve codegen for const JSPropertySpec arrays

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 months ago
Currently the JSPropertySpecs in the DOM bindings look like:
 { "backgroundColor", JSPROP_SHARED | JSPROP_ENUMERATE, { { { { GenericBindingGetter, &backgroundColor_getterinfo } }, { { GenericBindingSetter, &backgroundColor_setterinfo } } } } },

With all those nested structs and unions, Visual Studio has trouble recognizing that this is const data. The 256k of bindings tables end up going into the writable .data section, and in one case, VS even generates a 64k constructor for CSS2PropertiesBinding::sAttributes_specs (much like bug 1334254 comment 0).

I was able to convince VS do the right thing by defining a struct in very simple terms:

struct JSPropertySpec__StaticInit {
    const char*      name;
    uint8_t          flags;
    JSNative         getter_op;
    const JSJitInfo* getter_info;
    JSNative         setter_op;
    const JSJitInfo* setter_info;
};

and then casting that to JSPropertySpec* (with assertions to verify the layout).
(Assignee)

Comment 1

10 months ago
Created attachment 8836961 [details] [diff] [review]
Define JSPropertySpec in a way that's easier for Visual Studio to initialize

Flagging the same reviewers as the JSPropertySpec changes in bug 958262.

This moves 256k of writable data to the read-only section of xul.dll. It also trims 64k from the binary by removing a constructor.

I'm not super proud of PrefablePropertySpecFromStatic. Ideally I'd cast away the __StaticInit at an earlier point, so that the contamination doesn't spread to Prefable<const JSPropertySpec__StaticInit*>. But this required the least contortion of codegen.py.
Assignee: nobody → dmajor
Attachment #8836961 - Flags: review?(jwalden+bmo)
Attachment #8836961 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

10 months ago
Ooh, ooh, I can do this without the ugly second struct! It turns out, all VS really needs is to use the elided-braces form of initialization.
(Assignee)

Comment 3

10 months ago
Created attachment 8837051 [details] [diff] [review]
Elided braces

This gets the same 256k+64k savings as before, and we still get to call it "JSPropertySpec".
Attachment #8836961 - Attachment is obsolete: true
Attachment #8836961 - Flags: review?(jwalden+bmo)
Attachment #8836961 - Flags: review?(bzbarsky)
Attachment #8837051 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

10 months ago
-                return "JSNATIVE_WRAPPER(nullptr)"
+                return "nullptr"

Oops, that should be 'return "nullptr, nullptr"'. I guess it happens to work by accident, but I'll fix it next time I touch this ptch.
(Assignee)

Comment 5

10 months ago
Argh, the clang platforms complain of "error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]".
(Assignee)

Comment 6

10 months ago
Created attachment 8837380 [details] [diff] [review]
Elided braces

Well, this builds on clang, but it's a bit yucky. :(
Attachment #8837051 - Attachment is obsolete: true
Attachment #8837051 - Flags: review?(bzbarsky)
Attachment #8837380 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

10 months ago
Created attachment 8837381 [details] [diff] [review]
Elided braces

It helps if I attach the right patch...
Attachment #8837380 - Attachment is obsolete: true
Attachment #8837380 - Flags: review?(bzbarsky)
Attachment #8837381 - Flags: review?(bzbarsky)
Comment on attachment 8837381 [details] [diff] [review]
Elided braces

The codegen parts look ok to me.

Whether this is an ok thing to do with a JSPropertySpec in general, and in particular whether this is likely to go horribly wrong if someone _modifies_ JSPropertySpec, I'm not sure.  Going to hand over review to waldo....
Attachment #8837381 - Flags: review?(bzbarsky) → review?(jwalden+bmo)
Comment on attachment 8837381 [details] [diff] [review]
Elided braces

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

Huh, I didn't know brace-omission was spec-valid.

I'm not all that gung-ho on removing braces for this stuff, for the changes-to-JSPropertySpec-break-this reason.  But if MSVC gonna MSVC, whaddaya gonna do.

I wish I could think of a way to statically test exact behavior of this, somehow, but nothing comes to mind.  However, it seems pretty likely failures would manifest super-fast as test failures, so this concern probably can't supersede making behavior with MSVC not sucktastic.
Attachment #8837381 - Flags: review?(jwalden+bmo) → review+

Comment 10

10 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/125a0e765379
Define JSPropertySpec values in a way that's easier for Visual Studio to initialize. r=Waldo

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/125a0e765379
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.