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

[Erik Österlund]

Hi Coleen,

On 2019-08-05 15:23, coleen.phillimore at oracle.com wrote:
>
> 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.

Thanks for having a look. I agree I would be happier if this could be 
done without reinterpret_cast.
It's obviously possible with some extra boiler plate, but I know Kim 
doesn't like boilerplate.
But yeah, as you say, there are very few of these OopStorage instances, 
so I have no strong opinions
about adding that boilerplate (with an assert so you can't miss it).
I was mostly trying to show that there are alternatives to the macro 
metaprogramming  + enum approach, as a direction.

> I look forward to seeing Kim's next version.

Me too!

Thanks,
/Erik

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