Closed Bug 1442378 Opened 2 years ago Closed 2 years ago

record more metrics for installer sizes

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Specifically, we should be recording numbers on our Windows DLL sizes, and we should be recording omni.ja sizes for our desktop platforms.  Mac is challenging to do at the moment, so I will leave that for a different bug.
The "installer" on Windows is just a zip file, so all we need to do to
start tracking xul size is add xul.dll as an interesting file.

(Note that this is a subtest of installer_size, not a top-level test.)
Attachment #8955289 - Flags: review?(jmaher)
Just a small cleanup.
Attachment #8955290 - Flags: review?(jmaher)
The zipfile and tarfiles paths for finding interesting files in the
installer have duplicated code.  We can eliminate this duplicated code
by factoring out a function to just get the paths and sizes for files
contained in the installer.  If we need to make changes to how paths are
processed, we now only have to make the change in a single place, and we
can add other kinds of installers easily in the future.
Attachment #8955291 - Flags: review?(jmaher)
Unlike Android installers, installers for desktop versions of Firefox
contain two copies of omni.ja, and the code to check for omni.ja was
ignoring both copies.  Let's special-case omni.ja so we get better
numbers for our desktop platforms.

A linux build with this patch produces informative messages like:

INFO - Size of target.tar.bz2: 62756496 bytes
INFO - Size of browser-omni.ja: 35319285 bytes
INFO - Size of libxul.so: 121983312 bytes
INFO - Size of omni.ja: 17040751 bytes

rather than the current:

INFO - Size of target.tar.bz2: 62679568 bytes
INFO - Size of libxul.so: 121983312 bytes
Attachment #8955293 - Flags: review?(jmaher)
We shouldn't really have this problem, and shouting about it early will
make sure that we address issues like multiple instances of omni.ja,
rather than not recording information that we should have been.
Attachment #8955294 - Flags: review?(jmaher)
Considering we have other possibly growing libraries other than libxul, I think it would be better to sum up all the libraries rather than just list the libxul size.
(In reply to Mike Hommey [:glandium] from comment #6)
> Considering we have other possibly growing libraries other than libxul, I
> think it would be better to sum up all the libraries rather than just list
> the libxul size.

I'm not super-excited about doing this: most of the libraries are unlikely to change a whole lot, and we'll be raising the baseline size, thereby making any changes in libxul look less significant.  On Windows especially, we distribute a lot of extra libraries.

In any event, I vote that we move that discussion to a different bug.
Attachment #8955289 - Flags: review?(jmaher) → review+
Attachment #8955290 - Flags: review?(jmaher) → review+
Attachment #8955291 - Flags: review?(jmaher) → review+
Comment on attachment 8955293 [details] [diff] [review]
part 4 - properly record omni.ja sizes for desktop platforms

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

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +1609,5 @@
> +                        # and one for the browser proper.
> +                        if name == 'omni.ja':
> +                            containing_dir = os.path.basename(os.path.dirname(path))
> +                            if containing_dir == 'browser':
> +                                name = 'browser-omni.ja'

doesn't this only get browser-omni.ja and ignore omni.ja ?
Attachment #8955293 - Flags: review?(jmaher) → review-
Comment on attachment 8955294 [details] [diff] [review]
part 5 - make multiple occurrences of the same file fatal

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

this patch is fine, possibly there is additional cleanup that can be done as well.

::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +1618,2 @@
>                  for name in subtests:
>                      if subtests[name] is not None:

do we now need this if clause given that we don't set |subtests[name] = None| anymore?
Attachment #8955294 - Flags: review?(jmaher) → review+
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #8)
> ::: testing/mozharness/mozharness/mozilla/building/buildbase.py
> @@ +1609,5 @@
> > +                        # and one for the browser proper.
> > +                        if name == 'omni.ja':
> > +                            containing_dir = os.path.basename(os.path.dirname(path))
> > +                            if containing_dir == 'browser':
> > +                                name = 'browser-omni.ja'
> 
> doesn't this only get browser-omni.ja and ignore omni.ja ?

No, because they live in different directories.  One lives in the toplevel of the installation, and the other one lives under browser/.  See comment 4 for example output.

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #9)
> Comment on attachment 8955294 [details] [diff] [review]
> part 5 - make multiple occurrences of the same file fatal
> 
> Review of attachment 8955294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch is fine, possibly there is additional cleanup that can be done as
> well.

Yeah, glandium was suggesting on IRC that we should look at the on-disk files, rather than digging them out of the installer.  That would certainly help with the Mac situation!
> ::: testing/mozharness/mozharness/mozilla/building/buildbase.py
> @@ +1618,2 @@
> >                  for name in subtests:
> >                      if subtests[name] is not None:
> 
> do we now need this if clause given that we don't set |subtests[name] =
> None| anymore?

That's a good point.  We do not, and I will remove it.

ni? for the part 4 response.
Flags: needinfo?(jmaher)
Comment on attachment 8955293 [details] [diff] [review]
part 4 - properly record omni.ja sizes for desktop platforms

thanks for clarifying that
Flags: needinfo?(jmaher)
Attachment #8955293 - Flags: review- → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc69a330ff47
part 1 - track size of xul.dll for Windows packages; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd20be50fa7
part 2 - re-use variables in _get_package_metrics; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/a66af1896dea
part 3 - factor out common code for zipfiles vs. tarfiles; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf8762fe6d9
part 4 - properly record omni.ja sizes for desktop platforms; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/387dadb05328
part 5 - make multiple occurrences of the same file fatal; r=jmaher
Product: Core → Firefox Build System
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.