Hook up new allocator to our build

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
XPCOM
P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Stuart Parmenter, Assigned: Stuart Parmenter)

Tracking

({footprint, perf})

Trunk
mozilla1.9beta4
x86
Windows XP
footprint, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 292180 [details] [diff] [review]
jemalloc.c

I'm not sure what the best way to hook up a custom allocator to replace malloc, realloc, calloc, new, new[], free, delete, delete[] for our application and its libraries is.  Perhaps we can take a look at what trace-malloc does?

While I'm not quite ready to call jemalloc the winner, the work to hook up one allocator should be mostly the same as hooking up others -- it would be nice if someone could take a stab at hooking it up to our builds.

Here is my modified (ported to mac/linux from freebsd (actually, if you want this to run on linux you'll need to change the spinlock code to use pthreads (most of it is commented out in the file)) jemalloc.c.  You also need tree.h which can be found at http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/sys/tree.h.  we should clean up my changes if we decide to stick with jemalloc and submit them upstream as well as port it to windows.

Comment 1

10 years ago
Michal or Stan is this something you could take a look at?

Comment 2

10 years ago
Did this code actually work on Macs? brk()/sbrk() aren't really supported, and the allocation scheme has gone through a couple rewrites (media support is a powerful incentive). I'd have to research more to know what can be reliably changed. Trace-malloc doesn't work on Mac either, apparently.

(Assignee)

Comment 3

10 years ago
it works under 10.5 for me.

Updated

10 years ago
Keywords: footprint, perf
(Assignee)

Comment 4

10 years ago
I'll take this bug for now.
Assignee: nobody → pavlov
(Assignee)

Comment 5

9 years ago
Created attachment 297245 [details] [diff] [review]
patch v1

Current version of the patch.  Should build without changes on mac (tested) and linux (untested).  Will build on Windows as well, but needs to be built as part of a CRT replacement and not in-tree as mac and linux are.
Attachment #292180 - Attachment is obsolete: true
(Assignee)

Comment 6

9 years ago
Created attachment 298003 [details] [diff] [review]
updated jemalloc.c
(Assignee)

Comment 7

9 years ago
Benjamin:  Can you see about hooking up building the CRT on Windows like we discussed?  It would be a huge help.
Created attachment 299231 [details] [diff] [review]
Packaging stuff, rev. 1

To hook up the CRT on Windows:

* Build the CRT+jemalloc from sources (Stuart has them)
* Launch start-msvc8.bat in mozillabuild
* export LIB="c:/path/to/custom-crt/build/intel;$LIB"
* export WIN32_REDIST_DIR=/c/path/to/custom-crt/build/intel
* export WIN32_CUSTOM_CRT=1
* You could probably even set the above environment stuff from tinder-config.pl
* Use the patch I'm attaching (we should just land this... it shouldn't break anything right now)
* build normally

Note that this is an untested educated guess
Attachment #299231 - Flags: review?(ted.mielczarek)
Attachment #299231 - Flags: review?(pavlov)
(Assignee)

Comment 9

9 years ago
Comment on attachment 299231 [details] [diff] [review]
Packaging stuff, rev. 1

Every time I've tried to build using LIB= several test apps crash as well as xpidl.  It seems to happen every time.
Comment on attachment 299231 [details] [diff] [review]
Packaging stuff, rev. 1

Are we always going to be building the custom CRT with that name?

r=me, pretty trivial
Attachment #299231 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 299231 [details] [diff] [review]
Packaging stuff, rev. 1

I don't think we need this with my new patch, since we're just going to build the DLL, we can install it to dist/bin.  We will need the packages-static bit though.
Attachment #299231 - Attachment is obsolete: true
Attachment #299231 - Flags: review?(pavlov)
Created attachment 300235 [details] [diff] [review]
build hax

I didn't build all the way with this, but this lets you build from a pre-patched CRT source directory.
Created attachment 300272 [details] [diff] [review]
build hax, round deux

This is still not great, but it now supports pointing at a pre-built CRT.  You can set WIN32_CUSTOM_CRT_DIR to the dir containing the dll+lib.  The DLL must be named nscrt19.dll.
Attachment #300235 - Attachment is obsolete: true
Attachment #300272 - Flags: review?(benjamin)
My build breaks in security/coreconf with this patch.  I wonder if my NSS build is breaking in some way.

make[3]: Entering directory `/c/obj-firefox/security/manager'
/local/bin/make -C /z/mozilla/security/coreconf MAKE="/local/bin/make -j1" -j1 CC="cl" SOURCE_MD_DIR=c:/obj-firefox/dist DIST=c:/obj-firefox/dist NSPR_INCLUDE_DIR=c:/obj-firefox/dist/include/nspr NSPR_LIB_DIR=c:/obj-firefox/dist/lib MOZILLA_CLIENT=1 NO_MDUPDATE=1 NSS_ENABLE_ECC=1 BUILD_TREE=c:/obj-firefox BUILD_OPT=1 OPT_CODE_SIZE=1 NS_USE_GCC= NS_USE_NATIVE=1 OS_TARGET=WIN95 clean
make[4]: Entering directory `/z/mozilla/security/coreconf'
make[4]: *** No rule to make target `clean'.  Stop.
make[4]: Leaving directory `/z/mozilla/security/coreconf'
make[3]: *** [.nss.cleaned] Error 2
make[3]: Leaving directory `/c/obj-firefox/security/manager'
make[2]: *** [export_tier_toolkit] Error 2
I'm an idiot, nevermind me.  I accidentally deleted the Makefiles in NSS after I accidentally configured in my srcdir.  Gonna try this again with an unscrewed NSS.

Comment 16

9 years ago
Please make sure to add the following comment to the diff file prior to checkin:

---
The Microsoft C Runtime source code to which this document refers is available directly from Microsoft Corporation, under a separate license.
Please ensure that if you are using that source code, you have appropriate rights to use it.  By providing you access to this file, Mozilla Corporation and its affiliates do not purport to grant any rights in that source code.  Binaries are available under separate licenses at [fill in an appropriate reference].

--
(Assignee)

Comment 17

9 years ago
Created attachment 300571 [details] [diff] [review]
new jemalloc.
Attachment #298003 - Attachment is obsolete: true
Created attachment 300694 [details] [diff] [review]
build hax ftw

Ok, this has the new jemalloc above, in mozilla/jemalloc, with fancy patching etc.  I think I forgot to add the license header schrep wanted to the CRT diff, but we can do that.
Attachment #297245 - Attachment is obsolete: true
Attachment #300272 - Attachment is obsolete: true
Attachment #300571 - Attachment is obsolete: true
Attachment #300694 - Flags: review?(benjamin)
Attachment #300272 - Flags: review?(benjamin)

Comment 19

9 years ago
(In reply to comment #18)
> Created an attachment (id=300694) [details]
> build hax ftw
> 
> Ok, this has the new jemalloc above, in mozilla/jemalloc, with fancy patching
> etc.  I think I forgot to add the license header schrep wanted to the CRT diff,
> but we can do that.

There are two places in the patch where "memory/" is used instead of "jemalloc/".

Comment 20

9 years ago
(In reply to comment #18)
> Ok, this has the new jemalloc above, in mozilla/jemalloc, with fancy patching
> etc.

There are two places in the patch where "memory/" is used instead of "jemalloc/".
Comment on attachment 300694 [details] [diff] [review]
build hax ftw

You seem to have some confusion about mozilla/memory versus mozilla/jemalloc. I'd prefer either mozilla/memory directly or mozilla/memory/jemalloc

You're going to need the change to package-static from my patch.

I suppose the change to google-breakpad crept in by accident?
Attachment #300694 - Flags: review?(benjamin) → review+
Pav asked where to put jemalloc in the tree, and I recited our mixed experience with container top-levels such as mozilla/db, mozilla/gc, and arguably mozilla/gfx, and favored the most concrete approach: mozilla/jemalloc.

I don't think we should lose the jemalloc name. But we could certainly try for better container karma next time, with mozilla/memory/jemalloc. Perhaps the trace-malloc library and about:memory impl could go there. Anyway, you have my blessing on mozilla/memory/jemalloc.

/be
I was going to comment, but Brendan said it all.

The google-breakpad change was actually necessary to build with the custom CRT, vswprintf is broken there, but vswnprintf works.  I'll get that landed upstream, they're exactly the same afaict.

Updated

9 years ago
Flags: blocking1.9?
Comment on attachment 300694 [details] [diff] [review]
build hax ftw

I checked in the memory/jemalloc dir and the client.mk change with a=beltzner.  Shouldn't affect anything, but will make it easier to test.  I'll post a new patch with the rest of the changes in a few minutes.
Created attachment 301349 [details] [diff] [review]
the bits that are left

This is everything left that I didn't check in.
Oh, and I checked in the breakpad change upstream in breakpad SVN.
(Assignee)

Comment 27

9 years ago
Created attachment 301435 [details] [diff] [review]
final bits go go go.
Attachment #300694 - Attachment is obsolete: true
Attachment #301349 - Attachment is obsolete: true
Comment on attachment 301435 [details] [diff] [review]
final bits go go go.

>+	#XXX: these don't link right for some reason
>+	rm $(CRT_OBJ_DIR)/build/intel/{libcmt,libcpmt}.lib

Can you file a followup on sorting this out and put the bug # in that comment?
Attachment #301435 - Flags: review+
(Assignee)

Updated

9 years ago
Blocks: 415712
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Comment 29

9 years ago
ted: I filed 415712 on the linkage issues.

Since the new allocator is hooked up and turned on on Windows and builds with --enable-jemalloc on Linux and Mac I'm going to close this bug out.  I'll open new bugs to turn on by default on other platforms as data comes in backing up those decisions.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Blocks: 415918
Blocks: 415928
Blocks: 416117

Comment 30

9 years ago
WIN32_CUSTOM_CRT_DIR doesn't work with spaces
for example:
set WIN32_CUSTOM_CRT_DIR=E:\Program Files\Minefield
Please file that as a new bug.  Thanks!

Updated

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