RFR: JDK-8256287: [windows] add loop fuse to map_or_reserve_memory_aligned

[Aleksey Shipilev]

On Thu, 12 Nov 2020 15:59:44 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> Hi,
> 
> may I please have reviews for this little fix:
> 
> On windows, `map_or_reserve_memory_aligned()` attempts to allocate aligned memory by:
> 1) reserving larger area
> 2) releasing it
> 3) attempting to re-reserve into the vacated address space a smaller area at the aligned start address
> 
> Since this may fail (between (2) and (3) someone may have grabbed part of that address space concurrently), we do this in a loop. However, when failing to release it we will loop-reserve endlessly.
> 
> This is one of the reasons for JDK-8255954 whose root cause will be fixed with JDK-8255978. But we should guard against endless loops independently from that.
> 
> This patch:
> - limits the number we try this to 20. If in 20 cases someone else grabs address space concurrently, something is off...
> - now correctly handles the return code of the release operation (1): in debug we assert, since if release_memory() fails something is wrong and we should take a look. In the release case, we now return an error.
> 
> 
> Thanks!

This makes sense to me. Just a few drive-by stylistic comments.

src/hotspot/os/windows/os_windows.cpp line 3165:

> 3163: 
> 3164:     bool rc = (file_desc != -1) ? os::unmap_memory(extra_base, extra_size) :
> 3165:                                   os::release_memory(extra_base, extra_size);

This style is inconsistent with style at L3155 and L3172. I think _those_ lines need to be brought in style.

src/hotspot/os/windows/os_windows.cpp line 3169:

> 3167:     if (!rc) {
> 3168:       aligned_base = NULL;
> 3169:       break;

Seems like we can just `return NULL`  on this path, like we do with `extra_base == NULL` check in the same loop? So we can have:

if (!rc) {
  assert(false, "Unmap/release should always succeed here");
  return NULL;
}

src/hotspot/os/windows/os_windows.cpp line 3152:

> 3150: 
> 3151:   char* aligned_base = NULL;
> 3152:   int attempts = 20;

static const int max_attempts = 20;
int attempts = max_attempts;

...and then you can use `max_attempts` in the assert message?

src/hotspot/os/windows/os_windows.cpp line 3175:

> 3173:       os::attempt_map_memory_to_file_at(aligned_base, size, file_desc) :
> 3174:       os::attempt_reserve_memory_at(aligned_base, size);
> 3175:     attempts --;

No space before unary op? E.g. `attempts--`? It might be even cleaner to move it to loop condition like `attempts-- > 0`?

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

PR: https://git.openjdk.java.net/jdk/pull/1191