478044 - Enable jemalloc for Windows Mobile builds

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Windows Mobile builds are using VS9 ARM compiler and the Windows Mobile 6.1 SDK

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch w/debugging cruft

this allows us to build and run the "alloc_toolkit" tests.  When building Fennec for windows ce the build fails when linking xr-stub because free is defined twice (jemalloc and coredll)

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP v.2

this allows us to build, link and "run".  Fennec crashes when freeing memory pointing to "\bin\xulrunner", which is a converted commandline argument.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP v.3

this gets further.  I think there is something wrong the implementation of operator new.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP v.4

This patch works.  I can load planet.m.o with a debug, non-opt build.  It needs a lot of clean up, but if stuart and vlad could take a look for places I've gone off in the wrong direction, I'd appreciate it.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: cleaned up patch

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

This patch applies over the patch on bug 476201.  Since I see no way to build the shunt conditional on MOZ_MEMORY other than by getting rid of the vc project, I'm marking this as dependent on it.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: cleaned up patch v.2

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

I've run this through the try server a few times.  The win32 builder fails, but I can't find an error in the log other than makefile.sub not being found (which is also in the logs of successful builds). I don't have visual studio 2005, so I can't reproduce locally.  Any suggestions/help would be appreciated.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1235854368.1235859298.25814.gz&fulltext=1

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: cleaned up patch v.3

fixed chunk_alloc_map to loop if unsuccessful allocating memory

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: just jemalloc changes

Comment 11

[Stuart Parmenter]

Comment on attachment 365355 [details] [diff] [review]
just jemalloc changes

there are a lot of random ifdef WINCE that are out of place, and should be MOZ_MEMORY_WINCE.  

I suggest that you implement a getenv function that returns null rather than ifdefing it out down below

for CHUNK_2POW_DEFAULT	, it might be better to use malloc_options -- be curious to see what jason thinks.

you really don't want to use printf for wrtmessage, since it will malloc.  i'd either not implement the function or use write as it does there.

if you need to implement abort, please move it up to with the other windows specific functions (ffs, etc).  Also check your indentation and style (empty line at the beginning of the function if no variables).  same with errno and friends.

given the number of things that are MOZ_MEMORY_WINDOWS || MOZ_MEMORY_WINCE, it might make sense to #define MOZ_MEMORY_WINDOWS for wince and && !wince the few exceptions

i still think that the chunk_alloc_mmap stuff needs to be made more generic, so that function doesn't have a bunch of direct wince calls in the middle of it.  i don't see any reason why we can't do what you're doing there on windows itself.

you've got some scattered printfs and debug code that should be removed

r? from jason for additional comments

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

a couple comments/explanations:

vsnprintf seems to allocate on windows ce.  I've punted on malloc_printf, not sure if there is a way to make that work other than implementing it from scratch (and not sure that's worth it)

VirtualFree is interesting.  The documentation says you should pass 0 as the size if releasing the memory (MEM_RELEASE).  If you do that, a subsequent call to VirtualAlloc with that address will fail.  If you pass in the size, VirtualFree returns an error code....but the subsequent call to VirtualAlloc will succeed.  This patch passes in the size and ignores the error code that gets returned.

Another interesting point is that VirtualQuery shows no change to the state of the memory after VirtualFree regardless of the size passed in.

Comment 14

[Stuart Parmenter]

Comment on attachment 367834 [details] [diff] [review]
patch v.2

i think this is mostly fine.  Jason, can you take a look?

Comment 15

[Jason Evans]

Comment on attachment 367834 [details] [diff] [review]
patch v.2

The patch looks fine for the most part, but I'm concerned that there might be something fundamentally wrong about how the page mapping (VirtualAlloc() and pages_map() code) is working.  There is a change in chunk_alloc_mmap() that apparently handles non-chunk-aligned results from pages_map(), but that should never happen -- pages_map() should return NULL instead of returning mapped memory at the wrong address.  I can't say for sure that there's something wrong here, but it doesn't feel right...

Minor cruft: The char addr_buf[12] and char size_buf[12] variables do not appear to be used.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #15)
> (From update of attachment 367834 [details] [diff] [review])
> The patch looks fine for the most part, but I'm concerned that there might be
> something fundamentally wrong about how the page mapping (VirtualAlloc() and
> pages_map() code) is working.  There is a change in chunk_alloc_mmap() that
> apparently handles non-chunk-aligned results from pages_map(), but that should
> never happen -- pages_map() should return NULL instead of returning mapped
> memory at the wrong address.  I can't say for sure that there's something wrong
> here, but it doesn't feel right...

Are you referring to this change?

-		while (ret == NULL) {
+		while (ret == NULL  || CHUNK_ADDR2BASE(ret) != ret) {
 
That was a condition I was hitting, but it was a bug in my code which I fixed.  I figured it was still a good sanity check though.  If you don't like it, I'll remove it.
 
> Minor cruft: The char addr_buf[12] and char size_buf[12] variables do not
> appear to be used.

yes, thank you for catching that, I'll get rid of it before pushing.  I was using itoa to do some debugging output since the printf stuff doesn't work with wince.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/20014cbebfb6

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: extra assertion

I'm seeing some odd behavior in debug builds, adding this assertion corrects it.  I suspect this is a compiler issue.

Also, I've confirmed that this is not an issue with release builds.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: if ret != requested address, free ret and return null

this approach is less hocus pocus

Comment 20

[Doug Turner (:dougt)]

guys, without the change brad mentioned:

-        while (ret == NULL) {
+        while (ret == NULL  || CHUNK_ADDR2BASE(ret) != ret) {


I crash on startup every time.  I am reopening this bug.  Jason/stuart/brad, any idea why we need this bandaide?

Comment 21

[Doug Turner (:dougt)]

blassey, on wince we have a pattern that reserves then frees, etc.  without committing.  This is different than on windows where each call to virtualalloc is not only a reserve but a commit.  If some other caller (another thread even) is also calling VirtualAlloc, cause problems?

For example:

Thread 1: ret = VirtualAlloc(null, size);
Thread 1: ret = VirtualAlloc(ret, size);
Thread 1: VirtualFree(ret)
Thread 2: ret = VirtualAlloc(null, size);
Thread 2: ret = VirtualAlloc(ret, size);
Thread 1: ret = VirtualAlloc(ret, size);  // bad?

is there some magic locking that i didn't see?  maybe the crt patches for w32 provide this?


Also, just inspecting the code here and looking for deltas between w32 and wce:

Would it make sense to make a COMMIT call right after the call to reserve:
http://mxr.mozilla.org/mozilla-central/source/memory/jemalloc/jemalloc.c#2170

Doing this would also let you get rid of this explict call:
http://mxr.mozilla.org/mozilla-central/source/memory/jemalloc/jemalloc.c#2506

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: reserves, but doesn't commit unaligned addersses

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.4

first attempt to reserve size+offset, giving us an aligned area to commit and leaving us aligned for the next reservation.  Then fallback to reserving size+chunksize, which is guaranteed to give us an aligned area to commit.

in pages_unmap, first try to VirtualFree the address that's passed in.  In the case of error code 87 (invalid parameter), which is what is returned when you try to release an address which isn't the base of a reserved region, use VirtualQuery to get the base address and release that.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.5

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.6

Comment 26

[Stuart Parmenter]

Comment on attachment 370143 [details] [diff] [review]
patch v.6

I think this looks OK, and will make things a lot better than they are now.

My only real comment is you might want to swap the pfd and alignment params to  pages_map_align() to be consistent with the rest of the code.


Jason can you also take a look at this?  After digging for a while, the problem we have on Windows Mobile is that we can't VirtualAlloc at a specific address, so if passing NULL in as the address doesn't work, we can't VirtualFree it and try remapping in that same spot + alignment offset.  With this code in Brad's testing we're ending up aligning correctly most of the time after a while.  Unfortunately, if we have to over-reserve, we don't give back the start of the block that we need to VirtualFree it with :/.  We added some hacky code to our pages_unmap code to VirtualQuery to address if VirtualFree fails to find the base address and try freeing that.  I'm not super happy with this, but not sure what else we can do short of keeping the real address somewhere and refactoring a lot of code.  The perf hit here should be minimal and is windows mobile only, so maybe not the end of the world?

The other piece I'm not sure about is:
+#ifdef MOZ_MEMORY_WINCE
+		ret = pages_map_align(chunk_size, pfd, alignment);
+#else

Should that just be a JEMALLOC_USES_MAP_ALIGN?

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

Stuart, you asked to get a log of our aligned vs. unaligned alloc's.  I ran the page load test from talos (3 pages, 5 times). 

1: aligned, over-alloc
2: aligned
3: unaligned, over-alloc
4: pages_map

Every pages_map_align starts with a pages_map, which is why the patter is 4 then 2.  None of the allocs in the run were over-allocated.

*******alloc******* 4
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2
*******alloc******* 4
*******alloc******* 2

Comment 28

[Jason Evans]

Comment on attachment 370143 [details] [diff] [review]
patch v.6

> static void *
> pages_map(void *addr, size_t size, int pfd)
> {
>-	void *ret;
>+	void *ret = NULL;
> #if defined(MOZ_MEMORY_WINCE)
>-	ret = VirtualAlloc(addr, size, MEM_RESERVE, PAGE_NOACCESS);
>+	void *va_ret;
>+	assert(addr != NULL);
>+	va_ret = VirtualAlloc(addr, size, MEM_RESERVE, PAGE_NOACCESS);
>+	if (va_ret)
>+		ret = VirtualAlloc(ret, size, MEM_COMMIT, PAGE_READWRITE);
                                   ^^^ va_ret ?
                                   ^^^^^^^^^^^^
>+	assert(va_ret == ret);
> #elif defined(MOZ_MEMORY_WINDOWS)
> 	ret = VirtualAlloc(addr, size, MEM_COMMIT | MEM_RESERVE,
> 	    PAGE_READWRITE);
> #endif
> 	return (ret);
> }

-----------------------------------------------------------------------------

> 	do {
>+#ifdef MOZ_MEMORY_WINCE
>+		ret = pages_map_align(chunk_size, pfd, alignment);
>+#else

This should go just outside the do..while loop, since as written it will cause an infinite loop if pages_map_align() fails.

-----------------------------------------------------------------------------

> [comment #26] Should that just be a JEMALLOC_USES_MAP_ALIGN?

Yes, it looks to me like that would be an improvement.

Overall, this patch looks good to me.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #28)

> >+	assert(addr != NULL);
fwiw, this should be addr == NULL.  Fixed locally.  Also, the adjustment of size came after the pages_map, fixed that as well.

> >+	va_ret = VirtualAlloc(addr, size, MEM_RESERVE, PAGE_NOACCESS);
> >+	if (va_ret)
> >+		ret = VirtualAlloc(ret, size, MEM_COMMIT, PAGE_READWRITE);
>                                    ^^^ va_ret ?
>                                    ^^^^^^^^^^^^
yes, fixed locally as well.

> -----------------------------------------------------------------------------
> 
> > 	do {
> >+#ifdef MOZ_MEMORY_WINCE
> >+		ret = pages_map_align(chunk_size, pfd, alignment);
> >+#else
> 
> This should go just outside the do..while loop, since as written it will cause
> an infinite loop if pages_map_align() fails.
> 
done
> -----------------------------------------------------------------------------
> 
> > [comment #26] Should that just be a JEMALLOC_USES_MAP_ALIGN?
> 
> Yes, it looks to me like that would be an improvement.
done

pushed http://hg.mozilla.org/mozilla-central/rev/865b0dbcf1a3

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch

Brad, I think VirtualFree with MEM_DECOMMIT should be called before invoking VirtualFree with MEM_RELEASE in pages_unmap since VirtualAlloc with MEM_COMMIT is invoked in pages_map.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 370534 [details] [diff] [review]
A patch

From the VirtualFree documentation, in the section describing MEM_RELEASE:

"The function does not fail if you attempt to release pages that are in different states, some reserved and some committed. This means that you can release a range of pages without first determining the current commitment state."

Comment 32

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #31)
> (From update of attachment 370534 [details] [diff] [review])
> From the VirtualFree documentation, in the section describing MEM_RELEASE:
> 
> "The function does not fail if you attempt to release pages that are in
> different states, some reserved and some committed. This means that you can
> release a range of pages without first determining the current commitment
> state."

I guess the document you mention is not for Windows Mobile.

I think VirtualFree document for Windows Mobile is http://msdn.microsoft.com/en-us/library/aa908947.aspx.

Comment 33

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #32)
> I guess the document you mention is not for Windows Mobile.
> 
> I think VirtualFree document for Windows Mobile is
> http://msdn.microsoft.com/en-us/library/aa908947.aspx.

Yes, I saw that before and tested to confirm since the docs have been so wrong in so many places.  VirtualFree is not returning an error code when freeing a range of memory with mixed states.  Also, after being freed that memory does get returned in subsequent calls to VirtualAlloc.  

If we were going to program to the docs (which is entirely reasonable), I would put the VirtualFree(addr, size, MEM_DECOMMIT) just before the VirtualQuery because there is a cost associated with these API calls and that is the only place where we should encounter ranges of memory of mixed states.

Comment 34

[Stuart Parmenter]

(In reply to comment #30)
> Created an attachment (id=370534) [details]
> A patch
> 
> Brad, I think VirtualFree with MEM_DECOMMIT should be called before invoking
> VirtualFree with MEM_RELEASE in pages_unmap since VirtualAlloc with MEM_COMMIT
> is invoked in pages_map.

Why do you think that?  I've got a small testcase that only uses MEM_RELEASE that can continue to allocate forever in a loop.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #34)
> (In reply to comment #30)
> > Created an attachment (id=370534) [details] [details]
> > A patch
> > 
> > Brad, I think VirtualFree with MEM_DECOMMIT should be called before invoking
> > VirtualFree with MEM_RELEASE in pages_unmap since VirtualAlloc with MEM_COMMIT
> > is invoked in pages_map.
> 
> Why do you think that?  I've got a small testcase that only uses MEM_RELEASE
> that can continue to allocate forever in a loop.

As far as I confirmed, without DECOMMIT, AllocationProtect obtained by VirtualQuery keeps sometimes PAGE_READWRITE after invoking VirtualFree with MEM_RELEASE.

I think this will cause something badness.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #30)
> > > Created an attachment (id=370534) [details] [details] [details]
> > > A patch
> > > 
> > > Brad, I think VirtualFree with MEM_DECOMMIT should be called before invoking
> > > VirtualFree with MEM_RELEASE in pages_unmap since VirtualAlloc with MEM_COMMIT
> > > is invoked in pages_map.
> > 
> > Why do you think that?  I've got a small testcase that only uses MEM_RELEASE
> > that can continue to allocate forever in a loop.
> 
> As far as I confirmed, without DECOMMIT, AllocationProtect obtained by
> VirtualQuery keeps sometimes PAGE_READWRITE after invoking VirtualFree with
> MEM_RELEASE.
> 
> I think this will cause something badness.

Oops, I was wrong. I watched Protect, and MSDN says Protect is undefined with 
MEM_FREE state.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367834 [details] [diff] [review]
patch v.2

please provide a rolled up 191 patch in bug 485227

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: rolled up patch

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 372934 [details] [diff] [review]
rolled up patch

a191=beltzner

Comment 40

[Aki Sasaki (not active)]

Attachment: [checked in] add to mozconfig

Comment 41

[Aki Sasaki (not active)]

Comment on attachment 373048 [details] [diff] [review]
[checked in] add to mozconfig

revision 1095:ac0f4c2e7058
should get picked up on the next build.

Comment 42

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8274e63ba0a5

Comment 43

[Brian Crowder]

Sorry for late drive-by, but the rolled-up patch seems to have 8-character indentation and tabs in some spots.