RFR: 8150676: Use BufferNode index

[Jon Masamitsu]

On 03/02/2016 02:49 PM, Kim Barrett wrote:
>> On Mar 2, 2016, at 3:09 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
> Jon,
>
> Thanks for looking at this.
>
>> http://cr.openjdk.java.net/~kbarrett/8150676/webrev.00/src/share/vm/gc/g1/dirtyCardQueue.hpp.frames.html
>>
>>    55   // Apply the closure to all active elements, from index to size.  If
>>
>> Change "size" to "_sz"?  It's hard to understand what "size" is from only the
>> signature of apply_closure().  Using "_sz" gives a (weak) hint that the upper limit
>> is a field in the class.
> Same applies to index vs. _index. But size is at least an accessor
> function.  And while I dislike mentioning internals in a "doc"
> comment, avoiding it here is probably harder than is useful.  So
> I'm updating the comment for both.

Thanks.
>
>>    69   static bool apply_closure_to_buffer(CardTableEntryClosure* cl,
>> Would "apply_closure_to_node" now be a better name?
> Not necessarily; the closure is to be applied to the node's buffer. [I
> also have some more cleanups in development; I'm not sure this
> function will survive in its current form.]

Ok.

>
>> http://cr.openjdk.java.net/~kbarrett/8150676/webrev.00/src/share/vm/gc/g1/satbMarkQueue.cpp.frames.html
>>
>>>   129     if (retain_entry(entry, g1h)) {
>>>   130       // Found keeper.  Search high to low for an entry to discard.
>>>   131       while ((src < --dst) && retain_entry(*dst, g1h)) { }
>>>   132       if (src >= dst) break;    // Done if no discard found.
>> If I'm at line 132 then "src" points to a keeper and is the lowest
>> (addressed) keeper.  If that's true, then if "dst" is less than "src"
>> as seems to be allowed at line 132, then the index calculation
>>>   137   _index = pointer_delta(dst, buf, 1);
>> seems like it will be off (too small).
> Recall that line 136 asserts (src == dst). Line 132 could equally well
> be "if (src == dst) break;" followed by asserting (src < dst), but I'm
> slightly less confident (or probably overly pessimistic) the compiler
> would optimize that away.

I stared a lot at the inequality in "if (src >= dst) break;" and could
no decide whether the inequality was needed so drifted down to the
consequences if  the inequality was needed (hence my question).

I would like "if (src == dst) break;" followed by asserting (src < dst)
better.  Or move the assertion up under line 132 so I don't miss it
the next time I look at this code.

>
> As written state (dst < src) is not possible anywhere in this
> algorithm, assuming the initial (_index <= _sz) invariant holds.
>
> I tried several ways of writing this, including the addition of state
> variables or using goto(!) (loop nests can be such a pain); what I
> published seemed like the easiest to understand (for me, today, but I
> might be too close to this code right now). I can provide some of
> those alternatives for discussion if you wish.
>
> One thing that makes it tricky is the aforesaid line 136 assertion,
> which I found very helpful, but which requires some additional code
> (such as line 132) to make it work. That is, this suffices,
>
>    for ( ; src < dst; ++src) {
>      if (retain_entry(entry, g1h)) {
>        while (src < --dst) {
>          if (!retain_entry(*dst, g1h)) {
>            *dst = entry;
>            break;
>          }
>        }
>      }
>    }
>    _index = pointer_delta(dst, buf, 1);
>
> but I find it less convincing without the final assert, which doesn't
> hold with this version.
>
>> http://cr.openjdk.java.net/~kbarrett/8150676/webrev.00/src/share/vm/gc/g1/ptrQueue.cpp.frames.html
>>
>>   209       BufferNode* node = BufferNode::make_node_from_buffer(_buf, _index);
>> Move 209 above 190
>>
>>   190     if (_lock) {
>>
>> and delete 222?
>>
>>   222       BufferNode* node = BufferNode::make_node_from_buffer(_buf, _index);
> I have another change in development that refactors things so that
> this code movement would need to be undone.

OK.

Looks good.

Jon
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160302/1fdc1412/attachment.htm>