Skip to content

OpenJ9 support for new OMR compiler tracing and logging infrastructure #22751

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
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

OpenJ9 support for new OMR compiler tracing and logging infrastructure #22751

wants to merge 10 commits into from
+7,484 −9,146

Conversation

@0xdaryl
Copy link
Contributor

@0xdaryl 0xdaryl commented Oct 8, 2025

This PR contains the OpenJ9 JIT changes required to support the new OMR compiler tracing and logging infrastructure (eclipse-omr/omr#7978). See the OMR PR for details.

This has a breaking dependency on eclipse-omr/omr#7978 and requires a coordinated merge.
@0xdaryl 0xdaryl requested a review from dsouzai as a code owner October 8, 2025 19:38
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Oct 8, 2025

Tagging the same cohort from the corresponding OMR PR.

FYI @mstoodle @vijaysun-omr @mpirvu @zl-wang @dsouzai @ymanton @jdmpapin @knn-k @gita-omr @hzongaro @r30shah @JamesKingdon @klangman
dsouzai
dsouzai reviewed Oct 16, 2025
View reviewed changes
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just had a couple of questions.

I see some places where a traceMsg is replaced with if (trace) log->printf; why not use trprintf(trace, log, ...)? I thought doing if (trace) was supposed to be an anti-pattern?
runtime/compiler/x/codegen/J9TreeEvaluator.cpp
Comment on lines -1173 to -1185
if (debug("traceConditionalHelperEvaluator"))
{
diagnostic("conditionalHelperEvaluator: Adding deps from " POINTER_PRINTF_FORMAT "\n", cursor);
}
for (int32_t i = 0; i < cursorDeps->getNumPostConditions(); i++)
{
TR::RegisterDependency *cursorPostCondition = cursorDeps->getPostConditions()->getRegisterDependency(i);
postConditions->unionPostCondition(cursorPostCondition->getRegister(), cursorPostCondition->getRealRegister(), cg);
if (debug("traceConditionalHelperEvaluator"))
{
TR_Debug *debug = cg->getDebug();
diagnostic("conditionalHelperEvaluator: [%s : %s]\n", debug->getName(cursorPostCondition->getRegister()), debug->getName(machine->getRealRegister(cursorPostCondition->getRealRegister())));
}
Copy link
Contributor

@dsouzai dsouzai Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these removed?
Copy link
Contributor Author

@0xdaryl 0xdaryl Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the early goals of this effort was to deprecate the diagnostic macros. They are only available in DEBUG mode which nobody builds with anymore and would frankly have to derive from first principles even how to do it. The effort involves not just deleting the diagnostic macros, but reasoning about which ones might actually be useful and then "promoting" them to the production build.

However, that goal became secondary as this work evolved. OpenJ9 took an initial stab at a few obvious diagnostic macros in one of its commits. An initial diagnostic cleanup commit is still present in this PR, though it is not an exhaustive cleanup.

In this particular instance in the X codegen, I considered the utility of the information conveyed by this diagnostic macro and how often one may have needed it in the past 9 years.

I could remove the diagnostic cleanup commit from this PR and save it for later. I'll defer to your opinion.
Copy link
Contributor

@dsouzai dsouzai Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could remove the diagnostic cleanup commit from this PR and save it for later. I'll defer to your opinion.

No I think it's fine to leave in this PR, just wanted to make sure there was a good reason for it is all.
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 1, 2025

I see some places where a traceMsg is replaced with if (trace) log->printf; why not use trprintf(trace, log, ...)? I thought doing if (trace) was supposed to be an anti-pattern?

Probably just an oversight. I found a couple of these places and will fix in next force push. Thanks for pointing that out.
hzongaro
hzongaro approved these changes Nov 3, 2025
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for commit 0ce5f35 has a line longer than 72 characters. I think the changes look good otherwise, so I'll go ahead and approve. Thanks!
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 3, 2025

Jenkins test sanity all jdk21 depends eclipse-omr/omr#7978
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 3, 2025

Jenkins test sanity all jdk21 depends eclipse-omr/omr#7978
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 4, 2025 •
edited
Loading

Linux x86-64 failure is #19047
0xdaryl added 10 commits November 10, 2025 15:03
@0xdaryl
Initial OMR::Logger support
18376cb 
* Use OMR::Logger in place of TR::FILE * Replace uses of trfprintf with equivalent OMR::Logger function Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Add JitDump Logger support and improve code readability
dc25555 
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Replace traceMsg() with direct log updates
3bb9779 
Guard tracing with checks for tracing enabled if missing. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Use conditional logger macros to make code more readable
27bad1e 
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Initial cleanup of diagnostic() macros
bb6dfc5 
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Replace Debug::trace() with OMR::Logger
3683038 
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Replace some uses of getLoggingEnabled()
a00fa1d 
Either guard with an appropriate tracing option (preferred) or use `isEnabled_DEPRECATED()` in situations that require non-trivial refactoring. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Support caching of log file and logger in Compilation object
7b21a5f 
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Ensure Logger is initialized in SVM_ASSERT_IMPL
468545a 
The assert may fire before a Logger is installed on the Compilation object. Check if the Logger exists before logging. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Use TRIOStreamLogger as default OpenJ9 logger
e6602fd 
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 10, 2025

The latest force push uses TR::IO for Loggers in OpenJ9. This is how tracing and logging is currently implemented in the JIT. While it is not as efficient as direct C stdio, I realized if we used C stdio immediately there would be a discrepancy between how the underlying log file is created and opened (still with TR::IO) and how updates to the log file are done (with C stdio). When you peel back the many layers, C stdio will ultimately be used even for TR::IO (which is why it works), but it is not obvious.

In a subsequent PR, I will do work to clean this up and address this difference. In the meantime, for the purposes of this initial Logger implementation, fall back to using TR::IO.

Usage of C stdio Loggers can be forced in OpenJ9 with a new -Xjit:forceCStdIOForLoggers option.
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 10, 2025

I would like this merged AFTER the 0.57 code split, currently targeted for Nov. 14, 2025.
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 11, 2025

Jenkins test sanity all jdk21 depends eclipse-omr/omr#7978
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 11, 2025

Internal testing on 32-bit platforms for JDK 8 was successful.
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 11, 2025

x86-64 Linux failure is #10847.
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 11, 2025

Windows failures appear test-setup related (shared classes) and both failures passed on re-run in a grinder.
@hzongaro hzongaro self-assigned this Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:jit depends:omr:breaking

3 participants

@0xdaryl @dsouzai @hzongaro