Open Bug 1108293 Opened 8 years ago Updated 4 years ago

mach command for analyzing C/C++ dependencies

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Let's provide a mach command to provide useful C/C++ dependency analysis to help developers understand how changes to certain files cause rebuilds.
Depends on: 1108294
Isn't that what REBUILD_CHECK=1 does? (but doesn't work on windows)
Depends on: 1108399
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.
Attached file MozReview Request: bz://1108293/gps (obsolete) —
Attachment #8532963 - Flags: review?(mh+mozilla)
/r/1297 - Bug 1108293 - mach dependencies: analyze build system dependencies

Pull down this commit:

hg pull review -r 57f0b00a7b5dcc3a69eb17eabede929903794e90
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.
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).
(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).
(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.
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.
Attachment #8532963 - Flags: review?(mh+mozilla)
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....
Attachment #8532963 - Flags: review?(mh+mozilla)
/r/1297 - Bug 1108293 - mach dependencies: analyze build system dependencies

Pull down this commit:

hg pull review -r 4303305b818c064faad547b1a11933fa5b1c2dcc
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.
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.
Attachment #8532963 - Flags: review?(mh+mozilla)
Attachment #8532963 - Attachment is obsolete: true
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.
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.
Assignee: gps → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1249793
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.
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.