JITServer: handle non-null compilation - #23846
Merged
Merged
Conversation
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
May 5, 2026 18:58
c9c432d to
4628208
Compare
mpirvu
self-requested a review
May 5, 2026 19:01
mpirvu
requested changes
May 13, 2026
Contributor
There was a problem hiding this comment.
I don't agree with the solution in this PR. The block of code that supposedly throws the StreamFailure exception you are trying to catch cannot be executed on the server. The block is protected by if (entry->isAotLoad() and AOT loads are only executed at the client. Moreover, the function that supposedly causes the StreamFailure has been redefined at JITServer:
virtual uint16_t getAllEnabledHints(J9Method *method) override
{
TR_ASSERT_FATAL(false, "called");
return 0;
}
so we know it cannot be executed.
The high level pseudocode of the code is like this:
try {
preCompilationTasks()
compile()
postCompilationTasks()
} catch {
TR_ASSERT_FATAL(comp==NULL)
postCompilationTasks()
}
It's possible that there is an exception thrown from postCompilationTasks(), but it's not caused by getAllEnabledHints. compilationEnd() is called from postCompilationTasks() and that's a big method. Maybe that one could throw?
Contributor
|
|
Contributor
|
It's very likely that there is an exception being generated in postCompilationTasks(). The comments for that method say: so we should not allow any exception to escape, but at the same time we cannot put a try/catch block around the entire method because we must execute all cleanup actions that postCompilationTasks() is supposed to do.
However, the methods above already have try/catch blocks around them, so they are not the culprits. |
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
2 times, most recently
from
June 11, 2026 18:03
a0abbf0 to
d94b94d
Compare
mpirvu
reviewed
Jun 11, 2026
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 11, 2026 18:59
d94b94d to
219a64d
Compare
mpirvu
reviewed
Jun 11, 2026
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 11, 2026 19:07
219a64d to
ef157f6
Compare
mpirvu
reviewed
Jun 11, 2026
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 11, 2026 19:52
ef157f6 to
bc35349
Compare
Contributor
|
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21 |
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 12, 2026 15:05
bc35349 to
7fdc06e
Compare
Contributor
|
jenkins test sanity,extended plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21 |
Contributor
Author
|
The following test suite had three errors jdk_collections_0 jdk_stream_0 |
Contributor
Author
|
The following test suite had one error |
Contributor
Author
|
The following test suite had 3 errors jdk_stream_0 jdk_security2_0 |
Contributor
Author
|
It appears that the build failures in this test suite all have the same common cause. |
Contributor
|
At least one of the aborted tests, extended.functional on xlinux, had multiple tests that experienced a timeout. The timeouts generate coredumps which fill the disk space. |
Contributor
|
jenkins test sanity.functional xlinux,xlinuxjit jdk21 |
Contributor
Author
|
Related, this Grinder I ran to test the issue has completed 75 out of 100 iterations of the original test case without any errors being thrown |
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 16, 2026 20:24
7fdc06e to
919d522
Compare
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
3 times, most recently
from
June 17, 2026 14:11
0aa2bad to
2cc5681
Compare
Contributor
Author
|
Grinder with 100 iterations: |
mpirvu
reviewed
Jun 17, 2026
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
3 times, most recently
from
June 17, 2026 20:51
9cf62db to
23c9130
Compare
Contributor
Author
|
New Grinder with the updated implementation |
mpirvu
reviewed
Jun 18, 2026
| TRIGGER_J9HOOK_JIT_COMPILING_END(_jitConfig->hookInterface, vmThread, method); | ||
|
|
||
| if (_compiler && _methodBeingCompiled->isOutOfProcessCompReq()) { | ||
| // If JITServer is enabled, this will send messages |
Contributor
There was a problem hiding this comment.
Suggested re-phrasing:
// In JITServer mode preemtively call _compiler->retainedMethods() to
// create the retainedMethod set if it doesn't exist. If this operation
// is not done now, it will be done later during:
// postCompilationTasks
// - compilationEnd
// - outOfProcessCompilationEnd
// - computeDataForCHTableCommit
// - getDataForClient
// The server will send a message to the client, which could cause an
// exception and exceptions are not allowed to be triggerred from postCompilationTasks()
| bool useServerOffsets = aotCacheStore && clientData->useServerOffsets(entry->_stream); | ||
| bool useServerOffsets = aotCacheStore; | ||
| try { | ||
| // in the case useServerOffsets triggers an message |
Contributor
There was a problem hiding this comment.
If using full sentences, the sentence must start with a capital letter.
Contributor
There was a problem hiding this comment.
Suggested re-phrasing:
Guard useServerOffsets() with a try block to prevent exceptions from
interrupting the flow of postCompilationTasks() at a higher level.
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 19, 2026 16:20
23c9130 to
8e96082
Compare
mpirvu
requested changes
Jun 19, 2026
When a JITServer stream failure is thrown, sometimes setting the compilation object to NULL can be skipped to head straight into exception handling. This causes an assert to be tripped. In this particular case, there is a front-end call in outOfProcessCompilationEnd (within computeDataForCHTableCommit) which can cause a stream error. Since it isn't guarded by a global try-catch, it results in the given assert being tripped. Addresses this issue: eclipse-openj9#23845
KavinSatheeskumar
force-pushed
the
jitserver-assert
branch
from
June 19, 2026 19:43
8e96082 to
51a5cc0
Compare
Contributor
|
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk25 |
Contributor
|
xlinux had one failure: for testJITServer_0 10x grinder passed: https://openj9-jenkins.osuosl.org/job/Grinder/5237/ |
Contributor
|
Since all tests passed (the xlinux one was infra related), this PR is ready to be merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a JITServer stream failure is thrown, sometimes setting
the compilation object to NULL can be skiped to head straight
into exception handling. This causes an assert to be tripped.
In this particular case, there is a front-end call in
postCompilationTasks which can cause a stream error. Since it
isn't guarded by a global try-catch, it results in the given
assert being tripped.
Addresses this issue:
#23845