Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build

RESOLVED FIXED in mozilla28

Status

Firefox Build System
General
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla28
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Depends on: 932618
(Assignee)

Comment 1

5 years ago
NO_PROFILE_GUIDED_OPTIMIZE currently can have two different values:
- 1 to disable in the whole directory
- a list of files that need not to be optimized

As sandbox values can only have one type, the same can't be done.

So, we need two variables. I'm proposing NO_PGO and NO_PGO_SOURCES. I think NO_PROFILE_GUIDED_OPTIMIZE is too long and too error prone for people to type (I often myself wonder if it's optimize, optimized or optimization). I did suggest in bug 932618 we could use NO_PGO = SOURCES for the = 1 case, but that's dangerous in the light of unified sources migration, and there might be people adding sources after NO_PGO is set.

I can see two different strategies for NO_PGO_SOURCES:
- Like now, list which files from the SOURCES list need not to be PGOed. We'd require the names in NO_PGO_SOURCES to have to be in SOURCES and *not* in UNIFIED_SOURCES.
- NO_PGO_SOURCES would be an independent list of sources, to supplement SOURCES and UNIFIED_SOURCES. So we'd requires the names in NO_PGO_SOURCES to *not* be in either SOURCES or UNIFIED_SOURCES. The disadvantage here is that whenever we conditionally add to NO_PGO_SOURCES (for msvc-only bugs), we'd need to add the file to SOURCES or UNIFIED_SOURCES on the else branch. Which in turn, could be considered an advantage because it would allow to use UNIFIED_SOURCES more easily when not excluding from pgo.

Now that I've written this, i realize i very much prefer the latter, so this is what i'm going to implement.

Thoughts?
(Assignee)

Updated

5 years ago
Depends on: 763495

Comment 2

5 years ago
I think the former option makes way more sense from the viewpoint of the consumers of this variable (non-build system hackers) since in their view, all things are PGOed unless you explicitly opt out of it, which is what NO_PGO_SOURCES seems like it would let you do.

Updated

5 years ago
Summary: Move NO_PROFILE_GUIDED_OPTIMIZE to moz.glue → Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build
(Assignee)

Comment 3

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> I think the former option makes way more sense from the viewpoint of the
> consumers of this variable (non-build system hackers) since in their view,
> all things are PGOed unless you explicitly opt out of it, which is what
> NO_PGO_SOURCES seems like it would let you do.

Do you prefer:

  if some_condition:
    SOURCES += [ 'foo.cpp']
    NO_PGO_SOURCES += ['foo.cpp']
  else:
    UNIFIED_SOURCES += ['foo.cpp']

or

  if some_condition:
    NO_PGO_SOURCES += ['foo.cpp']
  else:
    UNIFIED_SOURCES += ['foo.cpp']

Note that going with the former would also set a precedent. None of the currently existing *SOURCES variable requires another variable to contain the same thing (SOURCES, GENERATED_SOURCES, UNIFIED_SOURCES, GTEST_SOURCES when that still existed). Also note that with the same kind of logic, we should have SOURCES += ['foo.cpp']; UNIFIED_SOURCES += ['foo.cpp'].
(Assignee)

Comment 4

5 years ago
Also, as Ms2ger reminded me on irc, the default is to no PGO for MSVC unless MSVC_ENABLE_PGO is set.
I'm fine with the name change. I don't remember why I made that variable so verbose at the time, probably just to try to make it clearer.

Also yeah, things are more confusing these days with opt-in MSVC PGO and opt-out GCC PGO.

Comment 6

5 years ago
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > I think the former option makes way more sense from the viewpoint of the
> > consumers of this variable (non-build system hackers) since in their view,
> > all things are PGOed unless you explicitly opt out of it, which is what
> > NO_PGO_SOURCES seems like it would let you do.
> 
> Do you prefer:
> 
>   if some_condition:
>     SOURCES += [ 'foo.cpp']
>     NO_PGO_SOURCES += ['foo.cpp']
>   else:
>     UNIFIED_SOURCES += ['foo.cpp']

Ideally we should never have code which puts something in either SOURCES or UNIFIED_SOURCES based on a build time condition.  When in doubt, we should always put stuff in SOURCES.

> or
> 
>   if some_condition:
>     NO_PGO_SOURCES += ['foo.cpp']
>   else:
>     UNIFIED_SOURCES += ['foo.cpp']

I was actually thinking about this:

SOURCES += ['foo.cpp']
if CONFIG['_MSC_VER']:
    # Don't PGO this on Windows
    DONT_PGO += ['foo.cpp']

Perhaps DONT_PGO is a better name than NO_PGO_SOURCES.

> Note that going with the former would also set a precedent. None of the
> currently existing *SOURCES variable requires another variable to contain
> the same thing (SOURCES, GENERATED_SOURCES, UNIFIED_SOURCES, GTEST_SOURCES
> when that still existed).

Well, maybe this is just a naming issue?  Let's just call it something without "SOURCES" in it.  The semantics that I would like to see is for me to be able to express "Here's the set of sources for this directory", and "Here's a subset of those sources which should not be PGOed under some conditions", so that if those conditions change for example, people don't need to "add" anything back to a *SOURCES variable.  IOW, DONT_PGO would just serve as an exclusion list for PGO, not as a way to declare your source files.

> Also note that with the same kind of logic, we
> should have SOURCES += ['foo.cpp']; UNIFIED_SOURCES += ['foo.cpp'].

I'm not sure if I understand this part.

(In reply to Mike Hommey [:glandium] from comment #4)
> Also, as Ms2ger reminded me on irc, the default is to no PGO for MSVC unless
> MSVC_ENABLE_PGO is set.

I think that's fine, if you consider DONT_PGO as an override mechanism, meaning "whatever you do, don't PGO this thing, ever!"
(Assignee)

Comment 7

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > > I think the former option makes way more sense from the viewpoint of the
> > > consumers of this variable (non-build system hackers) since in their view,
> > > all things are PGOed unless you explicitly opt out of it, which is what
> > > NO_PGO_SOURCES seems like it would let you do.
> > 
> > Do you prefer:
> > 
> >   if some_condition:
> >     SOURCES += [ 'foo.cpp']
> >     NO_PGO_SOURCES += ['foo.cpp']
> >   else:
> >     UNIFIED_SOURCES += ['foo.cpp']
> 
> Ideally we should never have code which puts something in either SOURCES or
> UNIFIED_SOURCES based on a build time condition.  When in doubt, we should
> always put stuff in SOURCES.

Why not, in cases like this, where the file needs to be in SOURCES just because it can't be PGOed on one platform because of a compiler bug?

> > Also note that with the same kind of logic, we
> > should have SOURCES += ['foo.cpp']; UNIFIED_SOURCES += ['foo.cpp'].
> 
> I'm not sure if I understand this part.

With the same kind of logic, we'd be declaring SOURCES and then the subset of those that can be unified.

Comment 8

5 years ago
(In reply to comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> > (In reply to Mike Hommey [:glandium] from comment #3)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > > > I think the former option makes way more sense from the viewpoint of the
> > > > consumers of this variable (non-build system hackers) since in their view,
> > > > all things are PGOed unless you explicitly opt out of it, which is what
> > > > NO_PGO_SOURCES seems like it would let you do.
> > > 
> > > Do you prefer:
> > > 
> > >   if some_condition:
> > >     SOURCES += [ 'foo.cpp']
> > >     NO_PGO_SOURCES += ['foo.cpp']
> > >   else:
> > >     UNIFIED_SOURCES += ['foo.cpp']
> > 
> > Ideally we should never have code which puts something in either SOURCES or
> > UNIFIED_SOURCES based on a build time condition.  When in doubt, we should
> > always put stuff in SOURCES.
> 
> Why not, in cases like this, where the file needs to be in SOURCES just because
> it can't be PGOed on one platform because of a compiler bug?

Because that will shift code into different unified modules on different platforms, which will make it easier for you to write and test your patch on one platform and have it break the build on another, which is painful for developers.

> > > Also note that with the same kind of logic, we
> > > should have SOURCES += ['foo.cpp']; UNIFIED_SOURCES += ['foo.cpp'].
> > 
> > I'm not sure if I understand this part.
> 
> With the same kind of logic, we'd be declaring SOURCES and then the subset of
> those that can be unified.

Hmm, I was not implying this at all!

Comment 9

5 years ago
Instead of introducing a new variable, how about annotating the existing SOURCES and UNIFIED_SOURCES variables?

We could have an attribute with a list or set of sources to not PGO:

  SOURCES += ['foo.cpp', 'bar'.cpp']
  SOURCES.nopgo += ['foo.cpp']

Or, since there are a few things still in the tree with source/variable specific overrides, we could allow retrieval of source objects by filename and then set an attribute:

  SOURCES += ['foo.cpp', 'bar.cpp']
  SOURCES['foo.cpp'].nopgo = True
(Assignee)

Comment 10

5 years ago
(In reply to Gregory Szorc [:gps] from comment #9)
>   SOURCES += ['foo.cpp', 'bar.cpp']
>   SOURCES['foo.cpp'].nopgo = True

That's an interesting idea, because it could be extended to express those file specific things we currently have in makefiles.
(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #9)
>   SOURCES += ['foo.cpp', 'bar.cpp']
>   SOURCES['foo.cpp'].nopgo = True

+1, I like this. I guess if you have multiple sources you can do like:
for s in ['foo.cpp', 'bar.cpp']:
  SOURCES[s].nopgo = True

?

Comment 12

5 years ago
(In reply to comment #10)
> (In reply to Gregory Szorc [:gps] from comment #9)
> >   SOURCES += ['foo.cpp', 'bar.cpp']
> >   SOURCES['foo.cpp'].nopgo = True
> 
> That's an interesting idea, because it could be extended to express those file
> specific things we currently have in makefiles.

That seems fine.
(Assignee)

Updated

5 years ago
Depends on: 946175
(Assignee)

Comment 13

5 years ago
Created attachment 8342920 [details] [diff] [review]
Add moz.build infrastructure to replace NO_PROFILE_GUIDED_OPTIMIZE from Makefile.in
Attachment #8342920 - Flags: review?(gps)
(Assignee)

Comment 14

5 years ago
Created attachment 8342921 [details] [diff] [review]
Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build
Attachment #8342921 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Blocks: 778236
(Assignee)

Comment 15

5 years ago
Created attachment 8342933 [details] [diff] [review]
Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build

Fixed typo
Attachment #8342933 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8342921 - Attachment is obsolete: true
Attachment #8342921 - Flags: review?(gps)

Comment 16

5 years ago
Note: I just landed bug 946877 which added a new use of this variable.
(Assignee)

Comment 17

5 years ago
Created attachment 8344054 [details] [diff] [review]
Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build

Updated to current trunk
Attachment #8344054 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8342933 - Attachment is obsolete: true
Attachment #8342933 - Flags: review?(gps)
(Assignee)

Comment 18

5 years ago
Created attachment 8344437 [details] [diff] [review]
Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build

Nick for the header change. Greg for the rest.
Attachment #8344437 - Flags: review?(n.nethercote)
Attachment #8344437 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8344054 - Attachment is obsolete: true
Attachment #8344054 - Flags: review?(gps)
Comment on attachment 8344437 [details] [diff] [review]
Move NO_PROFILE_GUIDED_OPTIMIZE to moz.build

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

The RangeAnalysis.h change?  Sure, have an r+.  That and $4 will buy you a coffee :)
Attachment #8344437 - Flags: review?(n.nethercote) → review+
Comment on attachment 8342920 [details] [diff] [review]
Add moz.build infrastructure to replace NO_PROFILE_GUIDED_OPTIMIZE from Makefile.in

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

::: python/mozbuild/mozbuild/util.py
@@ +322,5 @@
> +    assert all(isinstance(v, type) for v in flags.values())
> +
> +    class Flags(object):
> +        __slots__ = flags.keys()
> +        __flags__ = flags

__flags__ isn't a known dunder variable. We shouldn't be using __ prefixes for non-core things.

@@ +356,5 @@
> +            self._flags = dict()
> +
> +        def __getitem__(self, name):
> +            if name not in self._flags:
> +                if not name in self:

if name not in self:

@@ +357,5 @@
> +
> +        def __getitem__(self, name):
> +            if name not in self._flags:
> +                if not name in self:
> +                    raise KeyError("'%s'" % name)

That's a weird error message.

@@ +358,5 @@
> +        def __getitem__(self, name):
> +            if name not in self._flags:
> +                if not name in self:
> +                    raise KeyError("'%s'" % name)
> +                self._flags[name] = self._flags_type()

self._flags_type() seems wrong.
Attachment #8342920 - Flags: review?(gps) → feedback+

Updated

5 years ago
Attachment #8344437 - Flags: review?(gps) → review+

Updated

5 years ago
Attachment #8342920 - Flags: review+
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/mozilla-central/rev/473d459b4bba
https://hg.mozilla.org/mozilla-central/rev/85196889c598
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to Mike Hommey [:glandium] from comment #21)
> https://hg.mozilla.org/mozilla-central/rev/85196889c598

had to backout this change because of testfailures in mochitests on windows pgo like https://tbpl.mozilla.org/php/getParsedLog.php?id=31675797&tree=Mozilla-Central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 947916
https://hg.mozilla.org/mozilla-central/rev/c7b7b00e867f
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 951587

Updated

4 years ago
Whiteboard: [qa-]

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.