-
Notifications
You must be signed in to change notification settings - Fork 409
Overhaul compiler tracing and logging infrastructure #7978
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.
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
base: master
Are you sure you want to change the base?
Conversation
FYI @mstoodle @vijaysun-omr @mpirvu @zl-wang @dsouzai @ymanton @jdmpapin @knn-k @kevindean12 @gita-omr @hzongaro @r30shah @JamesKingdon @klangman While seemingly a large PR, the bulk of this PR consists of refactored changes to the existing tracing/diagnostic/logging sites to use the new logging infrastructure. The main new contributions can be found in |
@0xdaryl This looks good to me overall. One point of clarification, just to make sure I'm following and can make the right choices about what I do and don't want to override downstream: it looks like (absent any downstream overrides of |
Yes, that's right. I had forgotten (because it has been a while since I looked at that part of the code :-) ) that it will wrap the |
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
* TR_TraceTreeVerification * TR_TraceBlockVerification * TR_TraceCFGVerification * TR_TraceNodeRefCountVerification Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Install default TR::Logger in TR::Options. Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Great! Thanks for the confirmation. |
Jenkins build all |
Guard tracing with checks for tracing enabled if missing. Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
The shorter `log()` function does not pollute the code visually as much as the longer `getLogger()`. Replace uses of OMR::Compilation::getLogger() with log() Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
…gger Signed-off-by: Daryl Maier <[email protected]>
Do not reach into the Options object each time the log pointer or logger object is required. They do not change once the Compilation object is created, so cache them for efficiency. Signed-off-by: Daryl Maier <[email protected]>
Signed-off-by: Daryl Maier <[email protected]>
Jenkins build all Fixed the missing include on the AIX build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM; I have one minor question.
int32_t TR::AssertingLogger::printf(const char *format, ...) | ||
{ | ||
TR_ASSERT_FATAL(false, "Unexpected Logger printf"); | ||
return -1; | ||
} | ||
|
||
int32_t TR::AssertingLogger::prints(const char *string) | ||
{ | ||
TR_ASSERT_FATAL(false, "Unexpected Logger prints"); | ||
return -1; | ||
} | ||
|
||
int32_t TR::AssertingLogger::printc(char c) | ||
{ | ||
TR_ASSERT_FATAL(false, "Unexpected Logger printc"); | ||
return -1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be useful to actually print out the data passed to this logger in the assert?
This PR begins to overhaul the diagnostic tracing and logging infrastructure in
the compiler component. Much of the motivation and proposed design was
presented at the Oct 24, 2024 OMR Architecture Meeting [1] and interested
parties are encouraged to watch that presentation for more background.
[1] https://www.youtube.com/watch?v=n9A0-2p-x8o
Further changes to improve the performance and functionality of the diagnostic
capabilities of the compiler component using this PR as a baseline will come
later.
NOTE: This is a breaking change for projects consuming the OMR compiler component
that use the existing tracing and logging infrastructure. Some work (tedious, but not
difficult) will be required to adapt to the new APIs. See below for details.
I intend to communicate with all known downstream consumers of the OMR compiler
informing them of the change and offering assistance with migration if
necessary (or, as in the case of OpenJ9, doing the work outright). My goal is
to have this PR merged with at least one month's grace before the next Eclipse
OMR release.
Goals (non-exhaustive)
Methods are significantly slower to compile when logging is enabled (e.g.,
4x-20x in some measurements), and logged methods are a significant source of
non-determinancy when investigating problems.
diagnostic reporting operations consume far more memory than is necessary, and
much of this memory is not reclaimed during a compilation which affects
non-diagnostic operations and optimizations in the compiler.
tracing and logging.
used with more than just the log file on the
TR::Compilation
objectdiagnostic functions.
Contributions
Introduces a
TR::Logger
abstract class declaring a lightweight logging APIand a family of useful logger implementations. Detailed documentation for
Loggers can be found in doc/compiler/debug/Loggers.md
Provides several implementations of Loggers for various purposes and situations,
some of which (for example, the
CStdIOStreamLogger
) with a significantly shorterpathlength for writing diagnostic content than the current implementation.
Replaces
TR::FILE
withTR::Logger
as the currency for tracing andlogging. Ensures a
TR::Logger
object is passed to all diagnostic functions.Currently, many diagnostic functions make an assumption that their output will
go to the log file found on the
TR::Compilation
object. This makes itdifficult to support creative debugging techniques (such as dumping a data
structure to an alternate file or simply stdio) because the destination is
fixed. Many diagnostic functions accept
TR::FILE
pointers for the outputdestination, but this limits their output to a file.
To improve versatility, this PR ensures all diagnostic functions accept a
TR::Logger
parameter when appropriate. This is in lieu of anyTR::FILE
parameters that may currently be passed. To improve consistency in the APIs,
the parameter is nearly always passed as the first parameter. There are
circumstances where a
TR::Logger
is not passed, but only in situations wherethe logging destination can be obviously derived from the context.
Deprecates and removes where possible existing redundant diagnostic output
mechanisms.
There are several means in use for producing diagnostic output. These include:
traceMsg()
macroTR_Debug::trace()
functiondiagnostic()
macro (for DEBUG builds only)trfprintf()
/TR::IO::fprintf()
family of functionsfprintf()
family of functions (CPP runtime)All uses of these diagnostic functions or macros have been scrutinized, and
most have been converted to a Logger equivalent. Their usage is deprecated,
and in some cases have been removed entirely from the code base.
Ensures all diagnostic output is guarded with a trace option.
Currently, there are many diagnostic artifacts written unconditionally to the
log file. This needlessly increases the size of a log file and incurs an
overhead cost, and there is no way for a user to suppress it. Such output is
also often provided without needed context, which greatly diminishes its
usefulness.
With this PR, all diagnostic output is now guarded with a trace option.
A common pattern for guarding a single output function is the following:
To improve code readability and density, convenience macros have been introduced
for the print family of functions that should be used in lieu of the pattern
above. For example, the code above is equivalent to:
If multiple output functions share a single guard then the macros should not be
used. This PR uses the convenience macros where appropriate.
With the introduction of loggers, it is possible to catch unguarded output by
introducing an "asserting logger" that can be installed under control of a
debug option. This will generate a JIT assert if any of printing functions are
called, allowing a developer to identify unguarded diagnostic output. Such a
logger is also useful to add to a testing mode of a project that the OMR
compiler is built into so that detecting unguarded output becomes part of its
regular testing.
Makes guarding diagnostic output by the mere existence of a log file an
anti-pattern.
There are currently many places in the JIT where diagnostic output is
controlled by the mere presence or absence of a log file. This is an
anti-pattern and should be replaced with a proper trace option guard. This PR
converts many of such checks to trace options, however some still remain in the
code due to complexities in refactoring. These should all be eliminated in
future PRs.
Chooses the most efficient API function for the information to output.
This PR introduces specialized output functions that do not expect all
diagnostic output to be C strings that may contain format specifiers. For
instance, new variants to output a constant string or a single character are
provided that do not expect varargs or format specifiers. The goal is to
provide alternatives to developers to reduce the pathlength to output an
unformatted string or character.
Testing
This PR was tested on 64-bit and 32-bit compilers in OMR and the OpenJ9 project
on all architectures. An asserting logger was installed in place of the default
logger to identify any unguarded tracing output, and any remaining issues
were fixed.
JitServer and JitDump paths were tested on x86 Linux as well.
Requirements for downstream projects
Downstream projects consuming the OMR compiler may have to adapt to the new
logging infrastructure. The main requirements are:
TR::Logger
you want to use. The default Logger in OMRsimply eats its input.
TR:Logger
instead ofTR::FILE
for diagnostic functionstraceMsg()
andtrfprintf
with equivalent functions on the logger (e.g.,printf()
)I considered maintaining the existing infrastructure in parallel with the new
infrastructure, but with the amount of duplication, ugliness of the code,
performance implications of morphing one API into another, and the relatively
small number of known downstream consumers, I decided against it.