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

[David Holmes]

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.

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

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