RFR (S): 8230611: infinite loop in LogOutputList::wait_until_no_readers()

[Kim Barrett]

> On Nov 20, 2019, at 11:51 AM, David Buck <david.buck at oracle.com> wrote:
> 
> Hi!
> 
> May I please get a review of this small fix:
> 
> bug report: https://bugs.openjdk.java.net/browse/JDK-8230611
> proposed fix: http://cr.openjdk.java.net/~dbuck/8230611_ver00/
> 
> Cheers,
> -Buck

Note that RVO becomes mandatory in C++17. Not that this helps us
today. Though I'm somewhat surprised that any even vaguely recent
compiler would fail to do it.

(Side comment on this code: yet another SMR mechanism.)

The proposed operator= has some problems:

(1) Self-assignment bug. Self-assignment is rarely an issue in
practice, but if it costs nothing to handle, then one should do so.
The problem is that decreasing the counter before increasing it could
allow an existing waiter to proceed. Just do the increase first.

(2) operator= should return *this unless there's a good reason not to.

(3) There's no reason for the rhs argument to be non-const.

(4) The indentation is not HotSpot's normal 2-space.

So a better definition would be (1)

Iterator& operator=(const Iterator& rhs) {
  _current = rhs._current; 
  rhs._list->increase_readers();
  _list->decrease_readers();
  _list = rhs._list;
  return *this;
}

If assignments where the _list is the same for both is a common case,
it might be worthwhile to avoid the counter manipulation altogether in
that case, e.g. (2)

Iterator& operator=(const Iterator& rhs) {
  _current = rhs._current; 
  if (_list != rhs._list) {
    rhs._list->increase_readers();
    _list->decrease_readers();
    _list = rhs._list;
  }
  return *this;
}

I think the copy-swap idiom probably isn't the way to go here.
Compared to (2) it has fewer counter mods for the rvalue with
different lists, but more for the lvalue with the same list. It is
always at least as good as (1) though. But to really minimize counter
manipulations one wants move-construct and move-assign to handle the
rvalue case, and we don't have those yet.

https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom/3279550#3279550

I think the move of the call to increase_readers() from
LogOutputList::iterator to the Iterator constructor is potentially a
mistake. I think the counter protection should be increased before the
node is obtained from the _level_start array. It might be that there
are reasons why the proposed change will work, but I think the
original code is safer and easier to analyze.