<p dir="ltr">Hi John,</p>
<p dir="ltr">Thanks for the feedback.  Regarding the asserts question, here's an example g1HotCardCache.cpp):</p>
<p dir="ltr">assert(worker_i < (int) (ParallelGCThreads == 0 ? 1 : ParallelGCThreads), "incorrect worker id");</p>
<p dir="ltr">Would be useful to see worker_i here?<br>
 <br>
Another in g1CardCounts.hpp:</p>
<p dir="ltr">  67   void check_card_num(size_t card_num, const char* msg) {<br>
  68     assert(card_num >= 0 && card_num < _committed_max_card_num, msg);<br>
  69   }</p>
<p dir="ltr">The msg will have the card_num, but you won't have _committed_max_card_num reported, for example.  That's the kind of thing I meant.</p>
<p dir="ltr">There may be a few more examples like that, but this came up during quick scan.</p>
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Jan 23, 2013 6:36 PM, "John Cuthbertson" <<a href="mailto:john.cuthbertson@oracle.com">john.cuthbertson@oracle.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Vitaly,<br>
<br>
Second response. Details inline....<br>
<br>
On 1/15/2013 8:49 PM, Vitaly Davidovich wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
A few more comments/suggestions:<br>
<br>
In g1CardCounts::ptr_2_card_num()<u></u>, I'd assert that card_ptr >= _ct_bot.  This is mostly to avoid a null card_ptr (or some other bogus value) causing the subtraction to go negative but then wrap around to a large size_t value that just happens to fit into the card range.  Unlikely and maybe this is too paranoid, so up to you.<br>

<br>
</blockquote>
<br>
Good idea. Done. I'm all for being paranoid. I've also changed the subtraction to use pointer_delta:<br>
<br>
    assert((size_t)card_ptr >= (size_t)_ct_bot, "wraparound?");<br>
    size_t card_num = pointer_delta(card_ptr, _ct_bot, sizeof(jbyte));<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also in this class, it's a bit strange that G1CardCounts::is_hot() also increments the count.  I know the comments in the header say that count is updated but the name of the method implies it's just a read.  Maybe call add_card_count() and then add an is_hot(int) method and call that with the return value of add_card_count?<br>

<br>
</blockquote>
<br>
Let me think about that one. I was looking for a much simpler interface that returned whether a card was was hot. How it made that determination was up to it. But I'm leaning toward following your suggestion.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
G1CardCounts::clear_region -- there are some casts of const jbyte to jbyte at bottom of method.  Perhaps if ptr_2_card_num() were changed to be taking const jbyte you wouldn't need the casts.  I think marking as much things const as possible is good in general anyway ...<br>

<br>
</blockquote>
<br>
Good point. Done.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In the various places where values are asserted to be in some range, it may be useful to add the valid range to the error message so that if it triggers you get a bit more context/diagnostic info.<br>
<br>
</blockquote>
<br>
I thought I was doing that in most cases. Can you give some examples?<br>
<br>
Thanks,<br>
<br>
JohnC<br>
</blockquote></div>