RFR: 8150676: Use BufferNode index

[Kim Barrett]

> 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.

>   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.]

> 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.

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.