1108293 - mach command for analyzing C/C++ dependencies

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Let's provide a mach command to provide useful C/C++ dependency analysis to help developers understand how changes to certain files cause rebuilds.

Comment 1

[Mike Hommey [:glandium]]

Isn't that what REBUILD_CHECK=1 does? (but doesn't work on windows)

Comment 2

[Gregory Szorc [:gps]]

I'm writing code for loading all .pp files and formatting useful output. Show dependencies sorted by count, should targets for a given dependency, etc. Should have code up for review soonish.

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1108293/gps

Comment 4

[Gregory Szorc [:gps]]

/r/1297 - Bug 1108293 - mach dependencies: analyze build system dependencies

Pull down this commit:

hg pull review -r 57f0b00a7b5dcc3a69eb17eabede929903794e90

Comment 5

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/1295/#review691

I hacked this up on the long train ride yesterday. I'm not yet sure how useful it is. It should be easy to add functionality.

I had to rebase this patch on top of central to appease the MozReview gods. It should be parented on fcebbe1c01c9 to get the mach sub-command and AccEventGen.py required pre-requisites.

Comment 6

[(not currently active) Ted Mielczarek]

I thought the thing we talked about last week was "which files cause the most objects to be rebuilt", right? The thing you're building is "which files could potentially cause the most objects to be rebuilt", if I'm interpreting your comments correctly (which still seems useful, but doesn't give us the actual stats on what causes rebuilds in the real world).

Comment 7

[Michael Shal [:mshal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> I thought the thing we talked about last week was "which files cause the
> most objects to be rebuilt", right? The thing you're building is "which
> files could potentially cause the most objects to be rebuilt", if I'm
> interpreting your comments correctly (which still seems useful, but doesn't
> give us the actual stats on what causes rebuilds in the real world).

You could combine this part with just going through the revision log to see which files are actually changed though, right? (Like git log --stat, or whatever the hg equivalent is).

Comment 8

[Gregory Szorc [:gps]]

(In reply to Michael Shal [:mshal] from comment #7)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> > I thought the thing we talked about last week was "which files cause the
> > most objects to be rebuilt", right? The thing you're building is "which
> > files could potentially cause the most objects to be rebuilt", if I'm
> > interpreting your comments correctly (which still seems useful, but doesn't
> > give us the actual stats on what causes rebuilds in the real world).
> 
> You could combine this part with just going through the revision log to see
> which files are actually changed though, right? (Like git log --stat, or
> whatever the hg equivalent is).

Pretty much.

We could also record $? at build time and analyze that. That could be a follow-up feature. You have to start somewhere. If nothing else, this patch gets us a basic platform on which to build more tools.

Comment 9

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1295/#review893

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +    @SubCommand('dependencies', 'dependency-counts',

mach dependencies dependency-counts... that seems a bit of a stretch. mach dependencies counts?

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +    @CommandArgument('--show-system', action='store_true',

show-all maybe?

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +        help='Show objdir paths even if they resolve to known source files.')

As a user, I wouldn't know what this means.

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +        for d in sorted(deps, key=lambda k: len(deps[k]), reverse=True):

Seems to me this should be sorted by number *and* path.

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +        path = mozpath.normpath(mozpath.abspath(path))

abspath returns normalized paths.

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +        if path.startswith(self.topobjdir):

if mozpath.basedir(path, [self.topobjdir])

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +            return '<objdir>/%s' % path[len(self.topobjdir) + 1:]

mozpath.join('<objdir>', mozpath.relpath(path, self.topobdir>))

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
> +            return path[len(self.topsrcdir) + 1:]

Why not prefix with <srcdir>?

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +    """Find paths to make dependency files.

s/make/Make/

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +        if not root.endswith('.deps'):

mozpath.basename(root) != '.deps'

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +        self.topobjdir = mozpath.normpath(topobjdir)

Aren't those paths supposed to be normalized already?

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +        """Obtain a Dependencies with system paths pruned."""

Seems to me this should be a flag to load_deps_file instead.

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +            depends = [d for d in depends if d.startswith(allowed)]

mozpath.basedir(d, allowed)

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +            if not target.startswith(allowed):

mozpath.basedir(target, allowed)

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +            raise Exception('Unknown install manifest encountered: %s' % f)

Seems to me this should be where install manifests are handled, such that if we ever add more, this doesn't silently start failing.

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +    def resolve_srcdir_paths(self):

like prune_system_paths, it seems to me this would be better as a flag to load_deps_file.

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +        if not source.endswith(('.c', '.cpp')):

There are many other source extensions.

::: python/mozbuild/mozbuild/analyze.py
(Diff revision 1)
> +    def get_targets(self, path, resolve_source=False):

This would probably be simpler if this was implemented as a filter during load_deps_file.

Comment 10

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa221e122997

Comment 11

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/1295/#review1115

> mozpath.basedir(target, allowed)

I'm using startswith(tuple) for performance reasons. All these string lookups and manipulations actually matter for performance.

> like prune_system_paths, it seems to me this would be better as a flag to load_deps_file.

Parsing is the most expensive operation. I wanted to optimize for parsing just once. That's how I settled on the immutable data structures API.

> Why not prefix with <srcdir>?

I thought it looked nicer. Without the prefixing, terminal word selection and file opening just works. Let me come up with a better solution....

Comment 12

[Gregory Szorc [:gps]]

/r/1297 - Bug 1108293 - mach dependencies: analyze build system dependencies

Pull down this commit:

hg pull review -r 4303305b818c064faad547b1a11933fa5b1c2dcc

Comment 13

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1295/#review1123

::: python/mozbuild/mozbuild/analyze.py
(Diff revisions 1 - 2)
>              if not target.startswith(allowed):

There's a reason to use mozpath.basedir instead. mozpath.basedir is correct. Using startswith is easy to get wrong, and you're doing that. If mozpath.basedir is not fast enough, make that faster. Also, have you actually profiled that using mozpath.basedir makes a (bad) difference?

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revisions 1 - 2)
>          for d in sorted(deps, key=lambda k: len(deps[k]), reverse=True):

Same comment as previous review: this should be sorted by number *and* path.

::: python/mozbuild/mozbuild/analyze.py
(Diff revisions 1 - 2)
>              raise Exception('Unknown install manifest encountered: %s' % f)

Same comment as previous review: this should be where install manifests are handled, such that if we ever add more, this doesn't silently start failing.

Comment 14

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1295/#review1125

> Parsing is the most expensive operation. I wanted to optimize for parsing just once. That's how I settled on the immutable data structures API.

Even if load_deps_file did everything you wouldn't be parsing twice, because you're never calling _get_deps twice in the same process anyways. I'd rather have simpler code than code prematurely optimized.

Comment 16

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1108293 - mach dependencies: analyze build system dependencies

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: part 1a - resolve symbolic links whenever possible

These two patches are things I found useful when playing around with |mach
dependencies|.  I think we also want a:

  mach dependencies how-did-i-include $HEADER $FILE

command, but that can come later.  Anyway, on to the explanation.

Entering:

  mach dependencies target $HEADER

tells you what depends on $HEADER...unless $HEADER is exported.  Then
you get different answers depending on whether you pass a
absolute-srcdir or an absolute-objdir path.  This patch at least makes
the answers what you expect when you pass a absolute-srcdir path, as we
now normalize symbolic links when recording dependencies.  (This patch
doesn't help systems where we don't have symbolic links, and it also
makes the absolute-objdir paths for such files impossible to query data
for.  We can at least partially solve the latter problem in a future
patch.)

Because resolving symbolic links repeatedly can be expensive, we include
a cache for the resolved results.  This cache makes things noticeably
faster, even on an SSD.

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: part 1b - add some DWIM to |mach dependencies targets|

We have enough information in mach commands to permit both srcdir- and
objdir-relative pathnames; we should permit both for convenience.  Given
that we resolve symbolic links in mozbuild.analyze.Dependencies, we
should do the same before passing the path into
Dependencies.get_targets, so that objdir paths Just Work.

Comment 19

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d07fb77725

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

As I wrote in the description of the bug I just duped:

The output that MSVC's -showIncludes option generates can be very useful sometimes for diagnosing problems with includes. I'd like to be able to specify an option to mach build (or something) so that this data could be teed out to a log verbatim.

The .pp files in deps aren't as useful to the human reader because they strip out both the indentation and any system headers, both of which necessary for figuring out the real structure of the include tree.

Comment 22

[Gregory Szorc [:gps]]

aklotz: you may be interested in python/mozbuild/mozbuild/action/cl.py, which is a wrapper we already use to invoke cl.exe. It adds -showIncludes to invocations so we can parse the output and write .deps files. You could probably hack it up to write results into a separate file for analysis if you wanted to do something quick and dirty.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1108293 - mach dependencies: analyze build system dependencies

Developers often want to know how header files and other files factor in
to build dependencies. This patch introduces the |mach dependencies|
command for querying dependency info from the build system.

It only offers basic querying so far. Functionality can be expanded as
requested by developers.

* * *

Bug 1108293 - part 1a - resolve symbolic links whenever possible

Entering:

  mach dependencies target $HEADER

tells you what depends on $HEADER...unless $HEADER is exported.  Then
you get different answers depending on whether you pass a
absolute-srcdir or an absolute-objdir path.  This patch at least makes
the answers what you expect when you pass a absolute-srcdir path, as we
now normalize symbolic links when recording dependencies.  (This patch
doesn't help systems where we don't have symbolic links, and it also
makes the absolute-objdir paths for such files impossible to query data
for.  We can at least partially solve the latter problem in a future
patch.)

Because resolving symbolic links repeatedly can be expensive, we include
a cache for the resolved results.  This cache makes things noticeably
faster, even on an SSD.

* * *

part 1b - add some DWIM to |mach dependencies targets|

We have enough information in mach commands to permit both srcdir- and
objdir-relative pathnames; we should permit both for convenience.  Given
that we resolve symbolic links in mozbuild.analyze.Dependencies, we
should do the same before passing the path into
Dependencies.get_targets, so that objdir paths Just Work.

Review commit: https://reviewboard.mozilla.org/r/252136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252136/