Closed Bug 1448454 Opened 7 years ago Closed 7 years ago

Make more fields private in xpt_struct.h

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mccr8, Unassigned)

References

Details

Attachments

(2 files)

Bug 1448454 - Make some fields in xpt_struct.h private.
(deleted), text/x-review-board-request
n.nethercote
: review+
Details
Back out bug 1448454 for compile time and installer size regressions.
(deleted), patch
Details | Diff | Splinter Review
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 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.
Attachment #8964579 - Flags: review?(n.nethercote) → review+
Yeah, I think I checked that, but I should recheck it.
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5eaa657f6e06 Make some fields in xpt_struct.h private. r=njn
Oops. I didn't think about the fact that one mandatory and one optional argument would mean the implicit/explicit stuff was in play...
Flags: needinfo?(continuation)
Err, two optional arguments.
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5f7b0762928 Make some fields in xpt_struct.h private. r=njn
Blocks: 1451425
https://hg.mozilla.org/mozilla-central/rev/e5f7b0762928
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I was looking at perfherder, and it seems like this may have caused a big installer size regression. Very odd.
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...
Are you sure that new code got compiled into straight data, rather than some giant static constructor(s)?
Flags: needinfo?(continuation)
(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.
Flags: needinfo?(continuation)
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.
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.
(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.
Please land the backout patch I just attached. I checked it locally to make sure it builds and runs.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
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.
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.
Yeah, I agree.
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.
That's alright. I agree that it would be better if those fields were private.
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/09c9390d42d6 Back out bug 1448454 for compile time and installer size regressions. r=mccr8
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
Assignee: continuation → nobody
Somebody can try again if MSVC gets better.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: