603574 - [Windows VC++] --disable-profile-guided-optimization is ignored

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

Bug 603485 comment 3:
{
Nick Thomas [:nthomas]      2010-10-11 19:43:09 PDT

The existence of '-LTCG:PGINSTRUMENT' in calls to link for the logs in comment
#2 implies --disable-profile-guided-optimization isn't working.
}

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why do we even have this flag?  Can't the difference in make targets handle this?

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Fix some unrelated nits

That puzzled me for a while! Until, by chance, I noticed the cause :->

Comment 3

[(not currently active) Ted Mielczarek]

(In reply to comment #1)
> Why do we even have this flag?  Can't the difference in make targets handle
> this?

I initially wrote it because we were planning on PGOing Mac builds, and I didn't want to waste the CPU cycles on the PPC half. I don't know that we'll ever get around to that, and it's kind of confusing, so maybe we should just remove it instead.  Serge, would you like to instead just write a removal patch?

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Serge, would you like to instead just write a removal patch?

Why not.
But what would be the plan for bug 603485 (then)?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, reading the other bug, I think a PGO configure option is useful as long as try forces PGO.  I think we should take the fix.

Comment 6

[(not currently active) Ted Mielczarek]

I dunno. It's kind of confusing, since people can be misled into thinking that --enable-profile-guided-optimization does something, when it doesn't. I would rather have the try chooser support an option to get a non-PGO build.

But anyway, if we're going to have the option it should at least do what it claims.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So make it error when --enable is selected.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 482501 [details] [diff] [review]
(Av1) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Fix some unrelated nits

Succeeded (to work around bug 603485) as
http://hg.mozilla.org/try/pushloghtml?changeset=e629f100d679
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e629f100d679/tryserver-win32/
and runs fine locally (with XPCOM_MEM_LEAK_LOG=1).

*****

(In reply to comment #6)

> --enable-profile-guided-optimization does something

As for other similar options I guess, --enable-* could (only) be used to override a confvars.sh setting or a .mozconfig default...

> try chooser support an option to get a non-PGO build

That should be discussed in bug 588099, I think.

(In reply to comment #7)
> So make it error when --enable is selected.

(A "useless" param certainly doesn't look like an error to me.)
Maybe just a warning in case the option isn't overriding anything?
(Though I would just not bother, fttb.)

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 482501 [details] [diff] [review]
(Av1) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Fix some unrelated nits

In this particular case, people could easily mistake --enable-profile-guided-optimization as "this is how you do a PGO build" and be confused about the result. Warnings are easy to miss in the output of configure, an error is very clear. Can you add that to your patch?

r=me with that change.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Error out on --enable-*, Fix some unrelated nits

Av1, with comment 9 suggestion(s),
plus keeping /js/src/ in sync'.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 483377 [details] [diff] [review]
(Av1a) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Error out on --enable-*, Fix some unrelated nits

How about in the error message, you link to https://developer.mozilla.org/En/Building_with_Profile-Guided_Optimization instead?

r=me

Comment 12

[(not currently active) Ted Mielczarek]

This can land in the build-system branch.

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Error out on --enable-*, Fix some unrelated nits

Av1a, with comment 11 suggestion(s),
unbitrotted,
and with a redundant comment removal.

Comment 14

[Mike Hommey [:glandium]]

Is that still going to be needed once bug 645166 is done ? (which catlee agreed to do at the same time as the gcc 4.5 switch for linux)

Comment 15

[(not currently active) Ted Mielczarek]

I added this originally while working on PGO for OS X, so I could disable PGO on the PPC half of our build. We should probably just remove this, since we're not doing OS X PGO anytime soon, and it's more confusing than useful.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 526590 [details] [diff] [review]
(Av1b) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Error out on --enable-*, Fix some unrelated nits

We should remove --disable-profile-guided-optimization. It wasn't a great idea when I added it, and glandium's work is better suited to the task.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits

Av1b, with comment 16 suggestion(s).

Ftr,
http://mxr.mozilla.org/build/search?string=able-profile-guided-optimization&case=on
"No results found" :-)

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 530948 [details] [diff] [review]
(Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits

Review of attachment 530948 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for reworking the patch! r=me

Comment 19

[Dão Gottwald [:dao]]

Serge, please provide a commit message following the "Bug XXX - Message. r=foo" pattern.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits [Checked in: Comment 21]

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5a70c6b33303

Comment 22

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) Remove --disable-profile-guided-optimization support, Fix some unrelated nits [Moved to bug 707517]

"comm-5.0" stub is not expected to be hit, but just in case.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 532646 [details] [diff] [review]
(Bv1-CC) Remove --disable-profile-guided-optimization support, Fix some unrelated nits
[Moved to bug 707517]

I think, as I've said before, I'd prefer these on new bugs specifically for comm-central as it is far easier to track what has gone into a release. I also don't know where comm-5.0 came from as we never created that.

Anyway, I'll let Callek review this.

Comment 24

[Justin Wood (:Callek)]

Comment on attachment 532646 [details] [diff] [review]
(Bv1-CC) Remove --disable-profile-guided-optimization support, Fix some unrelated nits
[Moved to bug 707517]

took me a while to get to this, and looks like it has a 5.0 based ifdef, lets re-do this patch so it doesn't.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to Mark Banner (:standard8) from comment #23)
> I'd prefer these on new bugs specifically for
> comm-central as it is far easier to track what has gone into a release. I
> also don't know where comm-5.0 came from as we never created that.

I filed bug 707517.