RFR: 8227054: ServiceThread needs to know about all OopStorage objects

[Kim Barrett]

> On Aug 3, 2019, at 6:36 AM, Erik Osterlund <erik.osterlund at oracle.com> wrote:
> On 2 Aug 2019, at 22:01, Kim Barrett <kim.barrett at oracle.com> wrote:
>> […]
>> I prefer code that doesn't invoke undefined behavior, even if it
>> might work (or even probably works) in practice.
> 
> Me too, unless it’s aliasing rules, which I don’t mind violating given the chance. Mind elaborating what you think I missed?

"... unless it’s aliasing rules, which I don’t mind violating given
the chance."

"... worth considering, I think, due to its simplicity."

Those two phrases cannot legitimately be used to describe the same
code.  Code that uses (and indeed fundamentally relies on) casts,
especially reinterpret_casts, is never simple.

Please don't casually violate the aliasing rules, and carefully
document when it's being done and why.  reinterpret_casts (by whatever
spelling) are a huge punch in the nose for the type system.  A
multitude of sins can be hidden, sometimes unintentionally and to our
significant detriment.  We resort to them *way* too often.

The problem being dealt with here doesn't even come close to being one
of those necessary evils where we (think we) must break the rules, so
no, I don't like or accept that proposal.

Also, when looking at that code the disreputable casts so drew my eye
that I very nearly missed another problem:

  47   static StrongOopStorageSet _strong;
  48   static WeakOopStorageSet _weak;

Nothing requires those objects be placed in memory in such a way that
the "all" iterator will work.

> 
>> And an iterator whose initial "current" is invalid?  You think
>> *macros* are confusing?
> 
> That’s Java style iterators I guess. You call next until you can’t. Before you called next the first time, there is no current element that has been handed out. I don’t mind changing the iterator style though if you think that is confusing. It is my preferred style for simple linear iteration.

In this case that idiom seems like it expends additional effort to
potentially hide bugs.

It also seems less convenient to use, though if one uses implicit
booleans (as the proposed code does), that disadvantage is nullified.
(I've had many occasions to want to use the declaration form of a
condition, but haven't because there were strong complaints when I
first tried doing so, and I've since seen it objected to when others
tried.  But I found some recently added, so maybe that's no longer
true?  Or maybe the previous complainers didn't notice.)

I have another version in progress that doesn't use x-macros and uses
somewhat fewer macros (though not none, since I really dislike
boiler-plate).  It isn't finished because of a power shutdown in the
office over the weekend.