1237619 - Add system and command metadata to resource_usage.json

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dan Minor [:dminor]]

We already collect resource usage data during the build and store it in resource_usage.json. Adding metadata about the local system (os, cpu, ram, etc.) and the build target would be a first step to getting something useful to report for Bug 1237610 on which we can then iterate.

Comment 1

[Dan Minor [:dminor]]

Attachment: MozReview Request: bug 1237619: save resource usage for "what" builds r?gps

This moves monitor start/end recording so it also occurs for "what"
builds rather than only for full builds.

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

Comment 2

[Dan Minor [:dminor]]

Attachment: MozReview Request: bug 1237619: Add system and command metadata to resouce_usage.json r?gps

This adds metadata about the local system to resource_usage.json that will
be useful for analysis when we start collecting these results. Some of it
is redundant with data collected for individual tiers, but having it
available at a top level should make analysis easier.

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

Comment 3

[Dan Minor [:dminor]]

Attachment: MozReview Request: bug 1237619: Record build objects in resource_usage.json r?gps

Collecting the list of object files compiled, while not ideal, will give us
some indication of how much work was involved in the build. This will help
with analyzing the data.

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

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8706547 [details]
MozReview Request: bug 1237619: save resource usage for "what" builds r?gps

https://reviewboard.mozilla.org/r/30373/#review27149

IIRC, the BuildMonitor and resource viewer have a somewhat tight coupling to the build "tiers." These tiers always exist in full builds. I wouldn't be surprised if their omission from non-full builds ends up breaking something.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8706548 [details]
MozReview Request: bug 1237619: Add system and command metadata to resouce_usage.json r?gps

https://reviewboard.mozilla.org/r/30375/#review27151

::: python/mozbuild/mozbuild/controller/building.py:375
(Diff revision 1)
> -            version=1,
> +            version=2,

Does this not break the HTML resource viewer? If it doesn't, shame on us for not checking the version number anywhere.

::: python/mozbuild/mozbuild/controller/building.py:409
(Diff revision 1)
> +        o['system'] = dict(

Could we also throw in `platform.python_version()`, and the various platform-specific versions starting at https://docs.python.org/2/library/platform.html#windows-platform?

::: python/mozbuild/mozbuild/controller/building.py:411
(Diff revision 1)
> +            cpu_count=psutil.cpu_count(),

Newer versions of psutil also have a `physical_cpu_count()`. I'd love for this to be added to. It shouldn't be that much work to upgrade the in-tree psutil if it is out of date. (We should probably upgrade it anyway since they are pretty good about fixing bugs upstream.)

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8706549 [details]
MozReview Request: bug 1237619: Record build objects in resource_usage.json r?gps

https://reviewboard.mozilla.org/r/30377/#review27153

::: python/mozbuild/mozbuild/controller/building.py:61
(Diff revision 1)
> +BUILDOBJECT_EXTS = ('.o', '.obj')

We should be able to get this extension from `self.substs['OBJ_SUFFIX']`.

::: python/mozbuild/mozbuild/controller/building.py:230
(Diff revision 1)
> +        if line.endswith(BUILDOBJECT_EXTS):

This is slurping the "echo" output from the compile make targets. It's a bit fragile and possibly leads to false positives. A more robust solution is for the build backend itself to write out file(s) indicating what it is doing and have the BuildMonitor slurp those files post-build. Search for `CPPOBJS` and `HOST_CPPOBJS` in config/rules.mk for the make rules that handle compilation.

It's worth noting that I've long wanted to install a compiler wrapper script that invokes the requested compilation command via psutil so we can get per-process metrics.

That being said, this overly simple approach might be good enough. I'm willing to r+ if another build peer is OK with it.

Comment 7

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/30377/#review27153

> This is slurping the "echo" output from the compile make targets. It's a bit fragile and possibly leads to false positives. A more robust solution is for the build backend itself to write out file(s) indicating what it is doing and have the BuildMonitor slurp those files post-build. Search for `CPPOBJS` and `HOST_CPPOBJS` in config/rules.mk for the make rules that handle compilation.
> 
> It's worth noting that I've long wanted to install a compiler wrapper script that invokes the requested compilation command via psutil so we can get per-process metrics.
> 
> That being said, this overly simple approach might be good enough. I'm willing to r+ if another build peer is OK with it.

And as glandium pointed out on IRC, we only print the filename if building with `make -s` / `mach -v`. See the `ELOG` definition in config/rules.mk.

Comment 8

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/30375/#review27151

> Does this not break the HTML resource viewer? If it doesn't, shame on us for not checking the version number anywhere.

I *really* meant to check that before pushing this for review :(

Comment 9

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/30375/#review27151

> Newer versions of psutil also have a `physical_cpu_count()`. I'd love for this to be added to. It shouldn't be that much work to upgrade the in-tree psutil if it is out of date. (We should probably upgrade it anyway since they are pretty good about fixing bugs upstream.)

It's cpu_count(logical=False) and is supported with the current version. I filed Bug 1238983 so we don't forget to upgrade the in-tree version at some point.

Comment 10

[Dan Minor [:dminor]]

Comment on attachment 8706548 [details]
MozReview Request: bug 1237619: Add system and command metadata to resouce_usage.json r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30375/diff/1-2/

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8706549 [details]
MozReview Request: bug 1237619: Record build objects in resource_usage.json r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30377/diff/1-2/

Comment 12

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/30375/#review27389

::: python/mozbuild/mozbuild/controller/building.py:424
(Diff revisions 1 - 2)
> +        # mac version is a special Cupertino snowflake
> +        r, v, m = platform.mac_ver()
> +        o['system']['mac_ver'] = [r, list(v), m]

I think we should only record `linux_distribution`, `win32_ver`, and `mac_ver` if the current system is the respective platform.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8706549 [details]
MozReview Request: bug 1237619: Record build objects in resource_usage.json r?gps

https://reviewboard.mozilla.org/r/30377/#review27391

Looks good. This adds a simple `echo` to each object file, which shouldn't be too expensive.

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8706547 [details]
MozReview Request: bug 1237619: save resource usage for "what" builds r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30373/diff/1-2/

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8706548 [details]
MozReview Request: bug 1237619: Add system and command metadata to resouce_usage.json r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30375/diff/2-3/

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8706549 [details]
MozReview Request: bug 1237619: Record build objects in resource_usage.json r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30377/diff/2-3/

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ef13bfbf24
https://hg.mozilla.org/integration/mozilla-inbound/rev/058ed31232d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b44cddd003

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5ef13bfbf24
https://hg.mozilla.org/mozilla-central/rev/058ed31232d1
https://hg.mozilla.org/mozilla-central/rev/e7b44cddd003