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

RESOLVED FIXED in Thunderbird 13.0

Status

Thunderbird
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Bienvenu, Assigned: standard8)

Tracking

(Blocks: 1 bug, {perf})

Trunk
Thunderbird 13.0
All
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Linux port to complete])

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 543484 [details] [diff] [review]
wip

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.
(Reporter)

Comment 1

6 years ago
Created attachment 543518 [details] [diff] [review]
patch that links on windows

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
(Reporter)

Comment 2

6 years ago
Protz, this is the patch that adds the telemetry stuff to Thunderbird that you said you could give a spin on Linux.
(Reporter)

Comment 3

6 years ago
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
(Reporter)

Comment 4

6 years ago
Created attachment 554915 [details] [diff] [review]
[Partially landed] attempt to fix linux build

linux failed w/ try server (though mac succeeded). This patch is an attempt to fix the linux bustage.
Attachment #543518 - Attachment is obsolete: true
(Reporter)

Comment 5

6 years ago
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)
Blocks: 487832
Keywords: perf
(Assignee)

Comment 6

6 years ago
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-
(Assignee)

Comment 7

6 years ago
Created attachment 561280 [details] [diff] [review]
Part 2

I've got to push this to try yet, but this should fix the additional executable renames that I was on about.
(Reporter)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 561284 [details] [diff] [review]
Part 2 v2

Well it would do if I refreshed before uploading the patch.
Attachment #561280 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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+
(Assignee)

Comment 11

6 years ago
Created attachment 561437 [details] [diff] [review]
[Partially landed] Part 2 v3

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)
(Reporter)

Comment 12

6 years ago
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+
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Comment 15

6 years ago
Created attachment 569317 [details] [diff] [review]
[checked in] Finish port for Mac

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)
(Assignee)

Comment 16

6 years ago
Created attachment 569319 [details] [diff] [review]
Linux port completion WIP

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.
(Reporter)

Comment 17

6 years ago
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+
(Assignee)

Updated

6 years ago
Attachment #561437 - Attachment description: Part 2 v3 → [Partially landed] Part 2 v3
(Assignee)

Updated

6 years ago
Attachment #554915 - Attachment description: attempt to fix linux build → [Partially landed] attempt to fix linux build
(Assignee)

Comment 18

6 years ago
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
(Assignee)

Updated

6 years ago
Whiteboard: [Linux port to complete]
Blocks: 360488
(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
Blocks: 722262
No longer blocks: 360488
Blocks: 451917
(Assignee)

Updated

5 years ago
Depends on: 727406
(Assignee)

Comment 20

5 years ago
Created attachment 598814 [details] [diff] [review]
Finish port for Linux

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
(Assignee)

Comment 22

5 years ago
Comment on attachment 598814 [details] [diff] [review]
Finish port for Linux

gozer, fancy taking a look?
Attachment #598814 - Flags: review?(mconley) → review?(gozer)
Duplicate of this bug: 730561
Attachment #598814 - Flags: review?(gozer) → review+
(Assignee)

Comment 24

5 years ago
Checked in the Linux part: http://hg.mozilla.org/comm-central/rev/8fecb303de4b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0

Comment 25

5 years ago
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.
(Assignee)

Comment 26

5 years ago
(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
Duplicate of this bug: 673899
Duplicate of this bug: 673900
(Assignee)

Updated

5 years ago
Depends on: 762621
You need to log in before you can comment on or make changes to this bug.