Closed Bug 510309 Opened 10 years ago Closed 10 years ago

Link XPTs for OS X DMG packages


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: adw, Assigned: adw)



(Whiteboard: [ts])


(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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/ -m '\.xpt$'
    0.33 /Users/adw/startup/dist/08-10/                 close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.33 /Users/adw/startup/dist/08-10/                         close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.35 /Users/adw/startup/dist/08-10/                    close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.35 /Users/adw/startup/dist/08-10/                close_nocancel,lstat,open_nocancel,read_nocancel,stat
    0.35 /Users/adw/startup/dist/08-10/                close_nocancel,lstat,open_nocancel,read_nocancel,stat
--- snip ---
   14.96 /Users/adw/startup/dist/08-10/                      close_nocancel,lstat,open_nocancel,read_nocancel,stat
   15.20 /Users/adw/startup/dist/08-10/                         close_nocancel,lstat,open_nocancel,read_nocancel,stat
   16.11 /Users/adw/startup/dist/08-10/                    close_nocancel,lstat,open_nocancel,read_nocancel,stat
   17.19 /Users/adw/startup/dist/08-10/                         close_nocancel,lstat,open_nocancel,read_nocancel,stat
   22.92 /Users/adw/startup/dist/08-10/                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/ -m '\.xpt$' -d '*'
   10.65 /Users/adw/startup/dist/08-11/          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
Attachment #394346 - Flags: review?(ted.mielczarek)
Whiteboard: [ts]
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/ 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/ (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 is used for OS X) having both the old interface in foo.xpt, and the new interface in firefox.xpt.
Attached patch patch v2 (obsolete) — Splinter Review
Thanks, Phil.  This adds the current XPTs to browser/installer/ 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.
Attachment #394346 - Attachment is obsolete: true
Attachment #394609 - Flags: review?(ted.mielczarek)
Attachment #394346 - Flags: review?(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.
Attachment #394609 - Flags: review?(ted.mielczarek) → review-
Attached patch patch v3Splinter Review
Attachment #394609 - Attachment is obsolete: true
Attachment #394899 - Flags: review?(ted.mielczarek)
Attachment #394899 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 511721
Depends on: 511851
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.
Blocks: 441379
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.