RFR (M) 8249768: Move static oops and NullPointerException oops from Universe into OopStorage

Wed Jul 22 13:13:41 UTC 2020

On 7/22/20 8:42 AM, David Holmes wrote:
> On 22/07/2020 9:50 pm, coleen.phillimore at oracle.com wrote:
>> On 7/22/20 2:32 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 22/07/2020 12:19 am, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 7/20/20 10:47 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> cc'ing serviceability due to SA changes.
>>>>>
>>>>> On 21/07/2020 6:53 am, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: Move static oops into OopStorage and make NPE oops an 
>>>>>> objArrayOop.
>>>>>>
>>>>>> I've broken up moving oops in Universe to OopStorage into several 
>>>>>> parts.  This change moves the global static oops. These 
>>>>>> OopHandles are not released.
>>>>>
>>>>> Overall looks good. But two things ...
>>>>>
>>>>> 1. naming
>>>>>
>>>>> !   // preallocated error objects (no backtrace)
>>>>> !   static OopHandle    _out_of_memory_error;
>>>>>
>>>>>     // array of preallocated error objects with backtrace
>>>>> !   static OopHandle _preallocated_out_of_memory_error_array;
>>>>>
>>>>> Both of these are pre-allocated arrays of OopHandles, differing 
>>>>> only in whether the underlying OOME has a backtrace or not. I find 
>>>>> the newly introduced _out_of_memory_error unclear in that regard. 
>>>>> At a minimum could _out_of_memory_error become 
>>>>> _out_of_memory_errors ? But ideally can we name these two arrays 
>>>>> in a more distinguishable way?
>>>>
>>>> I put this code in functions next to each other because it was 
>>>> confusing.  The _out_of_memory_error array is really preallocated 
>>>> throwables, that are used to fill in the message for 
>>>> preallocated_out_of_memory_errors if there are enough of the latter 
>>>> left.
>>>> I could rename _out_of_memory_error => 
>>>> _out_of_memory_error_throwables  ?
>>>
>>> That doesn't really help. As I said both of these variables are 
>>> arrays of pre-allocated OOME instances (which are throwables) - the 
>>> only difference is one set is single-use (as it can have its 
>>> backtrace set) while the other is reusable. The existing variable
>>>
>>> _preallocated_out_of_memory_error_array
>>>
>>> tells you clearly it is an array of preallocated OOME instances (but 
>>> doesn't saying anything about the backtrace or being single-use). 
>>> The problem is that that is exactly what _out_of_memory_error is as 
>>> well, but the name _out_of_memory_error doesn't convey that it is an 
>>> array, nor that anything is pre-allocated (and also nothing about 
>>> backtraces or re-usability). So given we now have two arrays of 
>>> extremely similar things, it would be clearer to give these clearer 
>>> names. If we want to keep the existing
>>>
>>> _preallocated_out_of_memory_error_array
>>>
>>> name, then the new array should indicate how it differs e.g.
>>>
>>> _reusable_preallocated_out_of_memory_error_array
>>> What do you think?
>>
>> This confuses me more than the code does.  Which array is this? This 
>> is a terrible name for whichever one it is (I guess the original 
>> _out_of_memory_error).  I don't think it's interesting to have the 
>> name _array in it, but being plural does tell you what it is, like 
>> _out_of_memory_errors.
>
> Yes at least being plural is essential to realize it is actually an 
> array.
>
>>   The implementation is a bit weird and some long name isn't going to 
>> help anyone.  The abstraction that this is _out_of_memory_errors is 
>> all anyone outside this implementation needs to know.
>
> My point, which is obviously not getting across, is that you now have 
> two arrays of these out-of-memory-errors that are almost identical, 
> except one is used for one purpose and one used for another, but the 
> variable names don't give you any clue about this.

I actually understand this, but the suggested names don't help.  You 
really need to look at the code and the comments in universe.hpp to see 
the distinction. I don't think we can provide more illumination with 
long names.  Since I moved the functions next to each other, it makes 
more sense when one reads it.

>
> But lets' just add the 's' and move on. :)

Thanks,  I added the 's' and fixed the formatting.  Thank you for 
reviewing this.
Coleen

>
> Cheers,
> David
> -----
>
>>>
>>> I also spotted a minor nit:
>>>
>>>  187 oop      Universe::system_thread_group()          { return 
>>> _system_thread_group.resolve(); }
>>>  188 void Universe::set_system_thread_group(oop group) { 
>>> _system_thread_group = OopHandle(vm_global(), group); }
>>>
>>> Extra spaces after oop on L187.
>>
>> Ok I'll fix the spacing.
>> Thanks,
>> Coleen
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>>
>>>>> 2. SA
>>>>>
>>>>> You've simply deleted all the SA/vmstructs code that referenced 
>>>>> the oops that are no longer present. Does the SA not care about 
>>>>> these things and need access to them? (I don't know hence cc to 
>>>>> serviceability folk.)
>>>>
>>>> Yes, the SA code was never used, so I deleted it.  SA might need in 
>>>> oop inspection to add walking Universe::vm_global() OopStorage area 
>>>> to find where oops come from if there's an error but there doesn't 
>>>> seem to be any other reason for SA to use these oops.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> This has been tested with tier1-3 and on also remote-build -b 
>>>>>> linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero.
>>>>>>
>>>>>> open webrev at 
>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8249768.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8249768
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>