openshmem-org / specification

OpenSHMEM Application Programming Interface
http://www.openshmem.org
50 stars 38 forks source link

Adding realloc semantics for shmem_malloc_with_hints routine #322

Closed manjugv closed 4 years ago

manjugv commented 4 years ago

The description of the behavior of shmem_realloc when ptr allocated with shmem_malloc_with_hints routine is passed to it.

manjugv commented 4 years ago

I'm adding this PR as discussed at the November 2019 meeting. I will read this ticket at the January meeting. If we think the chapter committee approval is good and does not require full committee approval, we can move it to the chapter committee.

manjugv commented 4 years ago

As per the discussion at the spec meeting, this PR will move to the chapter committee.

jdinan commented 4 years ago

@manjugv I took an AR to propose an update to shmem_malloc.tex instead of the proposed update to shmem_malloc_hints.tex on this PR. With apologies for the slow response, here is the proposed patch:

diff --git a/content/shmem_malloc.tex b/content/shmem_malloc.tex
index 17a004d1..90cd4a7b 100644
--- a/content/shmem_malloc.tex
+++ b/content/shmem_malloc.tex
@@ -45,13 +45,16 @@ void *@\FuncDecl{shmem\_align}@(size_t alignment, size_t size);

     The \FUNC{shmem\_realloc} routine changes the size of the block to which
     \VAR{ptr} points to the size (in bytes) specified by \VAR{size}.  The contents
-    of the block are unchanged up to the lesser of the new and old sizes. If the new
+    of the block are unchanged up to the lesser of the new and old sizes.
+    The \FUNC{shmem\_realloc} routine preserves allocation hints, e.g. if `ptr`
+    was allocated by \FUNC{shmem\_malloc\_with\_hints}. If the new
     size is larger, the newly allocated portion of the block is
     uninitialized.  If \VAR{ptr} is a null pointer, the
     \FUNC{shmem\_realloc} routine behaves like the \FUNC{shmem\_malloc} routine for
     the specified size.  If \VAR{size} is \CONST{0} and \VAR{ptr} is not a
     null pointer, the block to which it points is freed. If the space cannot
-    be allocated, the block to which \VAR{ptr} points is unchanged.
+    be allocated or if hints cannot be preserved, the block to which \VAR{ptr}
+    points is unchanged.

     The \FUNC{shmem\_malloc}, \FUNC{shmem\_align}, \FUNC{shmem\_free}, and \FUNC{shmem\_realloc} routines
     are provided  so that multiple \acp{PE} in a program can allocate symmetric,
manjugv commented 4 years ago

@jdinan Look good for me! Thanks.

manjugv commented 4 years ago

@swpoole @agrippa @jamesaross @tonycurtis @minsii Please approve or disapprove and suggest any changes. Thanks.

minsii commented 4 years ago

@manjugv @jdinan One question for Jim's patch: If hints cannot be preserved for realloc-block, should the block be unchanged? Or should it be resized with no hint?

tonycurtis commented 4 years ago

Looks ok...

manjugv commented 4 years ago

@manjugv @jdinan One question for Jim's patch: If hints cannot be preserved for realloc-block, should the block be unchanged? Or should it be resized with no hint?

@minsii It is unchanged. In a way, it is in the same spirit as realloc.

jdinan commented 4 years ago

@minsii My preference is for unchanged. A reallocation without hints can't be distinguished from a reallocation that preserved hints in the current API.

minsii commented 4 years ago

@manjugv @jdinan The reasons make sense to me. My only concern was that it somehow takes the hints as a "requirement" of realloc but not only a "hint." But I agree that there seems no better option than unchanged.

Approve the change.

manjugv commented 4 years ago

Ping @swpoole @agrippa @jamesaross

jamesaross commented 4 years ago

@jdinan's patch is better.

Codes really shouldn't be reallocating (at least expanding) heap memory. OpenSHMEM should have used brk/sbrk from the start :-)

Side note: It's odd that I do not receive mentions in email or otherwise. I have to look through the notifications, which is why I've been delayed.

manjugv commented 4 years ago

@agrippa You are looking at old changes. As discussed during the specification meeting, @jdinan changes to shmem_malloc.tex will replace the changes to shmem_malloc_hints.tex.

Please look here https://github.com/openshmem-org/specification/pull/322#issuecomment-577356017

jdinan commented 4 years ago

@manjugv Would it be helpful to apply that change to this PR so the "Files Changed" tab accurately reflects what is being proposed? I'm going to ask you to do this anyway before I can click the merge button. :neckbeard:

manjugv commented 4 years ago

@jdinan Please look here: https://github.com/manjugv/specification/pull/12

manjugv commented 4 years ago

@agrippa Please approve this https://github.com/manjugv/specification/pull/12

swpoole commented 4 years ago

manjugv:

approved

manjugv commented 4 years ago

Replaced by manjugv#12