Closed Bug 407459 Opened 17 years ago Closed 16 years ago

Hook up new allocator to our build

Categories

(Core :: XPCOM, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: pavlov, Assigned: pavlov)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 9 obsolete files)

Attached patch jemalloc.c (obsolete) — Splinter Review
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.
Michal or Stan is this something you could take a look at?
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.

it works under 10.5 for me.
Keywords: footprint, perf
I'll take this bug for now.
Assignee: nobody → pavlov
Attached patch patch v1 (obsolete) — Splinter Review
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
Attached patch updated jemalloc.c (obsolete) — Splinter Review
Benjamin:  Can you see about hooking up building the CRT on Windows like we discussed?  It would be a huge help.
Attached patch Packaging stuff, rev. 1 (obsolete) — Splinter Review
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)
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)
Attached patch build hax (obsolete) — Splinter Review
I didn't build all the way with this, but this lets you build from a pre-patched CRT source directory.
Attached patch build hax, round deux (obsolete) — Splinter Review
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.
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].

--
Attached patch new jemalloc. (obsolete) — Splinter Review
Attachment #298003 - Attachment is obsolete: true
Attached patch build hax ftw (obsolete) — Splinter Review
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)
(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/".
(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.
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.
Attached patch the bits that are left (obsolete) — Splinter Review
This is everything left that I didn't check in.
Oh, and I checked in the breakpad change upstream in breakpad SVN.
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+
Blocks: 415712
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta4
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
Closed: 16 years ago
Resolution: --- → FIXED
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!
Depends on: 419127
You need to log in before you can comment on or make changes to this bug.