LOG4J2-2940: Context selectors are aware of ClassLoader dependency #475
Conversation
| : null; | ||
| return anchor == null | ||
| ? LogManager.getContext() | ||
| : getContext(StackLocatorUtil.getCallerClass(anchor)); |
There was a problem hiding this comment.
In the case where the context selector is classloader dependent, I imagine we could merge the anchor = StackLocatorUtil.getCallerClass(FQCN, PACKAGE) and StackLocatorUtil.getCallerClass(anchor) calls into a single sweep, potentially cutting the work in half.
There was a problem hiding this comment.
You would have to make a new method that gets the caller of the caller. It might be more efficient. In Java 8 you could continue searching from the last index. In Java 9+ you could continue processing the stream.
There was a problem hiding this comment.
That's what I had in mind. We could provide an overload which takes an additional integer parameter of additional frames to skip.
I'd prefer to implement that optimization separately from this change, and also implement another ContextSelector equivalent to BasicContextSelector for fully asynchronous logging so folks don't have to write their own to avoid classloader lookups.
There was a problem hiding this comment.
I can iterate on this PR with each component in an isolated commit, that may be easiest to review. Let me know if you think the current approach adding boolean isClassLoaderDependent() is reasonable.
There was a problem hiding this comment.
Yes, the optimization should be done separately. I'm not exactly sure what you mean by iterating on this PR. I would suggest merging this one and then do the other work as follow-up items later. So long as what you merge works we should be fine. What you have here looks fine to me.
carterkozak
force-pushed
the
ckozak/LOG4J2-2940
branch
from
fe1091a to
8558a19
Compare
| * Single-application instances should prefer this implementation over the {@link AsyncLoggerContextSelector} | ||
| * due the the reduced overhead avoiding classloader lookups. | ||
| */ | ||
| public class BasicAsyncLoggerContextSelector implements ContextSelector { |
There was a problem hiding this comment.
I still need to add a commit to mention this class in the documentation.
This allows LoggerContext lookups to avoid searching for the calling class to discover a classloader when the ContextSelector implementation is declared to be agnostic to the class loader. Custom LoggerContextFactory and ContextSelector implementations should be updated to override the new `isClassLoaderDependent` method.
…r instance Previously we walked the stack twice, once to find the `org.slf4j.LoggerFactory` interaction, and a second time to discover the application code which called it. Now we use a new API to skip a frame at the end to find the final result in a single interaction.
The BasicAsyncLoggerContextSelector is equivalent to the AsyncLoggerContextSelector without ClassLoader introspection and associated overhead.
carterkozak
force-pushed
the
ckozak/LOG4J2-2940
branch
from
8558a19 to
472852b
Compare
|
Merged support for |
2 participants
This allows LoggerContext lookups to avoid searching for the calling
class to discover a classloader when the ContextSelector implementation
is declared to be agnostic to the class loader.
Custom LoggerContextFactory and ContextSelector implementations
should be updated to override the new
isClassLoaderDependentmethod.