Closed Bug 1557762 Opened 1 year ago Closed 1 year ago

Single-machine PGO builds don't get NS_FREE_PERMANENT_DATA

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: dmajor, Assigned: Gijs)

References

Details

Attachments

(1 file)

I haven't confirmed in the output, but by inspection, 4b78f4fb473c doesn't do the right thing on single-machine PGO builds. In those builds, MOZ_PROFILE_GENERATE is set by the profiledbuild:: target rather than --enable-profile-... switches.

An additional trickiness is that single-machine builds only set the C++ define in some isolated directories which may not play nice with the fact that nscore.h is included from a bunch of different places.

Is the MOZ_PROFILE_GENERATE config flag going to be already set in that toolchain.configure file (as a result of the profiledbuild target), ie can we also make that trigger the define in the same toolchain.configure file? Or does that not work?

Flags: needinfo?(dmajor)
Blocks: 1196094

I'm afraid I don't speak enough build to be able to answer that.

Flags: needinfo?(dmajor)

FWIW I am close to getting Windows switched over to 3-tier PGO (perhaps before the all-hands?), so depending on your timeline you may be able to wait for that. I just filed bug 1557785 if you want to follow along. The profiledbuild version just sets a make variable, and unfortunately doesn't go through configure.

(In reply to Michael Shal [:mshal] from comment #3)

FWIW I am close to getting Windows switched over to 3-tier PGO (perhaps before the all-hands?), so depending on your timeline you may be able to wait for that. I just filed bug 1557785 if you want to follow along. The profiledbuild version just sets a make variable, and unfortunately doesn't go through configure.

Right... configure and mozbuild are basically unaware of MOZ_PROFILE_GENERATE in single-machine pgo builds. If we need to set it that probably needs to happen around here: https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/config/config.mk#135

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Michael Shal [:mshal] from comment #3)

FWIW I am close to getting Windows switched over to 3-tier PGO (perhaps before the all-hands?), so depending on your timeline you may be able to wait for that. I just filed bug 1557785 if you want to follow along. The profiledbuild version just sets a make variable, and unfortunately doesn't go through configure.

I think our only real timeline is "before 69 merges to beta". There might have been more schedule pressure if there was a perf regression, but I think so far nothing's come up... Does that sound safe?

(In reply to Chris Manchester (:chmanchester) from comment #4)

Right... configure and mozbuild are basically unaware of MOZ_PROFILE_GENERATE in single-machine pgo builds. If we need to set it that probably needs to happen around here: https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/config/config.mk#135

FWIW, I tried to look at this just to make sure we have a failsafe if for some reason we can't switch to the 3-tier thing, but it's not obvious to me how we'd do this, presumably because I've forgotten any make-speak I may have once known. Would we have to string-modify the list of defines in here somehow, or is there an easier way? Because that feels pretty yucky...

Flags: needinfo?(mshal)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(cmanchester)

Something like PGO_CFLAGS += -DNS_FREE_PERMANENT_DATA=1 in that block I linked should do it (yes it's yucky).

Flags: needinfo?(cmanchester)

(In reply to Chris Manchester (:chmanchester) from comment #6)

Something like PGO_CFLAGS += -DNS_FREE_PERMANENT_DATA=1 in that block I linked should do it (yes it's yucky).

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1559878#c3 I think this works, so I'll just attach a patch for this...

(In reply to :Gijs (he/him) from comment #5)

(In reply to Michael Shal [:mshal] from comment #3)

FWIW I am close to getting Windows switched over to 3-tier PGO (perhaps before the all-hands?), so depending on your timeline you may be able to wait for that. I just filed bug 1557785 if you want to follow along. The profiledbuild version just sets a make variable, and unfortunately doesn't go through configure.

I think our only real timeline is "before 69 merges to beta". There might have been more schedule pressure if there was a perf regression, but I think so far nothing's come up... Does that sound safe?

Sorry for the delay, I was hoping to report that the 3-tier PGO would've landed sooner (should be this week, I think?)

In any case, I don't think there are any blockers that will prevent it from going forward. Your patch should work fine in the meantime though, and we can always pull it back out as part of the final PGO cleanup for simplicity (bug 1557788).

Flags: needinfo?(mshal)

Gijs, can you land your patch? It looks like it's blocking enabling the socket process in bug 1558205.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Eric Rahm [:erahm] from comment #10)

Gijs, can you land your patch? It looks like it's blocking enabling the socket process in bug 1558205.

Sorry, I thought I had, looks like bad wifi interfered with me using lando or something. Tree's closed right now but I've submitted it.

Flags: needinfo?(gijskruitbosch+bugs)

(Though, bug 1557785 is on central now, so I think we might be OK? Hard to tell from bug 1558205.)

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/455dff329fcc
ensure we define NS_FREE_PERMANENT_DATA for single-stage pgo builds, r=chmanchester

(In reply to :Gijs (he/him) from comment #12)

(Though, bug 1557785 is on central now, so I think we might be OK? Hard to tell from bug 1558205.)

I'm landing the last of the patches to move all central builds to 3-tier PGO now, but I think your patch is still good to land in case I've missed anything. (In particular, I'm not certain all beta / release builds are using 3-tier builds yet).

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4cb7eb5b8a42
ensure we define NS_FREE_PERMANENT_DATA for single-stage pgo builds, r=chmanchester
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

== Change summary for alert #21700 (as of Tue, 02 Jul 2019 07:55:00 GMT) ==

Improvements:

4% raptor-speedometer-firefox windows7-32-shippable opt 77.58 -> 80.83
4% raptor-speedometer-firefox windows7-32-shippable opt 77.60 -> 80.69

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

Blocks: 1559878
You need to log in before you can comment on or make changes to this bug.