The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Trunk
mozilla6
x86
Windows Server 2003
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
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.
}
Why do we even have this flag?  Can't the difference in make targets handle this?
(Assignee)

Updated

7 years ago
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Flags: in-testsuite-
Summary: --disable-profile-guided-optimization doesn't seem to work → [Windows VC++] --disable-profile-guided-optimization is ignored
(Assignee)

Comment 2

7 years ago
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 :->
Attachment #482501 - Flags: review?(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?
(Assignee)

Comment 4

7 years ago
(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)?
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.
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.
So make it error when --enable is selected.
(Assignee)

Comment 8

7 years ago
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.)
(Assignee)

Updated

7 years ago
Blocks: 588099
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.
Attachment #482501 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 10

7 years ago
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'.
Attachment #482501 - Attachment is obsolete: true
Attachment #483377 - Flags: review?(ted.mielczarek)
Attachment #483377 - Flags: approval2.0?
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
Attachment #483377 - Flags: review?(ted.mielczarek) → review+
Attachment #483377 - Flags: approval2.0? → approval2.0-
This can land in the build-system branch.
(Assignee)

Comment 13

6 years ago
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.
Attachment #483377 - Attachment is obsolete: true
Attachment #526590 - Flags: review?(ted.mielczarek)
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)
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 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.
Attachment #526590 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 17

6 years ago
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" :-)
Attachment #526590 - Attachment is obsolete: true
Attachment #530948 - Flags: review?(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
Attachment #530948 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Serge, please provide a commit message following the "Bug XXX - Message. r=foo" pattern.
Keywords: checkin-needed
(Assignee)

Comment 20

6 years ago
Created attachment 532434 [details] [diff] [review]
(Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits
[Checked in: Comment 21]
Attachment #530948 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5a70c6b33303
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Updated

6 years ago
Attachment #532434 - Attachment description: (Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits → (Av2) Remove --disable-profile-guided-optimization support, Fix some unrelated nits [Checked in: Comment 21]
(Assignee)

Comment 22

6 years ago
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.
Attachment #532646 - Flags: review?(mbanner)
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.
Attachment #532646 - Flags: review?(mbanner) → review?(bugspam.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.
Attachment #532646 - Flags: review?(bugspam.Callek) → review-
(Assignee)

Updated

5 years ago
Blocks: 707517
(Assignee)

Updated

5 years ago
Attachment #532646 - Attachment description: (Bv1-CC) Remove --disable-profile-guided-optimization support, Fix some unrelated nits → (Bv1-CC) Remove --disable-profile-guided-optimization support, Fix some unrelated nits [Moved to bug 707517]
Attachment #532646 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.