Closed Bug 510309 Opened 12 years ago Closed 12 years ago
Link XPTs for OS X DMG packages
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.
That looks like you're in an ifndef UNIVERSAL_BINARY, which isn't really going to help on anything we ship, is it?
I have no idea. Could you point me to the relevant section?
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.
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 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.
Attachment #394609 - Flags: review?(ted.mielczarek) → review-
Attachment #394899 - Flags: review?(ted.mielczarek) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Any chance we could actually stop copy-pasting the .xpt list everywhere?
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.
You need to log in before you can comment on or make changes to this bug.