Refactor the Deserializer to use a helper struct #22135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

dsouzai wants to merge 1 commit into eclipse-openj9:master from dsouzai:deserializerHelper

+213 −172

Contributor

dsouzai commented Jun 24, 2025

To faciltate a non-compilation thread invoking the deserializer, which can happen outside the context of a compilation, the APIs in the Deserializer need to be updated to no longer require a TR::Compilation object directly.

dsouzai added the comp:jitserver label

Contributor Author

dsouzai commented Jun 24, 2025

@mpirvu could you review? I couldn't think of a better name than DeserializerHelper, but I'm happy to change it to something better.

mpirvu added this to JIT as a Service

github-project-automation bot moved this to In progress in JIT as a Service

Contributor

mpirvu commented Jun 24, 2025

I couldn't think of a better name than DeserializerHelper, but I'm happy to change it to something better.

I suggest DeserializerContext. Also, maybe we should create two constructors: one that only needs the comp object and the other one that needs vmThread, fe and trMemory.

I am in the process of reviewing the PR.

mpirvu reviewed

View reviewed changes

runtime/compiler/control/JITClientCompilationThread.cpp Outdated

                        if (auto deserializer = compInfo->getJITServerAOTDeserializer())
                           {
                           bool wasReset = false;
-                          deserializer->cacheRecords((const uint8_t *)recordsStr.data(), recordsStr.size(), comp,
+                          auto helper = DeserializerHelper(comp->j9VMThread(), comp->fej9(), comp->trMemory(), comp);
+                          deserializer->cacheRecords((const uint8_t *)recordsStr.data(), recordsStr.size(), &helper,

Contributor

mpirvu Jun 24, 2025

Wouldn't be better for the helper param to be provided as a const reference?

runtime/compiler/runtime/JITServerAOTDeserializer.cpp Outdated

                  {
                  ++_numDeserializationFailures;
                  if (TR::Options::getVerboseOption(TR_VerboseJITServer))
                     TR_VerboseLog::writeLineLocked(TR_Vlog_JITServer,
                        "ERROR: Failed to deserialize AOT method %s%s",
-                       comp->signature(), wasReset ? " due to concurrent deserializer reset" : ""
+                       helper->_comp->signature(), wasReset ? " due to concurrent deserializer reset" : ""

Contributor

mpirvu Jun 24, 2025

In the new world _comp may not exist if I understood correctly the purpose of this commit. Then we must take that into account and test for it.

runtime/compiler/runtime/JITServerAOTDeserializer.cpp Outdated

                  if (TR::Options::getVerboseOption(TR_VerboseJITServer))
-                    TR_VerboseLog::writeLineLocked(TR_Vlog_JITServer, "Deserialized AOT method %s", comp->signature());
+                    TR_VerboseLog::writeLineLocked(TR_Vlog_JITServer, "Deserialized AOT method %s", helper->_comp->signature());

Contributor

mpirvu Jun 24, 2025

_comp may not exist.

runtime/compiler/runtime/JITServerAOTDeserializer.cpp Outdated

    
                 auto fej9vm = comp->fej9vm();

                 void *thunk = fej9vm->getJ2IThunk((char *)record->signature(), record->signatureSize(), comp);

                 auto fej9vm = helper->_fej9;

                 void *thunk = fej9vm->getJ2IThunk((char *)record->signature(), record->signatureSize(), helper->_comp);

Contributor

mpirvu Jun 24, 2025

Can _comp be NULL?

runtime/compiler/runtime/JITServerAOTDeserializer.cpp Outdated

                                  "Invalid TR_AOTMethodHeader version: %d.%d", header->majorVersion, header->minorVersion);
                  TR_ASSERT_FATAL((header->offsetToRelocationDataItems != 0) || (method->numRecords() == 0),
                                  "Unexpected %zu serialization records in serialized method %s with no relocation data",
-                                 method->numRecords(), comp->signature());
+                                 method->numRecords(), helper->_comp->signature());

Contributor

mpirvu Jun 24, 2025

Can _comp be NULL?

runtime/compiler/runtime/JITServerAOTDeserializer.cpp Outdated

                  if (wasReset)
-                    comp->failCompilation<J9::AOTDeserializerReset>("Deserializer reset during relocation of method %s",
-                                                                    comp->signature());
+                    helper->_comp->failCompilation<J9::AOTDeserializerReset>("Deserializer reset during relocation of method %s",

Contributor

mpirvu Jun 24, 2025

Can _comp be NULL? Similar question for other places.

Contributor

mpirvu commented Jun 25, 2025

Thinking more about this one: deep inside the deserializer routines it may be hard to determine whether the comp object is valid or not. Maybe the routines that that will be accessed from a non-compilation threads should have a DeserializerContext as parameter and everything else should have a comp object.

dsouzai force-pushed the deserializerHelper branch from 05ebeac to 75e7a80 Compare

June 26, 2025 15:59

Contributor Author

dsouzai commented Jun 26, 2025

I suggest DeserializerContext. Also, maybe we should create two constructors: one that only needs the comp object and the other one that needs vmThread, fe and trMemory.

Updated in force push.

Thinking more about this one: deep inside the deserializer routines it may be hard to determine whether the comp object is valid or not. Maybe the routines that that will be accessed from a non-compilation threads should have a DeserializerContext as parameter and everything else should have a comp object.

It turns out that most of the comp object accesses were just for the signature. There were only two places that required the comp object, and both scenarios do not make sense outside of a compilation (related to thunk serialization and generated class). So I just put TR_ASSERT_FATALs in those locations.

Wouldn't be better for the helper param to be provided as a const reference?

Ah missed this; yeah I can make the update.

Contributor Author

dsouzai commented Jun 26, 2025

Maybe the routines that that will be accessed from a non-compilation threads should have a DeserializerContext as parameter and everything else should have a comp object.

It's also really hard to know which routines are accessed by a non-comp thread because there are so many helper routines; also the fact that there's more than one type of deserializer really complicates matters.

dsouzai force-pushed the deserializerHelper branch from 75e7a80 to 5e18e8d Compare

June 26, 2025 16:09

Contributor Author

dsouzai commented Jun 26, 2025

Wouldn't be better for the helper param to be provided as a const reference?

Ah missed this; yeah I can make the update.

Updated in force push.

dsouzai marked this pull request as draft

June 26, 2025 16:10

Contributor Author

dsouzai commented Jun 26, 2025

Converted to draft because I accidentally modified code unrelated to this PR (in trying to convert helper to context).


          Refactor the Deserializer to use a helper struct

026876d

To faciltate a non-compilation thread invoking the deserializer, which
can happen outside the context of a compilation, the APIs in the
Deserializer need to be updated to no longer require a TR::Compilation
object directly.

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

dsouzai force-pushed the deserializerHelper branch from 5e18e8d to 026876d Compare

June 26, 2025 16:19

dsouzai marked this pull request as ready for review

June 26, 2025 16:21

Contributor Author

dsouzai commented Jun 26, 2025

Ok, good for review again.

dsouzai mentioned this pull request

Support AOT Dependencies in JITServer #22014

Open

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels