Hi Coleen,
On 22/07/2020 12:19 am, coleen.phillim...@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.phillim...@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
