Closed Bug 1149164 Opened 5 years ago Closed 4 years ago

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

Categories

(Tree Management :: Perfherder, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gbrown, Assigned: wlach)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

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.
<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.
Blocks: fatfennec
OS: Linux → All
Hardware: x86_64 → All
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.
Depends on: 1156885
Component: Treeherder → Perfherder
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).
Assignee: nobody → gbrown
Attachment #8626617 - Flags: review?(wlachance)
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.
Attachment #8626617 - Flags: review?(wlachance) → review+
This is in use now, at least on mozilla-central. Leaving open for a more comprehensive solution via perfherder.
Assignee: gbrown → nobody
Dunno if you noticed that this is reporting libxul as being 648,116,144 bytes, which doesn't seem right to me…!
Flags: needinfo?(gbrown)
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.
Flags: needinfo?(gbrown)
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.
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!
Attachment #8653218 - Flags: feedback?(wlachance)
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).
Attachment #8653218 - Flags: feedback?(wlachance) → feedback+
Assignee: nobody → gbrown
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
Attachment #8653218 - Flags: review+
Thanks much Eric. 

Still leaving open as in Comment 6.
Assignee: gbrown → nobody
Keywords: leave-open
Working on getting this data into perfherder right now...
Assignee: nobody → wlachance
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...
Attachment #8679109 - Flags: review?(gbrown)
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.
Attachment #8679112 - Flags: review?(emorley)
Attachment #8679112 - Flags: feedback?(jmaher)
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?
Attachment #8679109 - Flags: review?(gbrown) → review+
(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 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.
Attachment #8679112 - Flags: feedback?(jmaher) → feedback+
(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. :)
Attachment #8679112 - Flags: review?(emorley) → review+
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.
Keywords: checkin-needed
Depends on: 1219386
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
Checked in myself, just need to wait for some results and we can mark this as done. :)
Status: NEW → ASSIGNED
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.
Attachment #8681515 - Flags: review?(gbrown)
Attachment #8681515 - Attachment is patch: true
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()?
Attachment #8681515 - Flags: review?(gbrown) → review-
Attached patch Updated patch again (obsolete) — Splinter Review
Oops right. I think splitext doesn't have quite the behaviour we want for this instance, so sticking with name[-3:]
Attachment #8681515 - Attachment is obsolete: true
Attachment #8681531 - Flags: review?(gbrown)
Attachment #8681531 - Flags: review?(gbrown) → review+
https://hg.mozilla.org/mozilla-central/rev/db39f79d33ef
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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.
Attachment #8681531 - Attachment is obsolete: true
Attachment #8681633 - Flags: review?(gbrown)
Comment on attachment 8681633 [details] [diff] [review]
Change to summarize installer size

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

That sounds excellent!
Attachment #8681633 - Flags: review?(gbrown) → review+
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.