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

[David Holmes]

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?

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.

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
>