RFR: 8150676: Use BufferNode index (original) (raw)

Jon Masamitsu jon.masamitsu at oracle.com
Wed Mar 2 22:56:42 UTC 2016

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 applyclosure(). 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 applyclosuretobuffer(CardTableEntryClosure* cl, Would "applyclosuretonode" 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 (retainentry(entry, g1h)) { 130 // Found keeper. Search high to low for an entry to discard. 131 while ((src < --dst) && retainentry(*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 = pointerdelta(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 (retainentry(entry, g1h)) { while (src < --dst) { if (!retainentry(*dst, g1h)) { *dst = entry; break; } } } } index = pointerdelta(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::makenodefrombuffer(buf, index); Move 209 above 190 190 if (lock) { and delete 222? 222 BufferNode* node = BufferNode::makenodefrombuffer(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>

More information about the hotspot-gc-dev mailing list