On 22/07/2020 9:50 pm, coleen.phillim...@oracle.com wrote:
On 7/22/20 2:32 AM, David Holmes wrote:
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?
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
