Closed Bug 668869 Opened 13 years ago Closed 12 years ago

port ffox work to lazily load libxul to speed up start-up perf and remove wrapper startup script

Categories

(Thunderbird :: General, defect)

All
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 13.0

People

(Reporter: Bienvenu, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [Linux port to complete])

Attachments

(4 files, 5 obsolete files)

Attached patch wip (obsolete) — Splinter Review
Thunderbird equivalent of bug 552864. Attached patch doesn't compile, but shows some of the direction. I think I need to make more changes to the mail/app Makefile.
Attached patch patch that links on windows (obsolete) — Splinter Review
haven't run this yet, but it gets a bit further in the build.

I haven't dealt with the linux shell wrapper issues; I'm trying to get windows building first.
Assignee: nobody → dbienvenu
Attachment #543484 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Protz, this is the patch that adds the telemetry stuff to Thunderbird that you said you could give a spin on Linux.
I've requested a try server build to see if mac/linux build and run with this patch - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-1f3fab21b043
linux failed w/ try server (though mac succeeded). This patch is an attempt to fix the linux bustage.
Attachment #543518 - Attachment is obsolete: true
Comment on attachment 554915 [details] [diff] [review]
[Partially landed] attempt to fix linux build

linux try server build happier with this patch
Attachment #554915 - Flags: review?(mbanner)
Keywords: perf
Comment on attachment 554915 [details] [diff] [review]
[Partially landed] attempt to fix linux build

I think, in particular, we need to also port the parts of attachment 539437 [details] [diff] [review], specifically the additional changes to browser/app/Makefile.in and the change to browser/app/macbuild/Contents/Info.plist.in - otherwise we'll still be using the wrapper script to start up.

Otherwise this looks good so far.
Attachment #554915 - Flags: review?(mbanner) → review-
Attached patch Part 2 (obsolete) — Splinter Review
I've got to push this to try yet, but this should fix the additional executable renames that I was on about.
ah, yes, I was going to pick this up again. If you get a try server build, Pascal might be a good test case for this since I believe he's seeing the startup perf issue. He said ffox used to have this issue on his linux laptop but doesn't anymore.
Attached patch Part 2 v2 (obsolete) — Splinter Review
Well it would do if I refreshed before uploading the patch.
Attachment #561280 - Attachment is obsolete: true
Comment on attachment 554915 [details] [diff] [review]
[Partially landed] attempt to fix linux build

Given my part 2 patch for this, I'm now happy with the changes here, if you're happy with my part 2 patch ;-)
Attachment #554915 - Flags: review- → review+
Minor tweak to address a couple of review nits from part 1.

This has passed try server builds, and fixes the current trunk bustage, hence I'd like to land it (I strongly suspect a subset of this is capable of fixing the bustage, something to do with the linking, but seeing as this is ready, lets land it).

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=43de3ff60b6e
Attachment #561284 - Attachment is obsolete: true
Attachment #561437 - Flags: review?(dbienvenu)
Comment on attachment 561437 [details] [diff] [review]
[Partially landed] Part 2 v3

sure, r=me, since it works on try-server...
Attachment #561437 - Flags: review?(dbienvenu) → review+
Ok, this isn't actually quite right and breaks Linux builds. Still figuring out if there's a subset we can land to fix the bustage, or if we have to fix the issue that breaks the Linux builds as well.
The combined patches didn't work fully for Linux - if you remove the start up script, then you need dependentlibs.list in your build and filling out for the extra ldap libs.

So I did a try server build that is a subset of the combined patches, which passed both try server and Linux locally, I've therefore pushed that to the tree:

http://hg.mozilla.org/comm-central/rev/11e47caa27fa

This should get the tree green again.

I'll post the rest of the combined patches tomorrow and note what needs to be done to complete this.
This completes the Mac part of the port. This won't actually buy us anything in terms of performance, but completes the port anyway. I'm also semi keen to do this so we can get the executable as just "thunderbird" rather than "thunderbird-bin" so that if people are using it direct, shortcuts etc can be updated as appropriate.

Try server builds coming here:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=24f27b50419d
Assignee: dbienvenu → mbanner
Attachment #569317 - Flags: review?(dbienvenu)
Attached patch Linux port completion WIP (obsolete) — Splinter Review
This "completes" the port of this bug, but still doesn't work as we need to hack dependentlibs.list to add our specific files. Posting here for now so it doesn't get lost.
Comment on attachment 569317 [details] [diff] [review]
[checked in] Finish port for Mac

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

probably want to post to m.d.a.t. so developers will know to debug thunderbird, not thunderbird-bin
Attachment #569317 - Flags: review?(dbienvenu) → review+
Attachment #561437 - Attachment description: Part 2 v3 → [Partially landed] Part 2 v3
Attachment #554915 - Attachment description: attempt to fix linux build → [Partially landed] attempt to fix linux build
Comment on attachment 569317 [details] [diff] [review]
[checked in] Finish port for Mac

Checked in: http://hg.mozilla.org/comm-central/rev/7582d11356ff
Attachment #569317 - Attachment description: Finish port for Mac → [checked in] Finish port for Mac
Whiteboard: [Linux port to complete]
(In reply to Mark Banner (:standard8) from comment #18)
> Checked in: http://hg.mozilla.org/comm-central/rev/7582d11356ff

Ftr, package-manifest.in part was from bug 549246.
(At least from bug 506493 comment 51 point of view.)
Depends on: 549246
Version: unspecified → Trunk
Depends on: 552864
No longer blocks: TB2SM
Depends on: 727406
Note: this needs the patch from bug 727406 applying first to make it work properly.

This is the patch that will complete this bug, it should in theory provide a bit of startup perf increase for Linux, but in any case, it'll match our code to Firefox's.

There is a try server build coming here:

http://hg.mozilla.org/try-comm-central/rev/0feda937a588

What this does:

- Sync mail/app/Makefile.in with the browser equivalent as much as possible. The diffs that remain are 1) application differences 2) the fact we still have a separate application.ini file rather than a predefined struct (I'll consider dealing with that in a follow-up), 3) diffs due to bug 726975 not having landed yet, 4) some diffs due to how we do branding.

- Sync mail/app/nsMailApp.cpp with nsBrowserApp.cpp, the diffs being the separate application.ini file, and that we don't allow xulrunner style startup on the Thunderbird exe.

Functionally:

- We drop the script for "thunderbird" and replace it with an executable the same as thunderbird-bin. We don't drop thunderbird-bin for backwards compatibility (some places thunderbird-bin is still used apparently), and the updater can't cope with symlinks.

- We drop compiling thunderbird against some libs it doesn't actually need to be compiled against, because libxul compiles against those.

- Port bug 582910 to reduce the diffs and we'll need it for 64 bit builds probably

- Drops old debug code and static build items from the splash.rc files (Firefox no longer has these)

- Drops the now obsolete MOZ_STATIC_BUILD_UNSUPPORTED flag

- Adds the LDAP libs to the dependentlibs.list file and packages the file.
Attachment #569319 - Attachment is obsolete: true
Attachment #598814 - Flags: review?(mconley)
Comment on attachment 598814 [details] [diff] [review]
Finish port for Linux

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

Hey Mark,

This looks OK to me, but I'm not sure I'm the right guy to review anything but the nsMailApp.cpp and all-thunderbird.js.  While I understand how Makefiles work, and can recognize a syntax error when I see one, a lot of our build system is voodoo to me.  I doubt I could pick up a defect if one was in there.

So I'm hesitant to give it an r+, since I can only really do an effective job on a subset of the files.

nsMailApp.cpp and all-thunderbird.js look OK to me though.

-Mike
Comment on attachment 598814 [details] [diff] [review]
Finish port for Linux

gozer, fancy taking a look?
Attachment #598814 - Flags: review?(mconley) → review?(gozer)
Attachment #598814 - Flags: review?(gozer) → review+
Checked in the Linux part: http://hg.mozilla.org/comm-central/rev/8fecb303de4b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Target Milestone: --- → Thunderbird 13.0

Is this for windows platform only or for Linux also ?

I've just tried Thunderbird 11 for Linux x86-64 - nothing changed.
(In reply to Lucas from comment #25)
> Target Milestone: --- → Thunderbird 13.0
> 
> Is this for windows platform only or for Linux also ?

This change will only affect Linux.

> I've just tried Thunderbird 11 for Linux x86-64 - nothing changed.

Correct, that is why the Target Milestone is set to 13 as you've already highlighted. Note that we cannot guarantee the size of the startup speed increase. We've not tried measuring it, but we do know that Firefox saw improvements with these changes.
OS: Windows 7 → Linux
Hardware: x86 → All
Depends on: 762621
You need to log in before you can comment on or make changes to this bug.