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

[coleen.phillimore at oracle.com]

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