Closed Bug 1263797 Opened 4 years ago Closed 4 years ago

Examine installer to report component sizes

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(1 file)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1263662#c1.

Many builds report their installer size to perfherder, and some (especially Android) report the size of components, like omni.ja, classes.dex, etc. Currently the sizes of components are determined by getting the file size from the appropriate file in the objdir, and that is proving fragile as files move around. We should be able to examine the installer archive (apk, etc) instead of expecting particular file paths.

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildbase.py#1859
Could we instead just report this data from inside the build somehow? The build system will always know where its files are.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Could we instead just report this data from inside the build somehow? The
> build system will always know where its files are.

That's interesting. I think it would be simple to generate TinderboxPrints in strategic places -- after classes.dex is built, etc. But collecting those numbers in one place to generate the PERFHERDER_DATA would be another challenge.

Examining the installer archive looks fairly simple and has the advantage that we can be certain we are reporting the size of the component that is actually being distributed (vs perhaps an intermediate, unstripped, etc, version of the file).

Almost working: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8775f3b4ce60
I've run into a similar problem with artifact archive packaging where I want to "annotate" certain files as needing to be included in the artifact build. We might be able to shoehorn something into moz.build files for this bug and my needs.

For this bug, moz.build processing could write out a file with a list of files whose size to record and some build action could later iterate that list and do the perfherder foo.
This patch changes the way we find the "component" files (like libxul.so) sometimes reported alongsize installer sizes. Instead of expecting such files to be in a specific location in the build, the installer archive file is searched, using python tarfile/zipfile modules.

This new approach found some files - like omni.ja - on some platforms for which such files were not previously being reported (presumably because they are built in an unexpected location). Unfortunately, this also uncovered unexpected results: Some installer archives (Windows?) have more than one omni.ja. To avoid confusion, this patch does not report results for a file found more than once in an installer. The practical result is that installer component sizes reported with this patch are identical to existing component sizes:

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

I appreciate :ted's suggestion, but overall I like this approach better: Conceptually, it seems right to me that we report the size of libxul.so (etc) from the installer archive. It is being presented as a component of the installer, so let's actually read it from the installer.
Attachment #8742476 - Flags: review?(wlachance)
Comment on attachment 8742476 [details] [diff] [review]
find component files by searching installer archive

Review of attachment 8742476 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! One small suggestion.

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +1888,5 @@
> +                            if name in interests:
> +                                if name in subtests:
> +                                    # File seen twice in same archive;
> +                                    # ignore to avoid confusion.
> +                                    subtests[name] = 0

I think it would make slightly more sense to set the value to None here, instead of 0.

@@ +1892,5 @@
> +                                    subtests[name] = 0
> +                                else:
> +                                    subtests[name] = size
> +                for name in subtests:
> +                    if subtests[name] > 0:

If you follow my previous suggestion, this would become `if subtests[name] is not None`
Attachment #8742476 - Flags: review?(wlachance) → review+
Landing with the 0 -> None change -- makes sense to me. Thanks!
https://hg.mozilla.org/mozilla-central/rev/3a4a2e016507
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.