RFR: 8277531: Print actual default stacksize on Windows thread logging

[Thomas Stuefe]

On Tue, 23 Nov 2021 08:47:43 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> I modified to log actual stack size on Windows by using `os::win32::default_stack_size()`.
>> Could you please review this fix?
>
> Hi,
> 
> I try to understand this:
> 
> - in libjli we parse Xss the first time
> - create main thread with stacksize=Xss
> - invoke CreateJavaVM
> - in `os::init_2` we do those VirtualQuery() games to sniff out the thread stack size of the main thread, store that as `win32::_default_stack_size`
> - When creating new threads, we have the switch construct in `os::create_thread` (lines 695ff) where we set stack_size of new thread depending on thread type (which, btw, is what all posix platforms do in their implementation of `os::default_stack_size()`). But it can be left zero. If left zero, we call _beginthreadex with stack_size=0. 
> - We then call `describe_beginthreadex_attributes()` with stack_size=0. With this patch it would then print out the value of `win32::_default_stack_size`. I don't get the logic. What would this correspond to whatever Xss was? We just told the system to create a stack with whatever it thinks the default stack size should be. Whereas in jli, we explicitly specified a stack size.
> 
> So, I am not sure yet this is correct. 
> 
> I would like to see tests where we explicitly specify `Xss`, `CompilerThreadStackSize` and `VMThreadStackSize` and make sure correct values show up in the log. Sorry, a wrong output on logging is worse than no output at all.
> 
> ---
> 
> Zooming out, unrelated to this specific patch: I dislike how `os::default_stack_size()` mean different things in Posix and Windows. On Posix, it is a constant defined by us. On windows, it is whatever the main thread had been started with, plus some VirtualQuery related fuzziness. Currently, I am not sure what use this is on Windows.
> 
> On windows, `os::default_stack_size()` seems unused, at least I'm not sure what it is used for. The only use I find is to calculate `win32::_os_thread_limit` (see `os::init_2`), but that itself seems unused. What would be nice is to unify logic of `os::default_stack_size()` across all platforms.
> 
> I may be missing something here.
> 
> ---
> 
> About `os::current_stack_size()`:
> 
> I am not 100% sure sure the VirtualQuery is always correct. When doing some work with VirtualQuery (see https://bugs.openjdk.java.net/browse/JDK-8255978) I saw that the kernel merges back-to-back segments, similar to what Linux does with its VMAs. So what we think the thread stack is may actually be a number of fused regions showing up as one. Maybe it is not an issue in reality though as side effect of ours or the systems stack guards.

>@tstuefe I had hoped my comment made this clearer. But we do have to look at the main thread created by the launcher separate from threads created by the VM to ensure the process is the same. If there is no -Xss set the libjli code will pass zero as the stacksize to ContinueInNewThread (else the Xss value). If that code receives zero as the stacksize it checks if the vm_args have set an explicit javaStackSize, and if so that is used. Else zero is passed through as the stacksize for _beginthreadex and the stack size is determined by Windows.

>The main thread then does the VirtualQuery dance that tries to determine what the main threads actual size is and that is set as the os:;win32::default_stack_size() and represents the stacksize for any thread for which zero is passed to beginthreadex. (Actually it will also report the size of a non-zero -Xss - it reports whatever stack size the main thread got.)

The latter was my point. E.g. what about Xss != 0 and CompilerThreadStackSize == 0. Wouldn't we then start compiler threads with stack_size=0? Let the system decide? But then we'd go into the "default" reporting case and report `os::default_stack_size()` which is whatever the main thread uses after having been started non-0 Xss.

>But all of this is somewhat of an aside to this PR. We have a function that reports what the stack size is for threads which pass zero to beginThreadex

No, we have a function which reports the stack size of threads started with whatever Xss was, which may not be the system default, and may not match the stack size of threads started with stack size zero.

Sorry for being difficult. The patch is useful. But we should be able to rely on the numbers. That's why I would like more thorough tests which actually test the log output semantically, for a combination of stack size arguments, as proposed previously.

Thanks!

Thomas

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

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