Thanks David!
On 17.11.2014 9:40, David Holmes wrote:
On 17/11/2014 7:23 AM, Ivan Gerasimov wrote:
Thank you Daniel!
Please find the updated webrev with your suggestions incorporated here:
http://cr.openjdk.java.net/~igerasim/8064694/1/webrev/
Concerning the thread priority: If the application is of
NORMAL_PRIORITY_CLASS, then setting the thread's priority level to
THREAD_PRIORITY_HIGHEST will result in its priority value to be only 10
(of maximum 31).
http://msdn.microsoft.com/en-us/library/windows/desktop/ms685100(v=vs.85).aspx
And if the process is HIGH_PRIORITY_CLASS, then the tread with the
HIGHEST priority level will have priority value == 15 of 31.
I believe, it should not be too much, and the machine will not become
busy with only those closing threads.
However, I hope it would be enough to make them complete faster than
other threads of the NORMAL priority level withing the same application.
I don't think this is necessary or desirable. Under normal usage we're
giving priority to exiting threads and that may disrupt the usual
scheduling patterns that applications see. You may posit that it is
"harmless" but we can't say that for sure. Nor can we actually know
that this will help with this particular bug. I would not add in this
new code.
There are two places where I put adjusting the thread's priority:
1) We've the array of handles filled up.
If we're found in this code branch, it'll mean that unfortunately we've
already got broken exit pattern, because the current thread has to do a
blocking call, having the ownership of a critical section.
The full array of handles means that many threads are exiting at that
time, thus all the threads that are starting to exit after the current
one will block at the attempt to grab ownership of the critical section.
Raising the priority of one thread that had already reached
_endthreadex(), seems appropriate to me in such a situation, because it
helps shorten the period of time when the threads remain blocked.
Choosing the oldest exiting thread ensures that the period of time when
the priority of one thread is higher is the smallest possible.
2) The process exit branch.
That's the main part of the fix -- here we make the process to wait for
all the threads having called _endthreadex() to complete, at the same
time preventing any other threads from starting the exiting procedure.
The execution flow is already changed here (I don't want to say
disrupted, because it was meant to fix the issue).
All running threads are about to be terminated soon by ending the
process, so raising the priority of some of the threads should not have
any bad impact on the program flow.
Instead, it may make the time the process has to wait before calling
exit() shorter.
I can surely remove that playing with the threads' priority, as it's not
the essential part of the fix.
However, I think it's a useful hint to the scheduler, which can improve
things in some situations, and I'm not really sure how it can harm.
Sincerely yours,
Ivan
David
Sincerely yours,
Ivan
On 15.11.2014 2:22, Daniel D. Daugherty wrote:
On 11/14/14 5:35 AM, Ivan Gerasimov wrote:
Hello!
The recent fix for JDK-8059533 ((process) Make exiting process wait
for exiting threads [win]) caused the warning message to be printed
in some test environments:
-----------
os_windows.cpp:3844 is in the newly updated
os::win32::exit_process_or_thread(Ept what, int exit_code)
-----------
This has been observed with debug builds on highly loaded systems.
To address the issue it is proposed to do three things:
1) increase the timeout for debug builds,
2) increase the maximum number of the thread handles to be stored,
3) rise the priority of the exiting threads, if we need to wait for
them.
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8064694
WEBREV: http://cr.openjdk.java.net/~igerasim/8064694/0/webrev/
src/os/windows/vm/os_windows.cpp
line 3784: #define MAX_EXIT_HANDLES NOT_DEBUG(32) DEBUG_ONLY(128)
Instead of NOT_DEBUG can you use PRODUCT_ONLY?
Instead of DEBUG_ONLY can you used NOT_PRODUCT?
That uses the smaller value for only one build config (PRODUCT).
line 3785: #define EXIT_TIMEOUT NOT_DEBUG(1000) DEBUG_ONLY(4000)
/*1 sec in product, 4 sec in debug*/
Instead of NOT_DEBUG can you use PRODUCT_ONLY?
Instead of DEBUG_ONLY can you used NOT_PRODUCT?
Please add spaces between the comment delimiters and the comment
text.
That uses the smaller timeout for only one build config (PRODUCT).
line 3836 // Rise the priority...
Typo: 'Rise' -> 'Raise'
About the general idea of raising the exiting thread's priority,
if the exiting thread is looping in some Win* OS code after this
point, will raising the priority make the machine unusable?
Dan
The fix was tested on all available platforms, with the hotspot
testset. No failures.
Sincerely yours,
Ivan
