1149164 - Make Android APK (executable, libxul?) size more visible

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

APK file size is a significant, on-going concern to the Firefox for Android team. More generally, we want to keep Firefox as trim as possible on all platforms. Also, much growth seems to happen in libxul, so the size of libxul is always of interest.

Sometimes a single change will increase APK (etc) size significantly, but more often we seem to grow slowly, and these incremental increases are hard to notice, or track.

Displaying simple build metrics in treeherder would increase our awareness of how each check-in affects product size.

As far as I am aware, the most direct way to access build sizes in today's treeherder is to select a build, then select the build link from the summary to open the ftp.mozilla.org link -- but that only shows file sizes in MB. The build log usually has more information (search for "packageSize" on Android), but who wants to do that? Surely we can do better.

Has this been considered before? I have some ideas for what this might look like, but also some uncertainty and concern about which metrics should be used, how visible they should be, and how much visual "clutter" is warranted.

Comment 1

[Richard Newman [:rnewman]]

<3

Broadly I think we're concerned with libxul.so, omni.ja, Fennec's own resources, and the APK size itself (which will basically be the former + Fennec code).

I agree that libxul itself is of interest on all platforms.

I think different developers will tend to alter different chunks of this -- a toolkit JS developer will rarely affect libxul, but could easily accidentally add 50KB to Fennec's omni.ja.

Comment 2

[William Lachance (:wlach)]

This would be easy to do if we started tracking build metrics inside perfherder (part of treeherder, see http://treeherder.mozilla.org/perf.html). I'm going to go out on a limb and say that's probably the best idea, in fact. :) Marking this as depending on that, if people want to solve this in another way, we can discuss.

Comment 3

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: simple report via TinderboxPrint

I realized this could be done very simply, if we don't get hung up on details and pretty UI: Report the size of some key files with good-old TinderboxPrint.

This doesn't work for taskcluster builds and only reports the package size for several platforms. Good enough for a "bridging" solution, while we wait for something better? I like it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=047de6657040 (click on a build, check the "Job details" pane).

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8626617 [details] [diff] [review]
simple report via TinderboxPrint

Awesome! Yeah this looks like a great interim solution. Let's leave this bug open for the perfherder implementation though.

Comment 5

[Geoff Brown [:gbrown] (pto Apr 11-18)]

https://hg.mozilla.org/build/mozharness/rev/a2868ad7a472

Comment 6

[Geoff Brown [:gbrown] (pto Apr 11-18)]

This is in use now, at least on mozilla-central. Leaving open for a more comprehensive solution via perfherder.

Comment 7

[Richard Newman [:rnewman]]

Dunno if you noticed that this is reporting libxul as being 648,116,144 bytes, which doesn't seem right to me…!

Comment 8

[Geoff Brown [:gbrown] (pto Apr 11-18)]

It is accurate...for the unstripped libxul.so.

Locally:

gbrown@mozpad:~/objdirs/droid$ ls -l ./toolkit/library/libxul.so
-rwxrwxr-x 1 gbrown gbrown 719327628 Jun 26 18:11 ./toolkit/library/libxul.so
gbrown@mozpad:~/objdirs/droid$ ls -l ./dist/fennec/assets/armeabi-v7a/libxul.so
-rw-rw-r-- 1 gbrown gbrown 22308901 Jun 26 18:14 ./dist/fennec/assets/armeabi-v7a/libxul.so

My solution tries to use generic (common to multiple platforms) build paths; I couldn't find a simple way to find the stripped libxul.so.

Comment 9

[Nathan Froyd [:froydnj]]

The usual way around this is to use the output of the |size| command instead:

[froydnj@cerebro build-mc]$ ls -l toolkit/library/libxul.so
-rwxr-xr-x 1 froydnj froydnj 723948152 Jun 29 10:41 toolkit/library/libxul.so
[froydnj@cerebro build-mc]$ size toolkit/library/libxul.so
   text	   data	    bss	    dec	    hex	filename
61589911	5876136	1030224	68496271	4152b8f	toolkit/library/libxul.so

Unstripped libxul: ~723MB; text section size: ~61MB.

Breaking out individual text/data sizes or adding them into one big number is up to you.

Comment 10

[Eric Rahm [:erahm]]

Attachment: Report stripped size of libxul.so

This is a completely untested possible patch. It uses |os.walk| to find the stripped libxul under $(OBJ_DIR)/dist/<product>/. In theory it should work for fennec, linux, and FxOS builds.

If William of Geoff want to finish it off that would be great!

Comment 11

[William Lachance (:wlach)]

Comment on attachment 8653218 [details] [diff] [review]
Report stripped size of libxul.so

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

This looks reasonable, however, I don't have time to finish this right now, as I'm busy with a bunch of perfherder stuff. Maybe :gbrown could take a look, or I might be able to circle back to this once perfherder is able to ingest such data and there's more of a use case for this (in a couple weeks hopefully).

Comment 12

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8653218 [details] [diff] [review]
Report stripped size of libxul.so

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

This needed a quick rebase since mozharness has moved in-tree, but other than that, it looks great and works fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ebf813345ea

For example:

Buildername: Android armv7 API 9 try build
script_revlink: https://hg.mozilla.org/build/mozharness/rev/production
Size of classes.dex: 3402900 bytes
Size of fennec-43.0a1.en-US.android-arm.apk: 39144782 bytes
Size of libxul.so: 23113779 bytes
Size of omni.ja: 9670009 bytes

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe0658b07b2

Comment 14

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Thanks much Eric. 

Still leaving open as in Comment 6.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfe0658b07b2

Comment 16

[William Lachance (:wlach)]

Working on getting this data into perfherder right now...

Comment 17

[William Lachance (:wlach)]

Attachment: Log Android file sizse in perfherder-compatible form

This logs a simple string like this after an Android build:

13:08:54     INFO - PERFORMANCE_DATA: {"framework": {"name": "build_metrics"}, "suites": [{"subtests": [{"name": "apk", "value": 42952270}, {"name": "omni.ja", "value": 9732537}, {"name": "classes.dex", "value": 5472784}, {"name": "libxul.so", "value": 23647403}], "name": "file sizes"}]}

Treeherder/Perfherder PR to actually use the information forthcoming...

Comment 18

[William Lachance (:wlach)]

Attachment: Add support for ingesting arbitrary performance data in perfherder + add a build_metrics performance framework for tracking

Hey Ed, could you take a look at this? It's perfherder-specific, but most of the changes are in the log parser which you're pretty familiar with. See the rest of this bug for more context.

Comment 19

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8679109 [details]
Log Android file sizse in perfherder-compatible form

Looks good - thanks.

For the curious: why the 'apk' renaming? Consistency across versions?

Comment 20

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #19)
> Comment on attachment 8679109 [details]
> Log Android file sizse in perfherder-compatible form
> 
> Looks good - thanks.
> 
> For the curious: why the 'apk' renaming? Consistency across versions?

Yes, exactly. Without the renaming, we'd have a new apk "series" in perfherder every time we released a new version of Fennec, which isn't what we want.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8679112 [details] [review]
Add support for ingesting arbitrary performance data in perfherder + add a build_metrics performance framework for tracking

overall this is great stuff- a few comments in the PR.  Do we have tests for autophone data submitted?  Does that data end up being called talosdata?  if so, then maybe we shouldn't call everything talos.

Comment 22

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #21)
> Comment on attachment 8679112 [details] [review]
> Add support for ingesting arbitrary performance data in perfherder + add a
> build_metrics performance framework for tracking
> 
> overall this is great stuff- a few comments in the PR.  Do we have tests for
> autophone data submitted?

Presently no, but we do have tests for generic performance data ingestion, which I think covers autophone.

> Does that data end up being called talosdata?  if
> so, then maybe we shouldn't call everything talos.

No, the only stuff that has talos in it:

1. The legacy Talos data adapters and parser
2. A performance "framework" fixture which we install by default

There are a bunch of talos'isms hardcoded in the frontend ui, but I don't think there's anything that should interfere with this. Hopefully we can make things more generic/flexible as we go along. :)

Comment 23

[William Lachance (:wlach)]

Attachment: Minorly updated patch to log performance data in m-c

Comment 24

[William Lachance (:wlach)]

https://bugzilla.mozilla.org/attachment.cgi?id=8680138 (comment 23) can be checked into m-c whenever, see try runs above.

I changed the log string from "PERFORMANCE_DATA" to "PERFHERDER_DATA" to make it slightly more clear what it does.

Comment 25

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/ce4b22341de0f889e2a1495ea0367ade8e4ce095
Bug 1149164 - Support ingestion of arbitrary perf data from logs

You can now specify performance data to be ingested from any job just
by adding a line containing PERFHERDER_DATA in it.

https://github.com/mozilla/treeherder/commit/2c4e935e5f1ecce7ff8b4620eebf74dddf757550
Bug 1149164 - Add a build_metrics performance framework

Comment 26

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/4c7eabd7780e901ea8e7cd59db6c746b3d54b830
Bug 1149164 - Followup to fix TalosResult line ingestion

Comment 27

[William Lachance (:wlach)]

Checked in myself, just need to wait for some results and we can mark this as done. :)

Comment 28

[William Lachance (:wlach)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db39f79d33efeeb1f0be775d9a0b513c1e5834ec
Bug 1149164 - Log Android file sizes in perfherder-compatible form;r=gbrown

Comment 29

[William Lachance (:wlach)]

Attachment: Truncate all installer names, not just APK

So it turns out we're now measuring all build artifacts, not just apk's. This is probably a good thing, but we need to do the same truncation strategy as we did with Android, otherwise we'll get a new performance series every release.

Comment 30

[William Lachance (:wlach)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df95da381e15

Comment 31

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8681515 [details] [diff] [review]
Truncate all installer names, not just APK

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

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +1610,5 @@
>              # FIXME: Remove the tinderboxprints when bug 1161249 is fixed and
>              # we're displaying perfherder data for each job automatically
>              if os.path.exists(path):
> +                if any(name.endswith(extension) for extension in ['apk', 'dmg', 'zip']):
> +                    name = name[-3] # truncate to file extension

I don't think that will work.

Did you mean name[-3:]?

Or maybe use os.path.splitext()?

Comment 32

[William Lachance (:wlach)]

Attachment: Updated patch again

Oops right. I think splitext doesn't have quite the behaviour we want for this instance, so sticking with name[-3:]

Comment 33

[William Lachance (:wlach)]

Yet another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a70ebceee678

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db39f79d33ef

Comment 35

[William Lachance (:wlach)]

Attachment: Change to summarize installer size

Sorry to keep on changing this. After sleeping on it, I think it makes more sense to use the installer size as a "summary" value for the suite. That way people can just look at that, then dig into the details as needed from perfherder compare views.

Comment 36

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8681633 [details] [diff] [review]
Change to summarize installer size

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

That sounds excellent!

Comment 37

[William Lachance (:wlach)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b520ee98535e9bfbe4a5dae90fc108e5628f6e
Bug 1149164 - Refine file size output for perfherder;r=gbrown

Comment 38

[William Lachance (:wlach)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3782aef6e34f972dd5699329e142a0ddba287a
Bug 1149164 - Fix minor logic error in previous commit;r=gbrown

Comment 39

[William Lachance (:wlach)]

Try submission for pushes above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54901484e4af

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8b520ee9853
https://hg.mozilla.org/mozilla-central/rev/1c3782aef6e3

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/db39f79d33ef
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f8b520ee9853
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1c3782aef6e3

Comment 42

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion