Add system and command metadata to resource_usage.json

RESOLVED FIXED in Firefox 46

Status

Firefox Build System
General
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46

Firefox Tracking Flags

(firefox46 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

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

Updated

2 years ago
Assignee: nobody → dminor
(Assignee)

Comment 1

2 years ago
Created attachment 8706547 [details]
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/
Attachment #8706547 - Flags: review?(gps)
(Assignee)

Comment 2

2 years ago
Created attachment 8706548 [details]
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/
Attachment #8706548 - Flags: review?(gps)
(Assignee)

Comment 3

2 years ago
Created attachment 8706549 [details]
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/
Attachment #8706549 - Flags: review?(gps)

Updated

2 years ago
Attachment #8706547 - Flags: review?(gps) → review+

Comment 4

2 years ago
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

2 years ago
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.)
Attachment #8706548 - Flags: review?(gps) → review+

Comment 6

2 years ago
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.
Attachment #8706549 - Flags: review?(gps)

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

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

Updated

2 years ago
Attachment #8706549 - Flags: review?(gps)
(Assignee)

Comment 11

2 years ago
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/
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.

Updated

2 years ago
Attachment #8706549 - Flags: review?(gps) → review+
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.
(Assignee)

Comment 14

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

Comment 15

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

Comment 16

2 years ago
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 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5ef13bfbf24
https://hg.mozilla.org/mozilla-central/rev/058ed31232d1
https://hg.mozilla.org/mozilla-central/rev/e7b44cddd003
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

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