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

[Erik Österlund]

Hi Kim,

On 2019-08-05 04:35, Kim Barrett wrote:
> "... 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.

This conversation whether we should or should not care about the 
aliasing rules has been repeated many times. And the answer is always 
the same: we should not care about aliasing rules. Summary of why:

* HotSpot inherently can't comply to the aliasing rules, as those rules 
are only defined in terms of C++ accesses interacting with each other. 
Since we JIT code performing accesses on the same memory as the C++ 
code, the aliasing rules can inherently never be trusted, as our 
interactions cross language barriers.
* Compilers used to compile HotSpot have all been told to ignore 
aliasing rules, and hence the behaviour of doing so is well defined. 
Because it has to be. If a compiler can't ignore aliasing, it can't 
compile HotSpot.
* Even if we were not inherently tied to ignore these aliasing rules, a 
significantly large part of HotSpot relies on it, and we can't 
realistically get rid of that reliance to turn the flags off. At least 
not by taking huge risk that we think we caught all the places, which 
would be rather irresponsible.
* Even if we could magically get rid of this reliance and somehow 
rewrite all code to conform with the aliasing rules, verify that we 
solved it all, and switch the compiler flags ignoring aliasing off, 
which would be a crazy amount of work, and be very risky, the benefits 
are seemingly unclear. They were introduced to allow compiler 
optimizations. I have not seen studies with numbers showing that it 
improves things. I have heard vague statements that it does, but not 
with numbers. I have found one study - a paper from Purdue where they 
had run with and without -fstrict-aliasing on a number of benchmarks, 
and observed no benefit. And most our time is spent in JIT:ed code, so I 
would expect even less if anything.
* Given the above, HotSpot does not, can not, and never will care about 
aliasing. They doe not make sense in our context, and the rules simply 
don't apply. Therefore, we can and should ignore the aliasing rules. 
Caring about them "sometimes" makes no sense to me. That's why I say I 
don't mind violating the aliasing rules, because they simply don't apply 
to HotSpot.

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

I agree that reliance is unnecessary, and was not fond of that myself 
either. Solved by wrapping _strong and _weak in a struct.

Incremental:
http://cr.openjdk.java.net/~eosterlund/8227054/webrev.01_02/

Full:
http://cr.openjdk.java.net/~eosterlund/8227054/webrev.02/

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

I don't know what you mean by that.

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

Yeah this style is indeed only nice when using the declaration form of a 
condition. If anyone thinks that should not be allowed for some reason, 
I don't have strong opinions about having a different iterator style.

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

Okay. I'm looking forward to seeing what you have cooked up.

Thanks,
/Erik