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

[coleen.phillimore at oracle.com]

Hi,

Both versions of this change has the part that I like, ie removing 
oopStorage initialization and most knowledge from the client code.

While I don't like capital letters either, X macros make sense and we 
have them everywhere.   I have to admit that this threw me and I was not 
happy that it was going to take me time to puzzle out this trick, which 
now I see is something that runs off the end of a struct.  please dont.

   47   static StrongOopStorageSet _strong;
   48   static WeakOopStorageSet _weak;
   49
   50   static OopStorage** strong_oopstorage_set() { return reinterpret_cast<OopStorage**>(&_strong); }
   51   static OopStorage** weak_oopstorage_set() { return reinterpret_cast<OopStorage**>(&_weak); }
   52   static OopStorage** all_oopstorage_set() { return strong_oopstorage_set(); }
   53

The new version of this isn't that much nicer to look at and still has 
the reinterpret cast.

The iterator and cpp file changes are nicer though.

This seems to be a lot of effort to not repeat listing any of the 6 
total oop storages that we have, which could be done in the .hpp file 
alongside the weak/strong classifications.

I look forward to seeing Kim's next version.

Coleen

On 8/5/19 3:33 AM, Erik Österlund wrote:
> 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