Thanks David and Daniel!
Yes, as David wrote, a thread-id is 32-bit on Windows.
I'll do all the suggested changes and will push the fix later today.
Sincerely yours,
Ivan
On 06.01.2016 7:25, David Holmes wrote:
On 6/01/2016 3:33 AM, Daniel D. Daugherty wrote:
Happy New Year!
And to you :)
> http://cr.openjdk.java.net/~igerasim/8145127/01/webrev/
src/os/windows/vm/os_windows.cpp
L3923: Atomic::cmpxchg((jint)GetCurrentThreadId(),
&process_exiting, 0);
What's the return size of GetCurrentThreadId()? Are we
It is a DWORD so 32-bit.
Cheers,
David
-----
down casting a larger size into a 'jint'? If so, that
might allow more than one thread into this code when we
only want one...
Also please consider adding a comment above this line.
Perhaps something like:
// Atomically set process_exiting before the critical section
// to increase the visibility between racing threads.
L3998: if (portion_count > MAXIMUM_WAIT_OBJECTS) {
L3999: portion_count = MAXIMUM_WAIT_OBJECTS;
Wrong indent; should be two spaces relative to L3998.
L4013: portion_count = handle_count - i;
Please consider adding a comment for this error case.
Perhaps something like:
// Reset portion_count so we close the remaining
// handles due to this error.
L4030: if (OrderAccess::load_acquire(&process_exiting) != 0 &&
L4031: process_exiting != (jint)GetCurrentThreadId()) {
L4032: while (true) {
L4033: // Some other thread is about to call exit(), so we
L4034: // don't let the current thread proceed to exit() or
_endthreadex()
The comment on L4033-4 is slightly misplaced now. It
should be between L4031 and L4032.
Thumbs up modulo the GetCurrentThreadId() return size question
above. If that size is OK and you choose to make the comment
changes, I don't need to see another webrev.
Dan
On 12/23/15 5:20 AM, Ivan Gerasimov wrote:
Thank you David for review!
Sincerely yours,
Ivan
On 23.12.2015 7:41, David Holmes wrote:
Looks okay.
Second review needed though.
Thanks,
David
On 19/12/2015 10:28 PM, Ivan Gerasimov wrote:
We will suspend the current thread if two conditions are satisfied:
process_exiting != 0 and process_exiting != current thread id.
But, yes, pr_ex isn't really needed, as we can use process_exiting
directly.
Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8145127/01/webrev/
I still see pr_ex ??
Oops.
Forgot to 'hg qrefresh' before generating the webrev.
The webrev has just been updated in place.
Sincerely yours,
Ivan
