Last Comment Bug 603574 - [Windows VC++] --disable-profile-guided-optimization is ignored
: [Windows VC++] --disable-profile-guided-optimization is ignored
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows Server 2003
: -- normal (vote)
: mozilla6
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 419348 588099 603485 707517
  Show dependency treegraph
 
Reported: 2010-10-12 02:41 PDT by Serge Gautherie (:sgautherie)
Modified: 2011-12-04 10:06 PST (History)
3 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(Av1) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Fix some unrelated nits (5.16 KB, patch)
2010-10-12 04:12 PDT, Serge Gautherie (:sgautherie)
ted: review+
Details | Diff | Review
(Av1a) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Error out on --enable-*, Fix some unrelated nits (10.51 KB, patch)
2010-10-14 20:03 PDT, Serge Gautherie (:sgautherie)
ted: review+
benjamin: approval2.0-
Details | Diff | Review
(Av1b) Move --disable-profile-guided-optimization outside of SKIP_COMPILER_CHECKS, Error out on --enable-*, Fix some unrelated nits (10.49 KB, patch)
2011-04-17 08:40 PDT, Serge Gautherie (:sgautherie)
ted: review-
Details | Diff | Review
(Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits (10.06 KB, patch)
2011-05-08 13:47 PDT, Serge Gautherie (:sgautherie)
ted: review+
Details | Diff | Review
(Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits [Checked in: Comment 21] (10.01 KB, patch)
2011-05-14 07:04 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Review
(Bv1-CC) Remove --disable-profile-guided-optimization support, Fix some unrelated nits [Moved to bug 707517] (5.02 KB, patch)
2011-05-16 08:06 PDT, Serge Gautherie (:sgautherie)
bugspam.Callek: review-
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2010-10-12 02:41:31 PDT
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 [:khuey] (khuey@mozilla.com) 2010-10-12 03:28:41 PDT
Why do we even have this flag?  Can't the difference in make targets handle this?
Comment 2 Serge Gautherie (:sgautherie) 2010-10-12 04:12:42 PDT
Created attachment 482501 [details] [diff] [review]
(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 Ted Mielczarek [:ted.mielczarek] 2010-10-12 05:27:03 PDT
(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) 2010-10-12 05:51:47 PDT
(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 [:khuey] (khuey@mozilla.com) 2010-10-12 06:02:03 PDT
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 Ted Mielczarek [:ted.mielczarek] 2010-10-12 06:11:09 PDT
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 [:khuey] (khuey@mozilla.com) 2010-10-12 06:11:56 PDT
So make it error when --enable is selected.
Comment 8 Serge Gautherie (:sgautherie) 2010-10-12 08:19:15 PDT
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/sgautherie.bz@free.fr-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 Ted Mielczarek [:ted.mielczarek] 2010-10-14 11:50:39 PDT
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) 2010-10-14 20:03:24 PDT
Created attachment 483377 [details] [diff] [review]
(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 Ted Mielczarek [:ted.mielczarek] 2010-10-15 05:54:55 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-03-22 07:19:36 PDT
This can land in the build-system branch.
Comment 13 Serge Gautherie (:sgautherie) 2011-04-17 08:40:03 PDT
Created attachment 526590 [details] [diff] [review]
(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] 2011-04-28 00:39:40 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-04-28 05:24:09 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-05-06 09:49:47 PDT
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) 2011-05-08 13:47:38 PDT
Created attachment 530948 [details] [diff] [review]
(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 Ted Mielczarek [:ted.mielczarek] 2011-05-12 05:37:42 PDT
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] 2011-05-14 06:24:31 PDT
Serge, please provide a commit message following the "Bug XXX - Message. r=foo" pattern.
Comment 20 Serge Gautherie (:sgautherie) 2011-05-14 07:04:16 PDT
Created attachment 532434 [details] [diff] [review]
(Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits
[Checked in: Comment 21]
Comment 21 Dão Gottwald [:dao] 2011-05-16 03:45:04 PDT
http://hg.mozilla.org/mozilla-central/rev/5a70c6b33303
Comment 22 Serge Gautherie (:sgautherie) 2011-05-16 08:06:36 PDT
Created attachment 532646 [details] [diff] [review]
(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) 2011-05-16 08:20:38 PDT
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) 2011-06-10 20:39:17 PDT
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) 2011-12-04 10:06:00 PST
(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.

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