Add API getDefaultValueSlotAddress

[a7ehuo]

getDefaultValueSlotAddress() returns the reference to the address of the default value instance for a value class.

[edited]
If the class is initialized, devalue value instance for the class is allocated. When generating aconst_init, we can load from the default value slot.

- [x] Depends on eclipse/omr#6530

[a7ehuo]

This change depends on

• Add defaultValueSymbol for value type class eclipse/omr#6530

@hzongaro @0xdaryl May I ask you to review? Thanks!

[a7ehuo]

@mpirvu May I ask you to review the commit ccb6fde that adds API getDefaultValueSlotAddress? That commit has code related to JITServer. Thanks!

[mpirvu]

Could you please update the MINOR_NUMBER as well in CommunicationStream.hpp ?
Also, is the value returned by vm->internalVMFunctions->getDefaultValueSlotAddress(j9class) immutable for a class? If it cannot change, we can cache it per class.

[0xdaryl]

Just so we're clear: OMR 6530 can merge on its own, can't it? Or do you require a coordinated merge?

[a7ehuo]

is the value returned by vm->internalVMFunctions->getDefaultValueSlotAddress(j9class) immutable for a class?

It's immutable. I'll cache it per class

[a7ehuo]

OMR 6530 can merge on its own, can't it?

eclipse/omr#6530 should be merged first on its own. No coordinated merge is required

[a7ehuo]

There are two more dependencies for this PR

• 1. Update the value of ACC_VALUE and remove j.l.IdentityObject #15130
• 2. Make getDefaultValueSlotAddress available on non-valhalla builds #15132

[a7ehuo]

is the value returned by vm->internalVMFunctions->getDefaultValueSlotAddress(j9class) immutable for a class?

@mpirvu Sorry while I'm updating the code to cache the value, I realized that caching might not work because flattenedClassCache->defaultValue is not initialized until the class is initialized. When JITServerHelpers::getAndCacheRAMClassInfo is called at other places to cache ClientSessionData::ClassInfo, the default value instance for the class might not be allocated yet.

[mpirvu]

There is at least one other place in the code where we have a similar issue. I'll try to find that example.
The idea is to check whether the class is initialized (I believe that the server knows this) and if it's not, we ask the client for the answer. At the same time the client can answer if the class become initialized, in which case caching becomes possible.

[mpirvu]

ClassInfo._classInitialized field can be used to test whether the caches class is initialized.

The example I was referring to above is: TR_J9ServerVM::canAllocateInlineClass(TR_OpaqueClassBlock *clazz)
Here, we first test if class is initialized, if not we send a message specifically to find this information (and cache it if the class has become initialized) then proceed with the rest of the method.

[mpirvu]

I see that the code in TR_J9ByteCodeIlGenerator::genAconst_init already has a test for comp()->fej9()->isClassInitialized(TR::Compiler->cls.convertClassPtrToClassOffset(clazz).
If the defaultvalueSlotAddress is not valid before the class gets initialized, is it worth adding a FATAL_ASSERT in J9::ClassEnv::getDefaultValueSlotAddress to make sure that the class is initialized (to prevent future mistakes)?

[a7ehuo]

Thanks @mpirvu! I'll update the code to retrieve defaultValueSlotAddress from the client if it's NULL on the server side. Otherwise, return the cached value.

When class info is cached, the class might not have been initialized and the defaultValueSlotAddress is cached as NULL. defaultValueSlotAddress() is only called at the server if TR_J9ServerVM::isClassInitialized() returns true. But it doesn't mean defaultValueSlotAddress is valid because isClassInitialized could be cached as FALSE first then updated later when canAllocateInlineClass() is called. So

• if defaultValueSlotAddress is NULL, check the client and update the cached value on the server side
• if defaultValueSlotAddress is non-NULL, return the cached value

[a7ehuo]

@mpirvu @hzongaro Addressed the above comments. Ready for another review. Thanks!

[mpirvu]

LGTM

[hzongaro]

Thanks, Annabelle. I think this looks good overall. Just one question about whether setCanCGandReturn should be getting set on the default value symbol reference.

[a7ehuo]

Change the status to WIP to explore other possibilities of the implementation in ILGen after offline discussion

[a7ehuo]

@hzongaro @mpirvu Just a heads up. I'm planning to use this PR to only submit the first commit "Add API getDefaultValueSlotAddress". The reason is that I have found that the other two commits require more factoring and AOT implementation which will require OMR change as well. I'll put them in another PR when it's ready.

I'll clean up the branch used in this PR and push another commit shortly.

[a7ehuo]

@hzongaro @mpirvu As mentioned on the above, I've cleaned up this branch to include only the first commit that adds the classEnv API getDefaultValueSlotAddress(). It's ready for review. Thank you!

[hzongaro]

I think the changes look good. Thanks!

[hzongaro]

Marius @mpirvu, I just thought I'd follow up - do you have any further comments?

[mpirvu]

I see that the JITServer related files have not been modified since my last review, so I ok with the changes

[hzongaro]

Jenkins test sanity all jdk17

[hzongaro]

Jenkins test sanity xlinuxval jdknext