RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v2]

[Daniel D.Daugherty]

On Mon, 14 Sep 2020 18:35:29 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> @kimbarrett:
>> 
>>> src/hotspot/share/oops/weakHandle.cpp
>>> 36 WeakHandle::WeakHandle(OopStorage* storage, oop obj) :
>>> 37 _obj(storage->allocate()) {
>>> 38 assert(obj != NULL, "no need to create weak null oop");
>>> 
>>> Please format this differently so the ctor-init-list is more easily
>>> distinguished from the body. I don't care that much which of the several
>>> alternatives is used.
>> 
>> After discussion with Erik, I changed the indent on L37 from two space to four spaces.
>
> @kimbarrett
> 
>> src/hotspot/share/runtime/objectMonitor.cpp
>> 244 // Check that object() and set_object() are called from the right context:
>> 245 static void check_object_context() {
>> 
>> This seems like checking we would normally only do in a debug build. Is this
>> really supposed to be done in product builds too? (It's written to support
>> that, just wondering if that's really what we want.) Maybe these aren't
>> called very often so it doesn't matter? I also see that guarantee (rather
>> than assert) is used a fair amount in this and related code.
> 
> I've changed check_object_context() to only be defined and called
> when ASSERT is defined. I've also changed the guarantee() calls
> to assert() calls.
> 
> I've done a couple of Mach5 Tier[1-8] test cycles on this code so I'm
> no longer worried about this code or its callers in release bits.

@kimbarrett

> src/hotspot/share/runtime/objectMonitor.cpp
> 249 guarantee(self->is_Java_thread() || self->is_VM_thread(), "must be");
> 250 if (self->is_Java_thread()) {
> 
> Maybe instead
> 
> if (self->is_Java_thread()) {
> ...
> } else {
> guarantee(self->is_VM_thread(), "must be");
> }

I've made this refactoring change, tweaked the comments above
the block a bit and switched from guarantee() to assert().

-------------

PR: https://git.openjdk.java.net/jdk/pull/135