Closed Bug 1339275 Opened 7 years ago Closed 7 years ago

Improve codegen for const JSPropertySpec arrays

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: away, Assigned: away)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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).
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)
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.
Attached patch Elided braces (obsolete) — Splinter Review
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)
-                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.
Argh, the clang platforms complain of "error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]".
Attached patch Elided braces (obsolete) — Splinter Review
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)
Attached patch Elided bracesSplinter Review
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+
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
https://hg.mozilla.org/mozilla-central/rev/125a0e765379
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.