Closed
Bug 407459
Opened 17 years ago
Closed 16 years ago
Hook up new allocator to our build
Categories
(Core :: XPCOM, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: pavlov, Assigned: pavlov)
References
Details
(Keywords: memory-footprint, perf)
Attachments
(1 file, 9 obsolete files)
11.75 KB,
patch
|
ted
:
review+
|
Details | Diff | 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.
Comment 1•17 years ago
|
||
Michal or Stan is this something you could take a look at?
Comment 2•17 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•17 years ago
|
||
it works under 10.5 for me.
Updated•17 years ago
|
Assignee | ||
Comment 5•16 years ago
|
||
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•16 years ago
|
||
Assignee | ||
Comment 7•16 years ago
|
||
Benjamin: Can you see about hooking up building the CRT on Windows like we discussed? It would be a huge help.
Comment 8•16 years ago
|
||
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•16 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 10•16 years ago
|
||
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 11•16 years ago
|
||
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)
Comment 12•16 years ago
|
||
I didn't build all the way with this, but this lets you build from a pre-patched CRT source directory.
Comment 13•16 years ago
|
||
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)
Comment 14•16 years ago
|
||
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
Comment 15•16 years ago
|
||
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•16 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•16 years ago
|
||
Attachment #298003 -
Attachment is obsolete: true
Comment 18•16 years ago
|
||
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•16 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•16 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 21•16 years ago
|
||
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+
Comment 22•16 years ago
|
||
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
Comment 23•16 years ago
|
||
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•16 years ago
|
Flags: blocking1.9?
Comment 24•16 years ago
|
||
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.
Comment 25•16 years ago
|
||
This is everything left that I didn't check in.
Comment 26•16 years ago
|
||
Oh, and I checked in the breakpad change upstream in breakpad SVN.
Assignee | ||
Comment 27•16 years ago
|
||
Attachment #300694 -
Attachment is obsolete: true
Attachment #301349 -
Attachment is obsolete: true
Comment 28•16 years ago
|
||
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+
Updated•16 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta4
Assignee | ||
Comment 29•16 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
Closed: 16 years ago
Resolution: --- → FIXED
Comment 30•16 years ago
|
||
WIN32_CUSTOM_CRT_DIR doesn't work with spaces for example: set WIN32_CUSTOM_CRT_DIR=E:\Program Files\Minefield
Comment 31•16 years ago
|
||
Please file that as a new bug. Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•