1214364 - Consider flagging IDL dictionaries that are being used solely as convenience to create JS objects

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

There are two things we may want to do:

1)  An extended attribute that says to not sort the dictionary members.  That would allow us to avoid issues like the one I mention in bug 1085293 comment 33, where per spec the "value" value needs to be defined before the "done" value.

2)  An extended attribute that says to not generate the Init method or to generate a simpler one that doesn't take a JS::Value or something, and maybe to skip InitFromJSON, ToJSON, etc.

Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

And maybe we can trim off other things too.  We generate a _lot_ of code for an IDL dictionary...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1214364 part 1. Only output conversions to/from JSON for dictionaries that need it. r=peterv

This saves about 200KB of codesize on Linux.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1214364 part 3. Only output ToObjectInternal methods for dictionaries that need it. r=peterv

I'm not too happy with the isUsed tracking here: it marks things used from
unused dictionaries, for example, and even so we need a lot of WantToObject
annotations. Maybe we shouldn't worry about this....

This saves about 55KB of codesize on Linux.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1214364 part 4. Only output full-featured Init methods for dictionaries that need them. r=peterv

Dictionaries that we never initialize with JS values don't need them.

Again, the isUsed tracking is not great: really we care about whether the
dictionaries participate in JS-to-IDL conversions. So maybe we can do slightly
better in terms of the analysis we do...

This saves about 100KB of codesize on Linux.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The bloaty output with all three patches applied looks like this, in an opt build:

     VM SIZE                         FILE SIZE
 --------------                   --------------
  -0.1% -45.8Ki LOAD [R]          -45.8Ki  -0.1%
      -0.0% -4.25Ki .rodata           -4.25Ki  -0.0%
      -0.1% -5.84Ki .eh_frame_hdr     -5.84Ki  -0.1%
      -0.2% -35.7Ki .eh_frame         -35.7Ki  -0.2%
  -0.4%  -350Ki LOAD [RX]          -350Ki  -0.4%
      -0.4%  -350Ki .text              -350Ki  -0.4%
  [ = ]       0 [Unmapped]        -2.42Mi  -0.1%
      [ = ]       0 [Unmapped]           +444   +11%
      [ = ]       0 .debug_abbrev        -546  -0.0%
      [ = ]       0 .symtab           -17.5Ki  -0.1%
      [ = ]       0 .strtab           -49.8Ki  -0.1%
      [ = ]       0 .debug_ranges     -53.5Ki  -0.1%
      [ = ]       0 .debug_str        -60.4Ki  -0.0%
      [ = ]       0 .debug_line        -243Ki  -0.3%
      [ = ]       0 .debug_info        -961Ki  -0.1%
      [ = ]       0 .debug_loc        -1.07Mi  -0.3%
  -0.2%  -396Ki TOTAL             -2.81Mi  -0.2%

So we're saving some code and function headers, but that's about it. And I'm really not sure why the linker is not removing this dead code to start with... :(

I'm not sure whether it's worth making these changes, but figured I'd attach them. Thoughts?

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

I'm not too happy with the isUsed tracking here: it marks things used from
unused dictionaries, for example, and even so we need a lot of WantToObject
annotations. Maybe we shouldn't worry about this....

Do you think that having correct isUsed tracking will catch a lot more cases?

In general these all seem nice to have IMO (though I'd do s/want/generate/ to make it clear that it turns on code generation).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Do you think that having correct isUsed tracking will catch a lot more cases?

I don't really know. I could test by removing the isUsed thing and then adding the relevant annotations to all the dictionaries that need them and seeing how things look. I'll try to give that a shot.

though I'd do s/want/generate/

Makes sense.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'll try to give that a shot.

It looks like removing the isUsed thing and then adding GenerateToObject everywhere that's needed to compile wins another ~200KB of codesize (on top of the 55KB we were already saving by skipping ToObjectInternal generation).

The developer experience is not great, of course, but it does sound like it would be worthwhile. I'll do a bit of poking around to see whether I can add better tracking of when ToObject should be implied automatically.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So I added better automated tracking of GenerateToObject, but I ran into an interesting issue.

Say we have a dictionary Dict. It is not flagged GenerateToObject and is not getting a ToObjectInternal method.

Now in a .webidl file different from the one Dict is declared in, I add an operation to an interface that returns Dict. This means Dict will now need a ToObjectInternal method. However, nothing indicates that codegen for Dict depends on this interface (and indeed until now it did not), so in a dep build after this change we will not rerun codegen for the .webidl file Dict is in, and won't output the now-neeed ToObjectInternal method.

I'm not sure how to address this yet, but thinking about it. Any bright ideas?

Comment 10

[Boris Zbarsky [:bzbarsky]]

The patch-in-progress in bug 1103153 might help with that problem; then I could dynamically compute the extra-changed-files by flagging all dictionaries whose GenerateToObject state changed and adding them to the list...

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, I now have measurements for Linux64 pgo (and hence LTO) builds from try. The wins there are much smaller than locally.

Locally, with the updated patches, I see numbers like so:

• Dropping unused JSON bits: wins 203KB
• Dropping unused ToObjectInternal bits: wins 272KB
• Dropping unused Init bits: wins 310KB

for a total of about 785KB of .text savings, plus some .eh_frame and .rodata and .eh_frame_hdr savings; 863KB total.

With the PGO build the numbers are:

• Dropping unused JSON bits: No effect (LTO must be dropping these already)
• Dropping unused ToObjectInternal bits: wins 19KB (again, LTO is dropping most of those)
• Dropping unused Init bits: wins 284KB (LTO can't drop these because the constructor calls them)

for a total of about 305KB of .text savings, plus the other bits, 323KB total. We do still need the JSON changes to get the other wins, because the JSON bits affect whether we need ToObjectInternal and Init.

Still worth it?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bloaty output for the overall change on an LTO Linux64 build:

     VM SIZE                         FILE SIZE
 --------------                   --------------
  -0.0%    -192 LOAD [RW]            -192  -0.0%
      -0.0%    -192 .data.rel.ro         -192  -0.0%
  -0.1% -18.1Ki LOAD [R]          -18.1Ki  -0.1%
      -0.1% -1.47Ki .eh_frame_hdr     -1.47Ki  -0.1%
      -0.1% -8.29Ki .eh_frame         -8.29Ki  -0.1%
      -0.0% -8.38Ki .rodata           -8.38Ki  -0.0%
  [ = ]       0 [Unmapped]        -33.1Ki  -0.1%
      [ = ]       0 [Unmapped]        -4.65Ki -71.8%
      [ = ]       0 .symtab           -5.13Ki  -0.1%
      [ = ]       0 .strtab           -23.3Ki  -0.1%
  -0.3%  -305Ki LOAD [RX]          -305Ki  -0.3%
      -0.3%  -305Ki .text              -305Ki  -0.3%
  -0.2%  -323Ki TOTAL              -356Ki  -0.2%

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1214364 part 2. Remove the dead code around MethodNotNewObjectError. r=peterv

I don't know when we stopped raising them, but we did at some point.

I am leaving the capability to not generate a union's ToJSVal method, because I will need it soon.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Bloaty output for the overall change on an LTO Linux64 build:

     VM SIZE                         FILE SIZE
 --------------                   --------------
  -0.0%    -192 LOAD [RW]            -192  -0.0%
      -0.0%    -192 .data.rel.ro         -192  -0.0%
  -0.1% -18.1Ki LOAD [R]          -18.1Ki  -0.1%
      -0.1% -1.47Ki .eh_frame_hdr     -1.47Ki  -0.1%
      -0.1% -8.29Ki .eh_frame         -8.29Ki  -0.1%
      -0.0% -8.38Ki .rodata           -8.38Ki  -0.0%
  [ = ]       0 [Unmapped]        -33.1Ki  -0.1%
      [ = ]       0 [Unmapped]        -4.65Ki -71.8%
      [ = ]       0 .symtab           -5.13Ki  -0.1%
      [ = ]       0 .strtab           -23.3Ki  -0.1%
  -0.3%  -305Ki LOAD [RX]          -305Ki  -0.3%
      -0.3%  -305Ki .text              -305Ki  -0.3%
  -0.2%  -323Ki TOTAL              -356Ki  -0.2%

LTO cuts down the wins quite a bit (a non-LTO build saves closer to 800K), but there's still a win.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22ca446987dc
part 1.  Only output conversions to/from JSON for dictionaries that need it.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/cbc214ea08f3
part 2.  Remove the dead code around MethodNotNewObjectError.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/f44823eeed6e
part 3.  Only output ToObjectInternal methods for dictionaries that need it.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/4938d59bdfac
part 4.  Only output full-featured Init methods for dictionaries that need them.  r=peterv

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/22ca446987dc
https://hg.mozilla.org/mozilla-central/rev/cbc214ea08f3
https://hg.mozilla.org/mozilla-central/rev/f44823eeed6e
https://hg.mozilla.org/mozilla-central/rev/4938d59bdfac