Closed
Bug 659950
Opened 13 years ago
Closed 13 years ago
Replace codesighs with something much simpler (staged package size & installer package size)
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: ted)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(1 file)
5.74 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/751e39fa2c74621f#
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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.
Component: Release Engineering → General
Product: mozilla.org → Testing
QA Contact: release → general
Version: other → unspecified
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → ted.mielczarek
Component: General → Build Config
Product: Testing → Core
QA Contact: general → build-config
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #542226 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•13 years ago
|
Summary: Replace codesighs with something much simpler (sum of binary sizes in staged package) → Replace codesighs with something much simpler (staged package size & installer package size)
Comment 7•13 years ago
|
||
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)
Attachment #542226 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 8•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/a3b7bdf2e5e8
Whiteboard: fixed-in-bs
Assignee | ||
Comment 10•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a3b7bdf2e5e8 Will file some followups to get this actually hooked up.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•