<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
On 3/12/13 6:08 PM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:513F610F.1060908@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Thanks for looking at the changes.<br>
<br>
On 3/12/2013 8:05 AM, Bengt Rutisson wrote:<br>
<blockquote cite="mid:513F4420.8050307@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
Thanks for fixing this so quickly!<br>
</div>
</blockquote>
<br>
The main change is based upon your patch. It just took a little
time to evaluate and disregard the alternatives (as well as fix
the underlying problems they discovered).<br>
<br>
<blockquote cite="mid:513F4420.8050307@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
I have only had a quick look at the change. I'll make sure to
look closer tomorrow. However, I have two questions. If you
have time to answer them it would be good. If you don't have
time I hope they become clear when I look more in detail at
the change tomorrow...<br>
<br>
First, in the constructor for
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
G1CMDrainMarkingStackClosure() we do:<br>
<br>
2285 _do_stealing = !_is_serial;<br>
2286 _do_termination = true;<br>
</div>
</blockquote>
<br>
Yes we can. I had thought of that but I thought people wouldn't
like it. <br>
</blockquote>
<br>
I think I would prefer a single _is_serial or even _is_par instance
variable.<br>
<br>
<blockquote cite="mid:513F610F.1060908@oracle.com" type="cite">
<blockquote cite="mid:513F4420.8050307@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Just from looking at this it seems like we could get away with
only having one instance variable instead of three. Is that
the case or can any of _do_stealing, _is_serial or
_do_termination change during the life time of a
G1CMDrainMarkingStackClosure instance?<br>
<br>
Second, as you describe in the text below you are actually
fixing two issues with this patch. Would it make sense to
split the changes up into two changesets?<br>
</div>
</blockquote>
<br>
OK. That's probably a good idea as long as the second gets in
fairly soon - the Lucene tests will fail with an assertion failure
when parallel reference processing is enabled, if the overflows
occur at just the right points. I'll split them up into adding the
serial path to do_marking_step followed by the changes for the
assertion failure.<br>
</blockquote>
<br>
Great, thanks! I think we can push the changes at the same time. It
is just much easier to review one thing at a time.<br>
<br>
<blockquote cite="mid:513F610F.1060908@oracle.com" type="cite"> <br>
New webrev soon.<br>
</blockquote>
<br>
:) Looking forward to it! <br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:513F610F.1060908@oracle.com" type="cite">
Thanks,<br>
<br>
JohnC<br>
<br>
</blockquote>
<br>
</body>
</html>