Last Comment Bug 659950 - Replace codesighs with something much simpler (staged package size & installer package size)
: Replace codesighs with something much simpler (staged package size & installe...
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Ted Mielczarek [:ted.mielczarek]
:
Mentors:
Depends on:
Blocks: 668216
  Show dependency treegraph
 
Reported: 2011-05-26 08:40 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2011-06-29 07:58 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implement a much simpler pair of codesize metrics: on-disk application size + installer size. (5.74 KB, patch)
2011-06-27 12:11 PDT, Ted Mielczarek [:ted.mielczarek]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2011-05-26 08:40:24 PDT
codesighs currently has a lot of issues. We don't have an implementation for Windows and the Linux implementation seems to have huge variance.

While investigating this, I realized that the numbers it's producing probably aren't what we care to measure anyway. It's summing up the total size of function + data symbols from all the binaries in dist/bin, which includes a bunch of crap we don't ship (like tests and utilities).

glandium proposed that we just sum up the on-disk file sizes of all binaries in the package staging directory after make package has been run (so that we have stripped binaries). This should be a much closer approximation of a codesize number that makes sense.

The alternative is to have someone spend a lot of time digging through the codesighs Perl + C code trying to figure out what the bugs are, implementing it for Windows, etc.

One downside is that we would lose the per-function diff reports that codesighs produces. This data probably isn't super useful with function inlining etc, and our implementation on tinderbox is pretty gross right now, since builds run in parallel, so they're just comparing vs. some arbitrary older build right now anyway. We should just drop all of that.

I'll also post this to dev.platform to see if there are any objections.
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-26 08:56:10 PDT
Why just binaries? I think that in total there are two things we want to measure:

1) download size of the installer package
2) on-disk size of the installed package

We could also break those numbers down by file, to help people understand where wins and losses are coming from, but I definitely think we ought to include libxul in the measurement.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2011-05-26 09:04:31 PDT
By "binaries" I mean "executables and shared libraries". It's true that maybe we don't even care about that, we just care about the total size of stuff we're shipping. (glandium suggested measuring installer size as well, I figured that'd be a good followup.)
Comment 4 Chris Cooper [:coop] 2011-05-26 13:47:56 PDT
Is this something better suited to Testing:General? 

I'm not sure Releng should be figuring out what the proper test is or how to run it, but we're happy to replace codesighs in automation once the new measure is ready.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2011-05-31 04:53:58 PDT
Currently the codesighs test is pretty tightly intertwined with the buildbot harness. I'll file a new RelEng bug once we figure out what we want here and get it implemented.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-06-27 12:11:40 PDT
Created attachment 542226 [details] [diff] [review]
implement a much simpler pair of codesize metrics: on-disk application size + installer size.

This patch implements comment 2, which is probably the simplest possible thing we can do that's useful. It's a really tiny Python script + a little Makefile target so we can do "make codesighs" and get the results. I'll file another bug on ripping out all the existing buildbot integration and replacing it with this. We'll have to get installer size added to the graph server as well to make that useful, but that's not as big of a deal. Then we can just `hg rm` the rest of the codesighs stuff.
Comment 7 Mike Hommey [:glandium] 2011-06-28 00:25:17 PDT
Comment on attachment 542226 [details] [diff] [review]
implement a much simpler pair of codesize metrics: on-disk application size + installer size.

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

::: toolkit/mozapps/installer/packager.mk
@@ +151,5 @@
>  RPM_CMD = \
>    echo Creating RPM && \
>    mkdir -p $(RPMBUILD_SOURCEDIR) && \
>    $(PYTHON) $(topsrcdir)/config/Preprocessor.py \
> +	-DMOZ_APP_NAME=$(MOZ_APP_NAME) \

Is that (non) change expected?

@@ +802,5 @@
>  
> +ifeq (WINNT,$(OS_TARGET))
> +CODESIGHS_PACKAGE = $(INSTALLER_PACKAGE)
> +else
> +CODESIGHS_PACKAGE = $(DIST)/$(PKG_PATH)$(PKG_BASENAME)$(PKG_SUFFIX)

$(DIST)/$(PACKAGE)
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-06-28 04:55:16 PDT
(In reply to comment #7)
> Comment on attachment 542226 [details] [diff] [review] [review]
> implement a much simpler pair of codesize metrics: on-disk application size
> + installer size.
> 
> Review of attachment 542226 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/installer/packager.mk
> @@ +151,5 @@
> >  RPM_CMD = \
> >    echo Creating RPM && \
> >    mkdir -p $(RPMBUILD_SOURCEDIR) && \
> >    $(PYTHON) $(topsrcdir)/config/Preprocessor.py \
> > +	-DMOZ_APP_NAME=$(MOZ_APP_NAME) \
> 
> Is that (non) change expected?

Yeah, emacs complained, there were leading spaces before a tab there.

> > +CODESIGHS_PACKAGE = $(DIST)/$(PKG_PATH)$(PKG_BASENAME)$(PKG_SUFFIX)
> 
> $(DIST)/$(PACKAGE)

Ah, crap, I copy&pasted this back and forth between here and the top-level Makefile, which doesn't have $(PACKAGE), and forgot to change it back.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-06-28 05:37:55 PDT
http://hg.mozilla.org/projects/build-system/rev/a3b7bdf2e5e8
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-06-29 06:46:57 PDT
http://hg.mozilla.org/mozilla-central/rev/a3b7bdf2e5e8

Will file some followups to get this actually hooked up.

Note You need to log in before you can comment on or make changes to this bug.