RFR: 8303852: current_stack_region() gets called twice unnecessarily

[Thomas Stuefe]

On Thu, 17 Aug 2023 02:45:54 GMT, David Holmes <dholmes at openjdk.org> wrote:

> We combine:
> 
> address os::current_stack_base();
> size_t os::current_stack_size();
> 
> into
> 
> void os::current_stack_base_and_size(address* stack_base, size_t* stack_size)
> 
> and so avoid making two underlying system calls. The os/platform specific specializations are handled by the `current_stack_region` calls. It made sense to modify that to export the `base` directly rather than the `bottom`. In doing that change it made sense to standardise on the the style of the code used (variable names etc) to make it easier to see where the 5 different versions were the same and where they differ. Having 5 versions is unfortunate ( 3 BSD: one per arch - zero, x64, aarch64; and 2 linux: zero and everything else), but trying to combine them with ifdefs would be even worse in my opinion.
> 
> In theory there should be zero functional changes here.
> 
> Testing:
>  - Tiers 1-3
>  - All our internal builds in tiers 1-5
>  - GHA
> 
> Thanks.

Hi David,

looks good.

some thoughts:

- the implementations for zero and non-zero Linux are identical and could be merged. I looked at them side by side. The zero implementation looks a bit bitrotted (e.g. does not have the latest guard size adjustments) so unifying them would make zero more correct.

- bsd aarch64 and x64 could be merged - the only difference is a workaround in x64 that is missing on arm, from 8020753: "JNI_CreateJavaVM on Mac OSX 10.9 Mavericks corrupts the callers stack size". That workaround claims to fix the problem of not-page-aligned stack size for the primordial thread. I just tested on my M1 arm, and the primordial thread has a stack size of 2060K (java -Xlog:os* -version), which is not page-aligned (16K pages). So either this workaround is not needed (since the JVM on M1 seems happy enough) or it is a hidden bug and we need the workaround. In both cases, we could merge both versions of current_stack_region.
- the bsd zero variant is almost equal to the arm variant, so we could merge those.

And then, since you equalized the interfaces of os::XX::current_stack_region() and os::get_stack_base_and_size, would it be possible to drop the former and implement the latter for the various OSes?

All these are thoughts, and possibly for future RFEs. THis patch looks ok as it is too.
 
Cheers, Thomas

src/hotspot/os/linux/os_linux.cpp line 5412:

> 5410:         fatal("pthread_attr_getguardsize failed with error = %d", rslt);
> 5411:       }
> 5412:       bottom += guard_size;

Why do we keep correcting bottom?

src/hotspot/os/linux/os_linux.cpp line 5428:

> 5426: }
> 5427: 
> 5428: 

nit: lose some newlines?

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

> 442:   // AllocationBase.
> 443:   while (1) {
> 444:     VirtualQuery(stack_bottom+size, &minfo, sizeof(minfo));

nit: spaces around +, since you are here?

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

Marked as reviewed by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15321#pullrequestreview-1583904296
PR Review Comment: https://git.openjdk.org/jdk/pull/15321#discussion_r1298045263
PR Review Comment: https://git.openjdk.org/jdk/pull/15321#discussion_r1298040841
PR Review Comment: https://git.openjdk.org/jdk/pull/15321#discussion_r1298041135