1448454 - Make more fields private in xpt_struct.h

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

Bug 1438688 changes many fields of the data structures in xpt_struct.h to be accessed only via accessors. These fields should be made private. This will require defining and using constexpr constructors for the classes, instead of aggregate initialization. Once somebody writes a patch, they will need to double check that all of the data structures in the generated code are still rodata.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1448454 - Make some fields in xpt_struct.h private.

Many of these fields have accessors, and can only be read indirectly
by going through the XPTHeader data structure anyways, so they should
be marked private. This makes the generated XPT data file noisier due
to the need for constexpr constructors.

I had to fix the ctors for the classes in xptinfo.h to be less weird
because there was a compiler error.

Members in two of the classes need to be marked protected because they
have subclasses in xptinfo.h. Ideally those classes would be merged
in.

Review commit: https://reviewboard.mozilla.org/r/233294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233294/

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8964579 [details]
Bug 1448454 - Make some fields in xpt_struct.h private.

https://reviewboard.mozilla.org/r/233294/#review239034

r=me if the tables are still rodata.

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, I think I checked that, but I should recheck it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5eaa657f6e06
Make some fields in xpt_struct.h private. r=njn

Comment 5

[Andreea Pavel [:apavel]]

Backed out for build bustages at /builds/worker/workspace/build/src/xpcom/typelib/xpt/xpt_struct.h:16

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5eaa657f6e06a62072c04c9f233f4e4508c15f51

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171754031&repo=autoland&lineNumber=8251

Backout: https://hg.mozilla.org/integration/autoland/rev/7d7ad254494eee551d26aafb9fc44906679480e6

Comment 6

[Andrew McCreight [:mccr8]]

Oops. I didn't think about the fact that one mandatory and one optional argument would mean the implicit/explicit stuff was in play...

Comment 7

[Andrew McCreight [:mccr8]]

Err, two optional arguments.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8964579 [details]
Bug 1448454 - Make some fields in xpt_struct.h private.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233294/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5f7b0762928
Make some fields in xpt_struct.h private. r=njn

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e5f7b0762928

Comment 11

[Andrew McCreight [:mccr8]]

I was looking at perfherder, and it seems like this may have caused a big installer size regression. Very odd.

Comment 12

[Andrew McCreight [:mccr8]]

It may have also regressed compile time on Windows by about 8%, which seems bad. Maybe MSVC just is not very fast at compiling a 26000 line file full of constexpr constructors...

Comment 13

[Nathan Froyd [:froydnj]]

Are you sure that new code got compiled into straight data, rather than some giant static constructor(s)?

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> Are you sure that new code got compiled into straight data, rather than some
> giant static constructor(s)?

I don't know how to check that. I checked that on Linux that the various arrays were all still in .rodata.

Comment 15

[Nathan Froyd [:froydnj]]

One option might be to paste some of the generated code into https://gcc.godbolt.org/ and see what it compiles to.  The other option is to actually look at an MSVC build tree somehow and see what the relevant .obj looks like.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

this change looks to have caused a build time regression:
== Change summary for alert #12502 (as of Wed, 04 Apr 2018 20:49:08 GMT) ==

Regressions:

  8%  build times windows2012-32 pgo taskcluster-c4.4xlarge     4,538.91 -> 4,917.75
  5%  build times windows2012-64 pgo taskcluster-c4.4xlarge     4,530.61 -> 4,766.80

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12502

I am not 100% sure if this is the cause, but looking at the related graphs seems to highlight the patch from here.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #16)
> I am not 100% sure if this is the cause, but looking at the related graphs
> seems to highlight the patch from here.

Yeah, the version of MSVC we're using seems to not deal with constexprs very well, so it is plausible. Between this and (more importantly) the installer size regression I see I think this should get backed out. I don't think there's a lot of danger that people are going to start poking around in these data structures, as it is very obscure. Maybe Nika's approach in bug 1444745 will work better.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: Back out bug 1448454 for compile time and installer size regressions.

Comment 19

[Andrew McCreight [:mccr8]]

Please land the backout patch I just attached. I checked it locally to make sure it builds and runs.

Comment 20

[Andrew McCreight [:mccr8]]

The installer size regression looks like this:
  59,119,719.00 (lower is better)
  Δ 180,407.00 (0.3%)
175kb isn't the end of the world I guess, if there was a stronger reason for taking it.

Comment 21

[Nathan Froyd [:froydnj]]

175K of *installer* size probably translates to something like 250-300K of libxul size (perfherder will tell you if you click the "include subtests" checkbox and search for "xul" to find the appropriate timeseries)...which is quite large.  Unless there was a *really* good reason for needing the feature, a cleanup patch that regresses installer size by 175K is not acceptable.

Comment 22

[Andrew McCreight [:mccr8]]

Yeah, I agree.

Comment 23

[Nicholas Nethercote [inactive]]

I apologize for suggesting this change in the first place. It's a good example of what looks like a zero-cost abstraction but actually isn't.

Comment 24

[Andrew McCreight [:mccr8]]

That's alright. I agree that it would be better if those fields were private.

Comment 25

[Andreea Pavel [:apavel]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/09c9390d42d6 
Back out bug 1448454 for compile time and installer size regressions. r=mccr8

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

backout shows build time improvements:
== Change summary for alert #12618 (as of Tue, 10 Apr 2018 05:08:57 GMT) ==

Improvements:

  5%  build times windows2012-64 pgo taskcluster-c4.4xlarge     4,796.76 -> 4,567.30

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12618

Comment 27

[Andrew McCreight [:mccr8]]

Somebody can try again if MSVC gets better.