Closed Bug 427627 Opened 12 years ago Closed 12 years ago

[Win] enable jemalloc on thunderbird trunk

Categories

(Thunderbird :: Build Config, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: dmose, Assigned: standard8)

References

(Depends on 1 open bug)

Details

(Whiteboard: [no l10n impact])

Attachments

(4 files, 1 obsolete file)

This probably wants to wait until we have some performance and memory numbers in our tinderboxen, but this is likely to help both speed and memory usage.  A snippet of info from IRC:

[1:48pm] ted: dmose: anyway, just ac_add_options --enable-jemalloc
[1:48pm] ted: i dunno where the WIN32_CRT_REDIST env var is set (you are shipping the VC8 CRT in trunk builds, right?)
[1:49pm] ted: if you're not, then just use that configure arg
[1:49pm] ted: if you are, then find out where it's set (someplace hidden in the environment) and remove that as well
Flags: blocking-thunderbird3+
Seamonkey had a packaging problem here:
https://bugzilla.mozilla.org/show_bug.cgi?id=423703#c5
(In reply to comment #1)
> Seamonkey had a packaging problem here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=423703#c5
> 
And in bug 426513
Sure, rub it in!
As penance, here's a patch to enable it on the Thunderbird trunk Windows tinderbox, including the packaging bits I forgot for SeaMonkey.

Someone should still find out if/where WIN32_REDIST_DIR is set in the tinderbox environment and kill that, so we don't have to ship two CRTs.
Depends on: 428398
Version: unspecified → Trunk
Priority: -- → P1
Target Milestone: --- → Thunderbird 3.0b2
Depends on: 462862
Whiteboard: [need perf tests before enabling]
We're still waiting on perf tests (some progress has been made) but moving this out to b2
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
giving to Standard8 so this will have an owner, and I think you've been doing some perf test work - please feel to re-assign
Assignee: nobody → bugzilla
Summary: enable jemalloc on thunderbird trunk → [Win] enable jemalloc on thunderbird trunk
Whiteboard: [need perf tests before enabling] → [need perf tests before enabling][Have extension; need to test]
Status: NEW → ASSIGNED
Whiteboard: [need perf tests before enabling][Have extension; need to test] → [need perf tests before enabling][Have extension; need to test][no l10n impact]
Comment on attachment 314315 [details] [diff] [review]
enable jemalloc on thunderbird trunk windows

The Thunderbird machines are all buildbot-driven now, I think, so this patch will no longer do the trick. At least, not the first half. The second half should still more-or-less apply to comm-central, and you could commit such an updated patch at any time, without depending on enabling jemalloc. The packages-static file is here:
http://mxr.mozilla.org/comm-central/source/mail/installer/windows/packages-static

You'll need to add the --enable-jemalloc line here:
http://hg.mozilla.org/build/buildbot-configs/file/3054c4e7a1dc/thunderbird/win32/mozconfig
when you finally make the call to turn it on.
Attachment #314315 - Attachment is obsolete: true
This patch just adds the appropriate libs to packages-static (with an appropriate ifdef) and also adds the appropriate removals to removed-files.in.

This doesn't enable jemalloc at the moment, just prepares the build system for when we do switch it.
Attachment #360487 - Flags: review?(philringnalda)
Attached file Test Extension
Attached file Test run script
I've just attached an extension which goes with this script I'm running now. I think to get perf tests up and running before beta 2 is unlikely, therefore I'm proposing we do some manual testing.

What we need:

- Release/Nightly Builds from the same source code version, one with jemalloc enabled, the other without.
- trace-malloc builds with and without jemalloc, again from the same source code version (but not necessarily the same as the release/nightly builds).

I'm going to ask gozer if he can generate these builds once the packaging patch is in the source base.

Some performance testing on a few machines for the release/nightly builds with the extension and this script.

I can do analysis of the trace-malloc builds on my virtual machine.

To get startup numbers with the extension and the script:

a) Install extension into Thunderbird and restart (to ensure registration of components is completed).
b) Exit Thunderbird
c) Try to ensure no other active processes are running on the machine, i.e. as quiet as possible.
d) Start Thunderbird using the script (I've called it runit.py)

python runit.py thunderbird.exe

e) Wait till Thunderbird starts, there will then be an alert of how many milliseconds it took to get to the stage where it loads the mail start page - note you must have a default account loading for this to work.

f) Exit Thunderbird and repeat steps c and d (say 20 times) to get a set of values.
Whiteboard: [need perf tests before enabling][Have extension; need to test][no l10n impact] → [no l10n impact][needs review philor][getting packaging fixed, then will generate builds to test]
Attachment #360487 - Flags: review?(philringnalda) → review+
Whiteboard: [no l10n impact][needs review philor][getting packaging fixed, then will generate builds to test] → [no l10n impact][getting packaging fixed, then will generate builds to test]
You might be interested in my patch on bug 468913. The runreftest.py script there uses automation.py to run reftest in Firefox. It installs reftest as an extension in the Firefox profile.
Attachment #360487 - Attachment description: Add required jemalloc libs to packaging → [checked in] Add required jemalloc libs to packaging
Comment on attachment 360487 [details] [diff] [review]
[checked in] Add required jemalloc libs to packaging

Checked in: http://hg.mozilla.org/comm-central/rev/9412648b9d07
Whiteboard: [no l10n impact][getting packaging fixed, then will generate builds to test] → [no l10n impact][waiting to generate test builds]
Comment on attachment 360487 [details] [diff] [review]
[checked in] Add required jemalloc libs to packaging

>diff --git a/mail/installer/removed-files.in b/mail/installer/removed-files.in
> #ifdef XP_WIN
> [...]
>+#ifdef XP_WIN

An #ifdef too much, or an #endif is missing:
http://mxr.mozilla.org/comm-central/source/mail/installer/removed-files.in?mark=320-328#318
(In reply to comment #13)
> (From update of attachment 360487 [details] [diff] [review])
> >diff --git a/mail/installer/removed-files.in b/mail/installer/removed-files.in
> > #ifdef XP_WIN
> > [...]
> >+#ifdef XP_WIN
> 
> An #ifdef too much, or an #endif is missing:

Opps, thanks for catch that, I've checked in a fix: http://hg.mozilla.org/comm-central/rev/ade17a0eda6e
Whiteboard: [no l10n impact][waiting to generate test builds] → [no l10n impact][waiting for one more set of results]
I think I've got enough information here now and IMO we should go for it. Here's the results:

Summary
-------
- Approx 4% Reduction in number of allocs on basic bloat tests.
- 2-3% increase in speed.
- Slightly increased maximum heap, probably due to increase in build size.

Numbers
-------

Installer Sizes: Without: 7927141 bytes, With: 8151688 bytes (2.75% increase)

Bloat Numbers (average from 2-3 runs):

Lk: 14737 about the same
Maximum Heap: Without: 16186319 bytes, With: 16199999 bytes (<0.8% increase)
Allocs: Without: 409103, With: 407229 (4% reduction)

Startup Numbers (average over 20 warm starts):

1) Core 2 Duo 2.0GHz, 4GB ram, Windows 7 beta, 64-bit
 Without: 1715.75ms, With: 1679.75ms

2) Pentium Dual Core, 1.73GHz, 1GB ram, Windows Vista Home 32-bit
 Without: 1238.2ms, With: 1200.9ms

2 to 3% increase in speed.

These speed increases were also reflected on my virtual machine, however due to the nature of virtual machines, I'm not including them as part of the results.

Source versions:

http://hg.mozilla.org/comm-central/rev/223c017c5a3d
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b746930b1c17
Attachment #361484 - Flags: review?(dmose)
Whiteboard: [no l10n impact][waiting for one more set of results] → [no l10n impact][needs review dmose]
Comment on attachment 361484 [details] [diff] [review]
[checked in] Enable jemalloc on tinderboxes

Those numbers look good; nice work!  r=dmose
Attachment #361484 - Flags: review?(dmose) → review+
Whiteboard: [no l10n impact][needs review dmose] → [no l10n impact][needs landing]
Whiteboard: [no l10n impact][needs landing] → [no l10n impact][needs landing, wed morning PST]
Just an update: I've asked gozer to land this as we should clobber the Windows boxes when we land it because of the different backend code.
Whiteboard: [no l10n impact][needs landing, wed morning PST] → [no l10n impact][gozer to land]
Puushed mozconfig change to unittest builder

http://hg.mozilla.org/build/buildbot-configs/rev/941

changeset:   941:26117b3fab61
tag:         tip
user:        Philippe M. Chiasson <gozer@mozillamessaging.com>
date:        Fri Feb 13 12:47:35 2009 -0500
summary:     Bug 427627. enable jemalloc on win32. p=Standard8 a=dmose
Pushed mozconfig changes to the rest of the builders

http://hg.mozilla.org/build/buildbot-configs/rev/942

changeset:   942:c6187a849daa
tag:         tip
user:        Philippe M. Chiasson <gozer@mozillamessaging.com>
date:        Fri Feb 13 16:03:18 2009 -0500
summary:     Bug 427627. enable jemalloc on win32. p=Standard8 a=dmose
Whiteboard: [no l10n impact][gozer to land] → [no l10n impact]
Attachment #361484 - Attachment description: Enable jemalloc on tinderboxes → [checked in] Enable jemalloc on tinderboxes
After complete clobber builds, the tree is still GREEN. Looks like it worked.
Thanks to gozer for landing this for me.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.