Consider moving recyclers to `log4j-core`

[vy]

As @ppkarwasz remarks here in #2104,

I start to wonder how useful is it to fill log4j-api with partial implementations. For example a recycled LogBuilder is useful for log4j-core, but not so much for log4j-to-slf4j. If log4j-to-slf4j is bound to Logback, the Log4j API builder will be recycled, but the underlying SLF4J builder won't. We might as well remove Recycler from AbstractLogger and move it into core.Logger.

Hence this ticket.

[vy]

@ppkarwasz, I see following major usages of recyclers in log4j-api:

1. AbstractLogger – The usage here can indeed be moved to log4j-core.
2. DefaultLogBuilder – I guess we can move the recycler logic here to log4j-core.
3. ParameterizedMessage – We cannot move the recycler logic here to log4j-core. But we can remove this class in favor of ReusableParameterizedMessage.

What do you think?

[ppkarwasz]

Regarding ParameterizeMessage, it must remain, since it is immutable and ReusableParameterizedMessage#memento() returns this type. However the recycler is only used here:

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java

Lines 185 to 196 in b338d07

	public String getFormattedMessage() {
	if (formattedMessage == null) {
	final StringBuilder buffer = STRING_BUILDER_RECYCLER.acquire();
	try {
	formatTo(buffer);
	formattedMessage = buffer.toString();
	} finally {
	STRING_BUILDER_RECYCLER.release(buffer);
	}
	}
	return formattedMessage;
	}

where it can easily be replaced by new StringBuilder(). It doesn't really matter if the method has one or two allocations.

As far as I can see, DefaultLogBuilder only uses a Recycler, it does not need a RecyclerFactory.

The main problem is AbstractLogger, since we need to look at how it is used in all three implementations.

log4j-to-slf4j

log4j-to-slf4j has its own implementation of LogBuilder that does not extend DefaultLogBuilder and it doesn't use AbstractLogger#recycler at all. It has its own Recycler<Slf4jLogBuilder> field.

As I remarked in #2104, for this implementation we should analyze how it works with Logback to provide a similar experience to Log4j API/Logback and SLF4J/Logback users.

When used with SLF4J's atLevel methods, Logback:

• allocates a new DefaultLoggingEventBuilder if the level is allowed,
• reuses a no-op builder if it is not.

I submitted LOGBACK-1719 to change this behavior and it has been resolved, but I don't see any changes in the code (maybe you can find them).

Currently, when used with Log4j API's atLevel methods, log4j-to-slf4j + Logback:

• use a recycled Slf4jLogBuilder,
• use a possibly recycled Message (it depends on the message factory of course),
• formats the Message (causing an allocation or two ;-) ),
• calls the classic SLF4J API.

We don't provide a no-op path as a fix to LOG4J2-3647.

If we remove the recyclers performance might suffer and much more garbage will be generated, although I don't think Logback users care about that.

log4j-to-jul

log4j-to-jul simply reuses the logic from AbstractLogger. If we remove recyclers, we might harm JBoss LogManager users (which extends JUL's LogManager), although they rather use the Log4j API-to-JBoss Logging bridge.

Proposition

I think that this feature enhancement is strictly linked with #2172. If we provide a light log4j-plugins-api artifact, you could move recyclers to their own artifact instead of including them in log4j-core.

[vy]

@ppkarwasz, thanks for the elaborate response. I think there are more pressing issues. I will pass this task.

[ppkarwasz]

@vy, I agree.

Your Recycler implementation has a very well thought separation between API (in public packages) and implementations (in private packages). Changes like the one proposed here will be possible in the future without breaking BC.