418016 - Ts jumped ~1% when enabling jemalloc on Linux (qm-mini-ubuntu01, qm-mini-ubuntu02, qm-mini-ubuntu05)

Status: RESOLVED WONTFIX

Description

[Mats Palmgren (:mats)]

Load the URL, enter "previous 9 days", click "Graph It!"

All three Linux boxes shows ~1% increase after 2008-02-12 15:54
when jemalloc was enabled on Linux (bug 417066).

There was a substantial drop in Tp at the same time (wohoo!) so I'm
not really complaining, just wondering if there's something we can
do to improve Ts as well.  It seems that allocation patterns during
startup is quite different from later on -- is there something we
can tune in jemalloc during startup to improve Ts?

Comment 1

[Stuart Parmenter]

those numbers are so noisy.. I'm not sure there is much we can do...  I suspect some of this is just library loading overhead.  Larger chunks or maybe pre-allocating a bunch of chunks in a single mmap call might help.  Not sure it is worth it.

Comment 2

[Mats Palmgren (:mats)]

(In reply to comment #1)
> those numbers are so noisy.. 

Yeah, but if you look at the average over several days before and after,
the regression is quite clear.

> I suspect some of this is just library loading overhead. 

You mean extra time to load libjemalloc.so itself?
(1% Ts seems a bit too much for that.)
If so, can we make it static and include it in some other lib.so?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Possibly link it with libxul?

Comment 4

[Jason Evans]

It isn't clear to me what Ts is measuring (startup time?), but what I see in the talos installation I have is lots of very short invocations of Firefox.  If essentially no work is performed during these runs, then it is conceivable that jemalloc's startup code could impose a measurable increased cost.

I need more information on what this benchmark measures in order to figure out how to proceed.

Comment 5

[Benjamin Smedberg]

Yes, Ts is "time from startup to the time onload is fired on a file:// URI in the browser", as printed by http://mxr.mozilla.org/mozilla/source/tools/performance/startup/startup-test.html?force=1

It's a roughly good measure of the time it takes the browser to become usable.

Comment 6

[Mats Palmgren (:mats)]

I haven't tried it myself yet, but there is a standalone version
of those tests:
http://groups.google.com/group/mozilla.dev.performance/browse_thread/thread/2e467e080cb8bdfe/f8bf1daf71e265a8?hl=en&tvc=2

Comment 7

[Jason Evans]

I ran the following 50 times each with/without jemalloc.so linked in:

./obj_opt/dist/bin/firefox -P Scratch -no-remote
"file:///home/jasone/Desktop/startup-test.html?begin=`expr \`date "+%s"\` '*'
1000 + \`date "+%N"\` '/' 1000000`"

This resulted in the following:

x Ts_jemalloc
+ Ts_glibc
+-----------------------------------------------------------------------------+
|           +                           xx                                    |
|           +++           x          *  xx                                    |
|           +++* +  + +  xx x  x+  + * +*x *                 x    x           |
|+    ++   ++*** +x***+* xxxxx x+ *++**+** *x+xx+ + ++  * xx x    x     x*x+ x|
|           |_________|_____MA_________A______|_________|                     |
+-----------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  50           583           689         625.5         626.1     28.277235
+  50           563           685         607.5        609.54     28.090386
Difference at 95.0% confidence
        -16.56 +/- 11.1834
        -2.64494% +/- 1.7862%
        (Student's t, pooled s = 28.184)

This difference is non-trivial.  Firefox makes. ~200,000 malloc() or free()
calls during startup, so at this point I have yet to rule out any of the
following possible explanations:

1) Dynamically loading libjemalloc.so may be a non-trivial overhead.

2) jemalloc's initialization code may have non-trivial overheads, such as file
operations that determine runtime configuration and page size.

3) jemalloc may simply suffer additional per-call overheads that are lower for
glibc's malloc (such as mutex operations), though we would expect to see this
reflected in all benchmarks.

I am currently attempting to make jemalloc part of libxul.so, in order to rule
out (1).

Comment 8

[Jason Evans]

Here are results with jemalloc integrated into libxul (thanks to bsmedberg for
build system help):

x Ts_xul
+ Ts_glibc
+-----------------------------------------------------------------------------+
|            +                                                                |
|            +                  x                                             |
|            +      *x+       x xx    +                                       |
|      +    ++* *x+ *x+  x   xxxx*   ++x+*  +                 x               |
|+     +   +++*+*x* *x+x*x xxxxxx*x*****+*+ + *   * *+ +  + x x   xx       + +|
|           |______|_________MAM_A____________||                              |
+-----------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  50           584           669           611        613.76     21.973602
+  50           563           685         607.5        609.54     28.090386
No difference proven at 95.0% confidence

It looks like the majority of the jemalloc startup overhead is due to loading a
separate shared library.  The above results inidicate no statistically
significant difference remains if we merge jemalloc into libxul.

Comment 9

[Jason Evans]

Created attachment 304322 [details] [diff] [review]
Integrate jemalloc into libxul on Linux

This patch modifies the build system to integrate jemalloc into libxul on all platforms except Windows and OS X.  This should work correctly on all ELF-based systems.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there any real downside to just linking jemalloc in with libxul on all platforms? I understand that the benefits of doing so are only really apparent on Linux, but if there's no downside to doing so, it at least reduces the build system complexity.

Stuart, thoughts?

Comment 11

[Stuart Parmenter]

it only makes sense on platforms that have a linux-like dynamic loader.  mac and windows don't qualify.

Comment 12

[Jason Evans]

The patch for this bug should in my opinion be committed.

Comment 13

[Benjamin Smedberg]

Comment on attachment 304322 [details] [diff] [review]
Integrate jemalloc into libxul on Linux

Ugh, this wasn't in my review queue because [e-mail] is a watcher account that mostly gets ignored. Please use [e-mail]

Comment 14

[Reed Loden [:reed] (use needinfo?)]

Checking in browser/app/Makefile.in;
/cvsroot/mozilla/browser/app/Makefile.in,v  <--  Makefile.in
new revision: 1.152; previous revision: 1.151
done
Checking in memory/jemalloc/Makefile.in;
/cvsroot/mozilla/memory/jemalloc/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/library/Makefile.in;
/cvsroot/mozilla/toolkit/library/Makefile.in,v  <--  Makefile.in
new revision: 1.67; previous revision: 1.66
done
Checking in browser/installer/removed-files.in;
/cvsroot/mozilla/browser/installer/removed-files.in,v  <--  removed-files.in
new revision: 1.44; previous revision: 1.43
done
Checking in browser/installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--  packages-static
new revision: 1.157; previous revision: 1.156
done

Comment 15

[Robert Kaiser]

This broke the SeaMonkey Linux tinderbox, probably everyone that doesn't have jemalloc explicitely enabled.

Comment 16

[Jason Evans]

I've tested this change with and without jemalloc enabled on Linux, without any
build problems.  The tinderbox build log makes it look like
memory/jemalloc/Makefile was somehow not updated in the object directory.  The
problem is that when libxul links, it expects libjemalloc.a, but clearly
libjemalloc.so is being built instead.

In summary, I suspect the problem is a build tree that is in an inconsistent
state, rather than the patch being at fault.

Comment 17

[Reed Loden [:reed] (use needinfo?)]

(In reply to comment #15)
> This broke the SeaMonkey Linux tinderbox, probably everyone that doesn't have
> jemalloc explicitely enabled.

No, it just broke people who don't use libxul and who should now explicitly link with jemalloc. This seems to only be SeaMonkey, at least as far as what tinderbox is showing.

Checking in suite/app/Makefile.in;
/cvsroot/mozilla/suite/app/Makefile.in,v  <--  Makefile.in
new revision: 1.31; previous revision: 1.30
done

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 304322 [details] [diff] [review]
Integrate jemalloc into libxul on Linux

>+ifdef MOZ_MEMORY
>+ifneq ($(OS_ARCH),WINNT)
>+ifneq ($(OS_ARCH),Darwin)
>+EXTRA_DSO_LDOPTS += $(DEPTH)/memory/jemalloc/$(LIB_PREFIX)jemalloc.$(LIB_SUFFIX)
>+endif
>+endif
>+endif

This patch seems to have broken non-libxul builds, I think.

I get this error, while building in obj/toolkit/library:
  c++: ../../memory/jemalloc/libjemalloc.a: No such file or directory

FWIW, if I edit the generated Makefile and replace $(LIB_SUFFIX) with ".so" in the line quoted above, the build completes successfully.

Comment 19

[Daniel Holbert [:dholbert]]

For reference, I'm using this .mozconfig:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj
mk_add_options MOZ_CO_PROJECT=browser
ac_add_options --enable-application=browser
ac_add_options --enable-debug --disable-optimize
ac_add_options --disable-libxul

and I'm building on Ubuntu Hardy.

Comment 20

[Stuart Parmenter]

bsmedberg and I should have caught it.  we need the browser/app/Makefile.in changes to be conditional on --disable-libxul stuff

Comment 21

[Benjamin Smedberg]

Actually no, I don't think that's what we want... we always build a small libxul, even in non-libxul builds, so we should just add FORCE_STATIC_LIB = 1 to memory/jemalloc/Makefile.in and that should work.

Comment 22

[Daniel Holbert [:dholbert]]

Created attachment 309528 [details] [diff] [review]
followup patch for non-libxul builds

(In reply to comment #21)
> we should just add FORCE_STATIC_LIB = 1
> to memory/jemalloc/Makefile.in and that should work.

I just tested that out via this attached patch, and I can confirm that it fixes the issue for me.

Comment 23

[Stuart Parmenter]

Comment on attachment 309528 [details] [diff] [review]
followup patch for non-libxul builds

this should live down in the else for linux

Comment 24

[Rinaldi J. Montessi]

Here, using FORCE_STATIC_LIB = 1 did indeed get the libjmalloc.a to build, however the build **** out later with:  usr/lib/gcc/i486-slackware-linux/4.2.3/../../../../i486-slackware-linux/bin/ld: cannot find -ljemalloc  So, it appears using this .mozconfig:

mk_add_options MOZ_CO_PROJECT=suite
ac_add_options --enable-application=suite
ac_add_options --disable-debug
ac_add_options --enable-optimize=-O2
ac_add_options --enable-tests
ac_add_options --enable-default-toolkit=cairo-gtk2
ac_add_options --enable-canvas
ac_add_options --enable-svg
ac_add_options --enable-pango

both libjemalloc.a and libjemalloc.so are required.

Note.  Backing put the patch and re-running gmake -f client.mk build_all was sufficient for the build to complete successfully.

Comment 25

[Daniel Holbert [:dholbert]]

Created attachment 309560 [details] [diff] [review]
 followup patch for non-libxul builds (FORCE_STATIC_LIB = 1)

(In reply to comment #23)
> (From update of attachment 309528 [details] [diff] [review])
> this should live down in the else for linux

Ok, this version moves it down there.

Comment 26

[Daniel Holbert [:dholbert]]

Thanks for the r+sr -- followup patch checked in.

/cvsroot/mozilla/memory/jemalloc/Makefile.in,v  <--  Makefile.in
new revision: 1.7; previous revision: 1.6
done

Comment 27

[Robert Kaiser]

FYI, SeaMonkey went green with this additional checkin and the backout of reed's change from comment #17 - thanks for your help.

Comment 28

[Mike Hommey [:glandium]]

Statically linking libjemalloc into libxul doesn't sound like a good idea for embedding applications...

Comment 29

[Jason Evans]

Created attachment 319841 [details] [diff] [review]
Revert jemalloc/libxul integration

The attached patch reverts integration of jemalloc into libxul.  There are problems with mixed allocator use that have no reasonable solutions unless jemalloc is available as a separate library that applications directly link to.  See bug #423279 and bug #423334 for details.

Comment 30

[Reed Loden [:reed] (use needinfo?)]

Comment on attachment 319841 [details] [diff] [review]
Revert jemalloc/libxul integration

Just a straight back out of the original patch.

Comment 31

[Mike Schroepfer]

Comment on attachment 319841 [details] [diff] [review]
Revert jemalloc/libxul integration

a+ schrep based on irc discussion with Pav.

Comment 32

[Ginn Chen]

I guess the revert will break Solaris build.
I'm working on it.

Comment 33

[Ginn Chen]

(In reply to comment #32)
> I guess the revert will break Solaris build.
> I'm working on it.
> 

I've revised my patch in bug 422055 to deal with Solaris builds.

Comment 34

[Reed Loden [:reed] (use needinfo?)]

Backout patch landed:

Checking in browser/app/Makefile.in;
/cvsroot/mozilla/browser/app/Makefile.in,v  <--  Makefile.in
new revision: 1.155; previous revision: 1.154
done
Checking in memory/jemalloc/Makefile.in;
/cvsroot/mozilla/memory/jemalloc/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/library/Makefile.in;
/cvsroot/mozilla/toolkit/library/Makefile.in,v  <--  Makefile.in
new revision: 1.69; previous revision: 1.68
done

Comment 35

[Reed Loden [:reed] (use needinfo?)]

Forgot to back out the installer changes. :(

Checking in browser/installer/removed-files.in;
/cvsroot/mozilla/browser/installer/removed-files.in,v  <--  removed-files.in
new revision: 1.49; previous revision: 1.48
done
Checking in browser/installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--  packages-static
new revision: 1.161; previous revision: 1.160
done