<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 4/6/2015 2:20 PM, Srinivas
Ramakrishna wrote:<br>
</div>
<blockquote
cite="mid:CABzyjynov=_JqjSDZ3pS=AE2jNVg5y6R0ABzb7JDvv710gGemw@mail.gmail.com"
type="cite">
<div dir="ltr">Hi Joe --</div>
</blockquote>
Hi Ramki, nice to hear from you and thanks for the review.
<blockquote
cite="mid:CABzyjynov=_JqjSDZ3pS=AE2jNVg5y6R0ABzb7JDvv710gGemw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div><br>
</div>
<div>I am not terribly familiar with G1 code yet, but here's a
suggestion:
<div><br>
</div>
<div>* rename the boolean field to "in_suspendible_thread_set"
(or "suspendible_thread"), the corresponding setters and
gettes to</div>
<div>set_in_suspendible_thread_set(),
clear_in_suspendible_thread_set(), and
is_in_suspendible_thread_set(), or preferably:</div>
<div>set_suspendible_thread(), clear_suspendible_thread() and
is_suspendible_thread(). (I don't particularly like the
set_...._set() naming.)</div>
</div>
</div>
</blockquote>
<br>
I changed the variable and the methods as you suggested. Much
easier on the eyes.<br>
<br>
DEBUG_ONLY(bool _suspendible_thread;)<br>
<br>
<blockquote
cite="mid:CABzyjynov=_JqjSDZ3pS=AE2jNVg5y6R0ABzb7JDvv710gGemw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div><br>
</div>
<div>Do we allow threads to recursively enter the suspendible
thread set in nested fashion? I'm guessing we don't, in
which case, you would</div>
<div>use this information local to the thread to
prevent/forbid such recursive entries or exits when it isn't
part of the set, by asserting</div>
<div>that the bit isn't set when a thread joins the set and
never clear before a thread leaves the set. I understand
that there's a scoped</div>
<div>join/leave abstraction via SuspendibleThreadSetJoiner,
but there also seem to be a bunch of naked join/leave calls,
so the</div>
<div>extra checking would likely be worthwhile.</div>
</div>
</div>
</blockquote>
This is a good question. If I put in checks in join / leave<br>
<br>
void SuspendibleThreadSet::join() {<br>
assert(Thread::current()->is_suspendible_thread() == false,<br>
"Thread already joined");<br>
<br>
void SuspendibleThreadSet::leave() {<br>
//assert(Thread::current()->is_suspendible_thread(),<br>
// "Thread not joined");<br>
<br>
jprt hits the assert in join.<br>
<br>
It doesn't seem right to call join if a thread is already joined. <br>
Would it be correct to just return if trying to rejoin or trying to
leave when not joined?<br>
<br>
What's there seems to be harmless but I can't be sure of that.<br>
<br>
joe<br>
<blockquote
cite="mid:CABzyjynov=_JqjSDZ3pS=AE2jNVg5y6R0ABzb7JDvv710gGemw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div><br>
</div>
<div>As Eric notes, you could make all the new fields/methods
NOT_PRODUCT, but I am not too concerned about leaving them
in product mode</div>
<div>as well, since the frequency of transition to and from
STS mode is probably fairly low, and the extra information
may be useful for</div>
<div>field serviceability. But I will let those more familiar
with G1 opine more strongly on that.</div>
<div><br>
</div>
<div>Looks good otherwise.</div>
<div><br>
</div>
<div>Thanks!</div>
<div>-- ramki (openjdk: ysr)</div>
<div><br>
</div>
<div><br>
</div>
</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Apr 6, 2015 at 10:25 AM, Joseph
Provino <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:joseph.provino@oracle.com" target="_blank">joseph.provino@oracle.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">CR: <a
moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-7006810"
target="_blank">https://bugs.openjdk.java.net/browse/JDK-7006810</a><br>
<br>
webrev: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejprovino/7006810/webrev.00"
target="_blank">http://cr.openjdk.java.net/~jprovino/7006810/webrev.00</a><br>
<br>
test: jprt<br>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</body>
</html>