510309 - Link XPTs for OS X DMG packages

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

Spun out of bug 463605 comment 8.  This is a quick Ts win for OS X packages.  Instead of blocking on bug 463605, Ted says maybe we can take this patch.

XPT linking is done for all platforms except OS X, and from what I understand its purpose is perf.  Some DTracing bears that out.  I wrote a script that captures time spent in I/O syscalls during startup.  With a single linked XPT the time spent during these calls during cold startup drops more than an order of magnitude, from nearly 300ms to 10ms.  A caveat is that I did not see a conclusive matching drop in wall clock time, and I'm not sure yet why.

Here's some data that shows time spent on each XPT during cold startup and the associated syscalls.  Numbers are durations in ms.  (Hope this isn't too hard to read in bugzilla!)

Without XPT linking:

adw@tinkerbell:~/startup/dtrace$ ./io_syscalls.rb data/08-11/Minefield.app-package-01 -m '\.xpt$'
    0.33 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/xpcom_system.xpt                 close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.33 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/pref.xpt                         close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.35 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/dom_range.xpt                    close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.35 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/xpcom_threads.xpt                close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.35 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/dom_traversal.xpt                close_nocancel,lstat,open_nocancel,read_nocancel,stat
--- snip ---
   14.96 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/satchel.xpt                      close_nocancel,lstat,open_nocancel,read_nocancel,stat
   15.20 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/fuel.xpt                         close_nocancel,lstat,open_nocancel,read_nocancel,stat
   16.11 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/downloads.xpt                    close_nocancel,lstat,open_nocancel,read_nocancel,stat
   17.19 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/intl.xpt                         close_nocancel,lstat,open_nocancel,read_nocancel,stat
   22.92 /Users/adw/startup/dist/08-10/Minefield.app/Contents/MacOS/components/toolkitsearch.xpt                close_nocancel,lstat,open_nocancel,read_nocancel,stat
TOTAL: 295.557289

With XPT linking (durations of each syscall invocation also shown):

adw@tinkerbell:~/startup/dtrace$ ./io_syscalls.rb data/08-11/Minefield_firefox.xpt.app-package-01 -m '\.xpt$' -d '*'
   10.65 /Users/adw/startup/dist/08-11/Minefield_firefox.xpt.app/Contents/MacOS/components/firefox.xpt          close_nocancel,lstat,open_nocancel,read_nocancel,stat
         |     0.01 lstat
         |     0.02 close_nocancel
         |     0.03 stat
         |     0.03 open_nocancel
         |    10.56 read_nocancel
TOTAL: 10.648244

This is part of Ts work I'm logging at https://wiki.mozilla.org/Firefox/Projects/Startup_Time_Improvements/adw_notes.

Comment 1

[Phil Ringnalda (:philor)]

That looks like you're in an ifndef UNIVERSAL_BINARY, which isn't really going to help on anything we ship, is it?

Comment 2

[Drew Willcoxon :adw]

I have no idea.  Could you point me to the relevant section?

Comment 3

[Phil Ringnalda (:philor)]

Oh, sorry, I got confused (yet again) by the unintuitive way that when you are actually doing a universal binary, we wind up in stage-package twice with UNIVERSAL_BINARY undefined and thus make it past that ifndef UNIVERSAL_BINARY and to your added line, because build/macosx/universal/flight.mk undefines it and calls stage-package for each architecture in postflight_all, before the actual unification.

But, your patch does need to add all the individual xpt files we're currently shipping to browser/installer/removed-files.in (and, less fun but less your responsibility, all the comm-central apps will have to add them, in #ifndef MOZILLA_1_9_1_BRANCH, too), so that after someone changes an interface, we don't have users who've used the updater (the only time removed-files.in is used for OS X) having both the old interface in foo.xpt, and the new interface in firefox.xpt.

Comment 4

[Drew Willcoxon :adw]

Attachment: patch v2

Thanks, Phil.  This adds the current XPTs to browser/installer/removed-files.in per comment 3.  There are a couple of #ifdef XP_MACOSX sections; the second looked more general, so I appended them to the end of it.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 394609 [details] [diff] [review]
patch v2

I'd prefer if you kept this consistent with the other block below, and linked into $(DIST)/xpt.

Comment 6

[Drew Willcoxon :adw]

Attachment: patch v3

Comment 7

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/fbb95b7cfc91

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Any chance we could actually stop copy-pasting the .xpt list everywhere?

Comment 9

[(not currently active) Ted Mielczarek]

Olli: you mean adding .xpt files to the packaging manifests? We could change the packaging to simply link every xpt file in the components/ dir. The downside to that would be that any interfaces that are used simply for testing (like nsIHttpServer, for example) would wind up being shipped with the product. I'm not sure if that's a big deal or not. I've also filed bug 511642, which I intend to fix after bug 463605, so that all platforms could share a common packaging manifest, meaning you wouldn't have to separately edit browser/installer/unix/packages-static, browser/installer/windows/packages-static.