Closed Bug 1250624 Opened 4 years ago Closed 4 years ago

Overall system resources is displayed twice

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(1 file)

With the changes we to capture build system telemetry we call the BuildMonitor record_resource_usage method twice so we get the "Overall system resources..." line twice.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Comment on attachment 8723109 [details]
MozReview Request: Bug 1250624 - Overall system resources is displayed twice; r?chmanchester

https://reviewboard.mozilla.org/r/36355/#review33107

A couple of notes that might clean things up a bit.

::: python/mozbuild/mozbuild/controller/building.py:254
(Diff revision 1)
> -            usage = self.record_resource_usage()
> +            self.log_resource_usage()
> +            usage = self.get_resource_usage()
>              if not usage:
>                  return

Could we move log_resource_usage() below the early return here to avoid the check for have_resource_usage in log_resource_usage()?

::: python/mozbuild/mozbuild/controller/building.py:427
(Diff revision 1)
> +        cpu_percent = self.resources.aggregate_cpu_percent(phase=None,
> +            per_cpu=False)
> +        io = self.resources.aggregate_io(phase=None)

It seems a little silly (really not egregious) to re-compute these just to log them. Would it be simpler to:

usage = self.get_resource_usage()
self.log_resource_usage(usage)

?

::: python/mozbuild/mozbuild/controller/building.py:434
(Diff revision 1)
>              io_reads=io.read_count,
>              io_writes=io.write_count,

Two unused fields (pre-existing condition).
Attachment #8723109 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/36355/#review33107

> Could we move log_resource_usage() below the early return here to avoid the check for have_resource_usage in log_resource_usage()?

Moving it makes sense, but we should probably keep the check in the unlikely event someone calls it from elsewhere.
Comment on attachment 8723109 [details]
MozReview Request: Bug 1250624 - Overall system resources is displayed twice; r?chmanchester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36355/diff/1-2/
Attachment #8723109 - Flags: review?(cmanchester)
Comment on attachment 8723109 [details]
MozReview Request: Bug 1250624 - Overall system resources is displayed twice; r?chmanchester

https://reviewboard.mozilla.org/r/36355/#review33237

Thanks.
Attachment #8723109 - Flags: review?(cmanchester) → review+
https://hg.mozilla.org/mozilla-central/rev/babba7dc57ae
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.