Modify rememberClass to return the offset to the class chain identifying the class #18832

cjjdespres · 2024-01-26T20:22:29Z

The rememberClass function now returns the offset to the class chain that it creates or finds in the local SCC, instead of returning the chain directly. Instead of returning NULL when class remembering fails, it now returns the value 0. The offset of a class chain can never be zero in the local SCC, because that offset is always encoded with encodeOffsetFromEnd by isPointerInSharedCache, which means that the resulting value will always be odd:

openj9/runtime/compiler/env/J9SharedCache.hpp

Lines 363 to 364 in c6df01b

    
           static inline uintptr_t encodeOffsetFromEnd(uintptr_t offset) { return ((offset << 1) | OFFSET_FROM_END); } 
        
           static inline uintptr_t decodeOffsetFromEnd(uintptr_t offset) { return (offset >> 1); }

Various structures that previously stored class chain pointers now instead store offsets instead.

cjjdespres · 2024-01-26T20:24:30Z

Attn @mpirvu, @dsouzai. This is a fairly mechanical change that implements what I talked about in #18821 (comment), though I decided to have rememberClass return the offset directly (or 0) instead of going with a bool return with uintptr_t & input scheme.

dsouzai · 2024-01-26T20:40:25Z

I'll go through the code in more detail as well, but just from a high level overview, I don't think it's a good idea to compare the returned encoded offset against 0; it weakens the conceptual integrity of the API because it exposes the implementation details to the callers of the API.

That said, I do think taking advantage of the fact that the SCC offset is always an offset from the end, and therefore is low tagged, is a valid approach to take.

So, in keeping with the contract that a caller of this API should not directly interact with the offset (which conceptually is a token or key of sorts), it would be better to add another method to J9SharedCache along the lines of offsetOfPointerIntoSCCIsValid(uintptr_t offset). This both hides the implementation details from callers as well as facilitates taking advantage of the low tagging.

cjjdespres · 2024-01-26T22:21:11Z

In a few places we (now) rely on the existence of a class chain offset that is not valid; before, we would just store a NULL pointer in those places. I've added a static const uintptr_t INVALID_CLASS_CHAIN_OFFSET = 0 to TR_SharedCache that we can compare with the return value of rememberClass and also use instead of a bare 0. Does that seem all right?

dsouzai · 2024-01-29T02:44:00Z

In a few places we (now) rely on the existence of a class chain offset that is not valid; before, we would just store a NULL pointer in those places. I've added a static const uintptr_t INVALID_CLASS_CHAIN_OFFSET = 0 to TR_SharedCache that we can compare with the return value of rememberClass and also use instead of a bare 0. Does that seem all right?

Yeah that seems reasonable to me.

dsouzai

LGTM; however, I'll let Marius be the final reviewer/committer.

mpirvu

Overall, it looks good I have a few inline comments though.

mpirvu · 2024-01-28T04:25:06Z

runtime/compiler/env/J9SharedCache.cpp

      }

   if (!create)
      {
      LOG(1, "\tnot asked to create but could create, returning non-null\n");
-      return (uintptr_t *)0x1;
+      return 1;


This needs a symbolic value. Can 1 be a valid offset?

I think that the only time create is false in a rememberClass call is when canRememberClass or one of its overloads is run:

openj9/runtime/compiler/env/J9SharedCache.hpp

Lines 176 to 179 in 00a123d

virtual bool canRememberClass(TR_OpaqueClassBlock *classPtr)

{

return rememberClass((J9Class *)classPtr, NULL, false) != NULL;

}

In those cases we don't care about the offset at all, and the return 1 is just a method of communicating that the chain could have been created. It looks like the intent is to avoid duplicating the rememberClass code in canRememberClass.

I'm not sure if 1 could be a valid offset - judging from isPointerInSharedCache:

openj9/runtime/compiler/env/J9SharedCache.cpp

Line 734 in 00a123d

TR_J9SharedCache::isPointerInSharedCache(void *ptr, uintptr_t *cacheOffset)

that would happen only if a class chain started at exactly the metadataStartAddress of the first layer of an SCC.

I'll add a symbolic constant.

runtime/compiler/env/J9SharedCache.cpp

mpirvu · 2024-01-28T04:49:11Z

runtime/compiler/env/VMJ9.cpp

@@ -7227,6 +7227,19 @@ TR_J9VM::getClassFromSignature(const char * sig, int32_t sigLength, J9ConstantPo
   return returnValue; // 0 means failure
   }

+uint32_t
+TR_J9VMBase::numInterfacesImplemented(J9Class *clazz)


Does this need to be overridden for server fe?

It looks like the cls.iTableOf() and cls.iTableNext() calls handle this properly:

openj9/runtime/compiler/env/J9ClassEnv.cpp

Lines 217 to 241 in 66c4255

J9ITable *

J9::ClassEnv::iTableOf(TR_OpaqueClassBlock * clazz)

{

#if defined(J9VM_OPT_JITSERVER)

if (auto stream = TR::CompilationInfo::getStream())

{

stream->write(JITServer::MessageType::ClassEnv_iTableOf, clazz);

return std::get<0>(stream->read<J9ITable*>());

}

#endif /* defined(J9VM_OPT_JITSERVER) */

return (J9ITable*) self()->convertClassOffsetToClassPtr(clazz)->iTable;

}

J9ITable *

J9::ClassEnv::iTableNext(J9ITable *current)

{

#if defined(J9VM_OPT_JITSERVER)

if (auto stream = TR::CompilationInfo::getStream())

{

stream->write(JITServer::MessageType::ClassEnv_iTableNext, current);

return std::get<0>(stream->read<J9ITable*>());

}

#endif /* defined(J9VM_OPT_JITSERVER) */

return current->next;

}

openj9/runtime/compiler/control/JITClientCompilationThread.cpp

Lines 2043 to 2054 in 00a123d

case MessageType::ClassEnv_iTableOf:

{

auto clazz = std::get<0>(client->getRecvData<TR_OpaqueClassBlock *>());

client->write(response, TR::Compiler->cls.iTableOf(clazz));

}

break;

case MessageType::ClassEnv_iTableNext:

{

auto clazz = std::get<0>(client->getRecvData<J9ITable *>());

client->write(response, TR::Compiler->cls.iTableNext(clazz));

}

break;

so it shouldn't need a server override.

runtime/compiler/compile/AOTClassInfo.hpp

runtime/compiler/env/J9SharedCache.cpp

runtime/compiler/runtime/SymbolValidationManager.cpp

cjjdespres · 2024-01-30T02:17:46Z

Rebasing to pull in the reversion in #18843, and to fix the JITServer version merge conflict. The review comments should all be addressed as well.

runtime/compiler/codegen/J9CodeGenerator.cpp

The length of the class chain associated to a class is relevant to the JITServer AOT cache, as this quantity is used to construct RAMClass chains that are sent from the client to the server. To support the operation of the JITServer AOT cache without a local SCC, this length must always be available.

Signed-off-by: Christian Despres <[email protected]>

cjjdespres · 2024-01-30T18:58:54Z

That should be all of the log messages fixed.

mpirvu

LGTM

mpirvu · 2024-01-30T19:35:04Z

jenkins test sanity all jdk17

mpirvu · 2024-01-31T04:18:22Z

jenkins test sanity aix jdk17

mpirvu · 2024-01-31T13:50:03Z

jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk17

…berclass-change" This reverts commit fe7f576, reversing changes made to c0cbb3e.

cjjdespres requested a review from dsouzai as a code owner January 26, 2024 20:22

cjjdespres force-pushed the rememberclass-change branch 2 times, most recently from 0903e6b to d0cfdd0 Compare January 27, 2024 16:23

dsouzai approved these changes Jan 29, 2024

View reviewed changes

mpirvu reviewed Jan 29, 2024

View reviewed changes

cjjdespres force-pushed the rememberclass-change branch 2 times, most recently from 642d05e to 16c7ebf Compare January 30, 2024 02:15

mpirvu self-assigned this Jan 30, 2024

mpirvu reviewed Jan 30, 2024

View reviewed changes

runtime/compiler/codegen/J9CodeGenerator.cpp Outdated Show resolved Hide resolved

cjjdespres force-pushed the rememberclass-change branch from 16c7ebf to 7e9e21b Compare January 30, 2024 16:09

cjjdespres added 4 commits January 30, 2024 18:47

Store class chain offset in ProfiledClassRecord

057bdf5

Signed-off-by: Christian Despres <[email protected]>

Store class chain offset in TR::ClassByNameRecord

7abd4ec

Signed-off-by: Christian Despres <[email protected]>

Store offset in ClassChainRecord, ClassChainInfo

ec3a37b

Signed-off-by: Christian Despres <[email protected]>

Store class chain offset in AOTClassInfo

21d1c20

Signed-off-by: Christian Despres <[email protected]>

cjjdespres force-pushed the rememberclass-change branch from 7e9e21b to 32ad2f5 Compare January 30, 2024 18:48

cjjdespres added 3 commits January 30, 2024 18:58

Modify rememberClass to return offset

5439842

Signed-off-by: Christian Despres <[email protected]>

Do not use 0 directly as an invalid chain offset

7504142

Signed-off-by: Christian Despres <[email protected]>

Bump JITServer version

528739f

Signed-off-by: Christian Despres <[email protected]>

cjjdespres force-pushed the rememberclass-change branch from 32ad2f5 to 528739f Compare January 30, 2024 18:58

mpirvu approved these changes Jan 30, 2024

View reviewed changes

mpirvu merged commit fe7f576 into eclipse-openj9:master Jan 31, 2024
21 checks passed

nbhuiyan mentioned this pull request Feb 15, 2024

InternalError: mh1 != mh2 #18882

Closed

cjjdespres added a commit to cjjdespres/openj9 that referenced this pull request Feb 16, 2024

Revert "Merge pull request eclipse-openj9#18832 from cjjdespres/remem…

0e75b47

…berclass-change" This reverts commit fe7f576, reversing changes made to c0cbb3e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify rememberClass to return the offset to the class chain identifying the class #18832

Modify rememberClass to return the offset to the class chain identifying the class #18832

cjjdespres commented Jan 26, 2024

cjjdespres commented Jan 26, 2024

dsouzai commented Jan 26, 2024 •

edited

Loading

cjjdespres commented Jan 26, 2024

dsouzai commented Jan 29, 2024

dsouzai left a comment

mpirvu left a comment

mpirvu Jan 28, 2024

cjjdespres Jan 29, 2024

cjjdespres Jan 29, 2024

mpirvu Jan 28, 2024

cjjdespres Jan 29, 2024

cjjdespres commented Jan 30, 2024

cjjdespres commented Jan 30, 2024

mpirvu left a comment

mpirvu commented Jan 30, 2024

mpirvu commented Jan 31, 2024

mpirvu commented Jan 31, 2024

	static inline uintptr_t encodeOffsetFromEnd(uintptr_t offset) { return ((offset << 1) \| OFFSET_FROM_END); }
	static inline uintptr_t decodeOffsetFromEnd(uintptr_t offset) { return (offset >> 1); }

	virtual bool canRememberClass(TR_OpaqueClassBlock *classPtr)
	{
	return rememberClass((J9Class *)classPtr, NULL, false) != NULL;
	}

	J9ITable *
	J9::ClassEnv::iTableOf(TR_OpaqueClassBlock * clazz)
	{
	#if defined(J9VM_OPT_JITSERVER)
	if (auto stream = TR::CompilationInfo::getStream())
	{
	stream->write(JITServer::MessageType::ClassEnv_iTableOf, clazz);
	return std::get<0>(stream->read<J9ITable*>());
	}
	#endif /* defined(J9VM_OPT_JITSERVER) */
	return (J9ITable*) self()->convertClassOffsetToClassPtr(clazz)->iTable;
	}

	J9ITable *
	J9::ClassEnv::iTableNext(J9ITable *current)
	{
	#if defined(J9VM_OPT_JITSERVER)
	if (auto stream = TR::CompilationInfo::getStream())
	{
	stream->write(JITServer::MessageType::ClassEnv_iTableNext, current);
	return std::get<0>(stream->read<J9ITable*>());
	}
	#endif /* defined(J9VM_OPT_JITSERVER) */
	return current->next;
	}

	case MessageType::ClassEnv_iTableOf:
	{
	auto clazz = std::get<0>(client->getRecvData<TR_OpaqueClassBlock *>());
	client->write(response, TR::Compiler->cls.iTableOf(clazz));
	}
	break;
	case MessageType::ClassEnv_iTableNext:
	{
	auto clazz = std::get<0>(client->getRecvData<J9ITable *>());
	client->write(response, TR::Compiler->cls.iTableNext(clazz));
	}
	break;

Modify rememberClass to return the offset to the class chain identifying the class #18832

Modify rememberClass to return the offset to the class chain identifying the class #18832

Conversation

cjjdespres commented Jan 26, 2024

cjjdespres commented Jan 26, 2024

dsouzai commented Jan 26, 2024 • edited Loading

cjjdespres commented Jan 26, 2024

dsouzai commented Jan 29, 2024

dsouzai left a comment

Choose a reason for hiding this comment

mpirvu left a comment

Choose a reason for hiding this comment

mpirvu Jan 28, 2024

Choose a reason for hiding this comment

cjjdespres Jan 29, 2024

Choose a reason for hiding this comment

cjjdespres Jan 29, 2024

Choose a reason for hiding this comment

mpirvu Jan 28, 2024

Choose a reason for hiding this comment

cjjdespres Jan 29, 2024

Choose a reason for hiding this comment

cjjdespres commented Jan 30, 2024

cjjdespres commented Jan 30, 2024

mpirvu left a comment

Choose a reason for hiding this comment

mpirvu commented Jan 30, 2024

mpirvu commented Jan 31, 2024

mpirvu commented Jan 31, 2024

dsouzai commented Jan 26, 2024 •

edited

Loading