RFR: 8151436: Leaner ArrayAllocator and BitMaps

[Stefan Karlsson]

Hi all,

I found a bug in the ArrayAllocator code. ArrayAllocator::free 
incorrectly calls should_use_malloc(size_for_malloc(length)) when the 
call should be should_use_malloc(length).

There is an -XX:+ExecuteInternalVMTests that exercises this code path, 
and try to find these kinds of bugs. The test actually manages to call 
allocate_malloc and then tries to free the memory with free_mmap, but 
the JVM doesn't crash. One reason why it doesn't crash is that 
os::release_memory, which is used by free_mmap, fails to unmap the 
malloced memory and we silently ignores that.

This patch:
1) Adds an assert to ensure that the os::release_memory call succeeds. 
This immediately shows the problem when we execute our internal vm tests.
2) Moves the implementation of should_use_malloc to the inline file, so 
that it can reuse size_for_malloc.
3) Fix the bug.

http://cr.openjdk.java.net/~stefank/8151436/webrev.01.delta/
http://cr.openjdk.java.net/~stefank/8151436/webrev.01/

Thanks,
StefanK

On 2016-03-08 11:33, Stefan Karlsson wrote:
> Hi all,
>
> Please review this patch to remove the state variables from 
> ArrayAllocator and make it an AllStatic class instead. The main 
> motivation for this patch is to cleanup the BitMaps data structure and 
> make the instances smaller.
>
> The patch builds on the fact that the current code invokes 
> AllocateHeap with the default value AllocFailEnum:EXIT_OOM, and 
> therefore never returns NULL. This means that use_malloc will never be 
> reset and the allocation strategy used (malloc or mmap) can be 
> determined by simply looking at the size input parameter. Instead of 
> changing the code to use AllocateHeap with AllocFailEnum::RETURN_NULL, 
> I chose to keep the current behavior so that we could cleanup and 
> minimize the size of our BitMaps.
>
> http://cr.openjdk.java.net/~stefank/8151436/webrev/
> https://bugs.openjdk.java.net/browse/JDK-8151436
>
> Thanks,
> StefanK