RFR: 8303852: current_stack_region() gets called twice unnecessarily [v2]

[David Holmes]

On Fri, 18 Aug 2023 08:20:08 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> David Holmes has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Excess newlines
>>  - Add spaces around operator
>
> 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

Thanks for looking at this @tstuefe - as I noted I didn't try to merge things further as I thought it would makes things more complex. But to address your specific points:

> the implementations for zero and non-zero Linux are identical and could be merged.

But as you then state they are not actually identical because zero is missing some of the guard page checks. But zero also has the IA64 code which causes a subtle change in the way size needs to be adjusted. So they are not the same and while they could potentially be merged I much prefer zero code to be kept isolated to zero so that the folks interested in zero can deal with it.

> bsd aarch64 and x64 could be merged - the only difference is a workaround in x64 that is missing on arm, from 8020753

I did wonder about that workaround but it was not something I wanted to touch in this PR. If it is not needed afterall, or actually needed on Aarch64 then that is something for a different RFE (or actual bug if the workaround is needed on aarch64).

> the bsd zero variant is almost equal to the arm variant, so we could merge those.

Again I like to see zero code isolated, but in addition there is nowhere to put such code except in the shared os_bsd.cpp and then have it ifdef'd out for x86. That said, if the x86 and aarch64 should be the same then yes we could have a single shared version. But again future RFE (I will file one).

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

Yes I think it can. The arch specific versions would just be moved to os namespace instead, and no shared implementation just to redirect to the arch-specific ones.

Thanks.

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

PR Comment: https://git.openjdk.org/jdk/pull/15321#issuecomment-1685417382