RFR(xs): 8210068: Unsafe.allocateMemory() should not round up requested memory size

[Zhengyu Gu]

Hi Thomas,

On 11/7/18 2:27 AM, Thomas Stüfe wrote:
> Hi Zhengyu,
> 
> I am not sure I understand what you mean. I do not align up the
> pointer but the requested malloc size. The pointer is returned by
> malloc and hence adheres to the inbuilt malloc alignment requirments:
> 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html
> "The pointer returned if the allocation succeeds shall be suitably
> aligned so that it may be assigned to a pointer to any type of object
> and then used to access such an object in the space allocated"
>
*suitably aligned* is vague :-) the allocation size has direct impact of 
the alignment.


> Oh. Or, do you mean if we allocate space less than sizeof(void*) ? In
> that case there is this theoretical difference where ::malloc() is
> only required to return a pointer sufficiently aligned for any object
> *fitting into the requested space* whereas the definition of
> Unsafe.allocateMemory() talks about unconditional alignment. So,
> malloc(4) could return, in theory, a 4byte aligned address whereas
> Unsafe.allocateMemory() requires us to return an 8bytes aligned
> address (64bit)?

Basically, yes. But I believe the alignment requirement goes beyond 
sizeof(void*), e.g. malloc(128) will give you 128 byte alignment in most 
of allocator, IIRC.

I am bit puzzled by the alignment requirement, it does not allocate 
memory from Java heap. But it is a part of API, and here to stay :-)

> 
> I wonder whether this is just a sloppy definition of
> Unsafe.allocateMemory(). Does any caller of this function really
> require 4 allocated bytes to be 8byte aligned? But nevertheless, you
> are right, this would break the contract of Unsafe.allocateMemory().
> 
> One could fix this by only aligning up the size if requested size is
> less than sizeof(void*):
> 
>   UNSAFE_ENTRY(jlong, Unsafe_AllocateMemory0(JNIEnv *env, jobject
> unsafe, jlong size)) {
>     size_t sz = (size_t)size;
> + if (sz > sizeof(void*)) {
>       sz = align_up(sz, HeapWordSize);
> + }
>     void* x = os::malloc(sz, mtOther);
>     return addr_to_java(x);
>   } UNSAFE_END
> 
> But the more I think about this, the less I feel like driving this
> forward. Noone - including Adam, who originally asked about this -
> seems to care that much. So I guess I just drop it.

Agree.

Thanks,

-Zhengyu


> 
> Cheers, Thomas
> 
> 
> 
> 
> 
> On Tue, Nov 6, 2018 at 2:24 PM Zhengyu Gu <zgu at redhat.com> wrote:
>>
>> Hi Thomas,
>>
>> Apparently, Unsafe.allocateMemory() does have alignment requirement: the
>> result address should be aligned to *all value types*. So, it looks like
>> align_up() is required to satisfy to the API requirement.
>>
>>
>> /**
>>        * Allocates a new block of native memory, of the given size in
>> bytes.  The
>>        * contents of the memory are uninitialized; they will generally be
>>        * garbage.  The resulting native pointer will never be zero, and
>> will be
>>        * aligned for all value types.  Dispose of this memory by calling
>> {@link
>>        * #freeMemory}, or resize it with {@link #reallocateMemory}.
>>        *
>>        * @throws IllegalArgumentException if the size is negative or too
>> large
>>        *         for the native size_t type
>>        *
>>        * @throws OutOfMemoryError if the allocation is refused by the system
>>        *
>>        * @see #getByte(long)
>>        * @see #putByte(long, byte)
>>        */
>>       public native long allocateMemory(long bytes);
>>
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>
>> On 11/05/2018 02:13 AM, Thomas Stüfe wrote:
>>> Hi all,
>>>
>>> I am cleaning up my bug queue for 12.
>>>
>>> may I please have reviews for this small fix:
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210068
>>> cr: http://cr.openjdk.java.net/~stuefe/webrevs/8210068-Unsafe.allocateMemory-shall-not-round-up-user-size/webrev.01/webrev/
>>>
>>> In Unsafe_AllocateMemory0(), we align the size requested by the caller
>>> up like this:
>>>
>>>     sz = align_up(sz, HeapWordSize);
>>>     void* x = os::malloc(sz, mtOther);
>>>
>>> This rounding is unnecessary, since the additional space is not needed
>>> by the caller. In addition, this rounding causes NMT to over-report
>>> the numbers of mtOther - which consists mainly of nio DirectByteBuffer
>>> related allocations - the more buffers and the smaller they are the
>>> more inaccurate those numbers become.
>>>
>>> --
>>>
>>> This bug has been originally reported by Adam Farley, see fragments of
>>> a scattered discussion on core-libs:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054412.html
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054797.html
>>>
>>> --
>>>
>>> The patch ran through jdk-submit. I will put it through our
>>> SAP-internal tests to check whether removal of that alignment space
>>> could possibly trigger any hidden overwrite bugs.
>>>
>>> Thanks, Thomas
>>>