On 7/22/20 8:42 AM, David Holmes wrote:
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.
I actually understand this, but the suggested names don't help. You
really need to look at the code and the comments in universe.hpp to see
the distinction. I don't think we can provide more illumination with
long names. Since I moved the functions next to each other, it makes
more sense when one reads it.
But lets' just add the 's' and move on. :)
Thanks, I added the 's' and fixed the formatting. Thank you for
reviewing this.
Coleen
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
