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

[Thomas Stüfe]

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"

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)?

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.

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
> >