Skip to content
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

Make location computation and message formatting more lazy #2329

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

ppkarwasz
Copy link
Contributor

We move the computation of location and message formatting to the last method call executed before a message leaves the current thread.

This has several consequences:

  • the refactoring of location computations should solve AsyncLogger does not compute location for some methods #1458,
  • since AbstractLogger no longer needs to compute the location of the caller, many messages were refactored to fit in the 35 bytes of bytecode limit. Only logIfEnabled methods with 4 or more Object parameters are over the limit and logMessage methods with 8 or more Object parameters, handleMessageException and the constructors,
  • messages that are filtered out by e.g. the filter of AsyncAppender do not include location information.

Part of #2290.

@ppkarwasz ppkarwasz added the STF-Milestones Milestones funded by the Sovereign Tech Fund label Feb 29, 2024
@ppkarwasz ppkarwasz added this to the 3.x milestone Feb 29, 2024
@ppkarwasz ppkarwasz self-assigned this Feb 29, 2024
Base automatically changed from fix/move-abstractlogger to feature/log4j-sdk February 29, 2024 15:14
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

Great simplification @ppkarwasz! 💯

  1. Since the Message is already populated with a loggerFqcn and that is enough to resolve the location in the LogEvent, we don't need to resolve the location in AbstractLogger, right?
  2. Due to to peekSource(), when a message gets copied elsewhere, peekSource() can be used to initialize the source, and reuse the already populated value, if present. Otherwise, getSource() can calculate it, if requested. This is the crux of this locate-at-the-end implementation, right?

final LocationInfo info = event.getLocationInformation();
return new StackTraceElement(
info.getClassName(), info.getMethodName(), info.getFileName(), Integer.parseInt(info.getLineNumber()));
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also be annotated with @Nullable too? (Applies to other getSource() changes delegating to peekSource().)

We move the computation of location and message formatting to the last
method call executed before a message leaves the current thread.
@ppkarwasz
Copy link
Contributor Author

1. Since the `Message` is already populated with a `loggerFqcn` and that is enough to resolve the `location` in the `LogEvent`, we don't need to resolve the location in `AbstractLogger`, right?

Right. Having the FQCN of the logger class allows us to retrieve the location at any time or never.

Previously the location was computed eagerly based on the result of requiresLocation(). This method can tell us if location information will be necessary, when the message will be logged. However it can not tell if the message will be logged at all.

2. Due to to `peekSource()`, when a message gets copied elsewhere, `peekSource()` can be used to initialize the `source`, and reuse the already populated value, if present. Otherwise, `getSource()` can calculate it, if requested. This is the crux of this locate-at-the-end implementation, right?

Partially, the original getSource() method was introduced when location was a binary choice: you could either compute it and log it or neither compute it nor log it. However the LogBuilder#withLocation method changed this: we can know the location without computing it.

If we just keep the getSource() method and the user sets includeLocation == false on a component (which means "do not compute location), we need to be extra careful while calling getSource:

boolean oldIncludeLocation = event.isIncludeLocation();
event.setIncludeLocation(false);
event.getSource();
event.setIncludeLocation(oldIncludeLocation);

The peekSource is a way to make the process easier.

@ppkarwasz ppkarwasz merged commit 79b6316 into feature/log4j-sdk Mar 14, 2024
5 checks passed
@ppkarwasz ppkarwasz deleted the fix/lazy-location branch March 14, 2024 19:51
@ppkarwasz ppkarwasz mentioned this pull request Mar 28, 2024
8 tasks
@vy vy removed the STF-Milestones Milestones funded by the Sovereign Tech Fund label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants