You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2023/01/09 18:47:38 UTC
[GitHub] [logging-log4j2] jvz opened a new pull request, #1194: Migrate Recycler API to log4j-api
jvz opened a new pull request, #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194
This will allow for reuse of this API in places where ThreadLocal-recycled objects are currently used. This will be beneficial to runtimes where workloads don't correspond to threads such as virtual threads, coroutines, and reactive streams. This adds an optional dependency on JCTools to log4j-api for one of the QueueingRecycler implementations.
Just one of many PRs to break out from #1144.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1084343619
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java:
##########
@@ -270,9 +265,10 @@ protected String serializeToString(final Serializer serializer) {
if (serializer == null) {
return null;
}
- final LoggerConfig rootLogger = getConfiguration().getRootLogger();
+ final LoggerConfig rootLogger = configuration.getRootLogger();
+ final LogEventFactory logEventFactory = configuration.getLogEventFactory();
// Using "" for the FQCN, does it matter?
- final LogEvent logEvent = getLogEventFactory().createEvent(rootLogger.getName(), null, Strings.EMPTY,
+ final LogEvent logEvent = logEventFactory.createEvent(rootLogger.getName(), null, Strings.EMPTY,
Review Comment:
The getter wasn't for a field; it was doing work each time. I realized upon fixing this that `Configuration` must be non-null at this point, so we can just get it from `Configuration::getLogEventFactory` (recently added method).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1399644043
> There is one `Recycler` usage case that is not covered by this PR: some objects like `LogBuilder` must be aware of the recycling lifecycle and be able to signal the recycler, when they are available.
>
> In the case of `LogBuilder` I imagine a sequence like this:
>
> * the logger acquires a `LogBuilder` from a `Recycler`,
> * the recycler calls `reset(Runnable release)` on the `LogBuilder`,
> * when the `log` method on the `LogBuilder` is called, the `LogBuilder` runs the `release` action.
Updated to support this. The recycler doesn't even need to reset the LogBuilder as that can be done in a central place already.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1399634073
Ignore test failures as those are broken in master already.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1084342459
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/RecyclerFactory.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+/**
+ * Factory for {@link Recycler} strategies. Depending on workloads, different instance recycling strategies may be
+ * most performant. For example, traditional multithreaded workloads may benefit from using thread-local instance
+ * recycling while different models of concurrency or versions of the JVM may benefit from using an object pooling
+ * strategy instead.
+ *
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface RecyclerFactory {
+
+ /**
+ * Creates a new recycler using the given supplier function for initial instances. These instances have
+ * no cleaner function and are assumed to always be reusable.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(final Supplier<V> supplier) {
+ return create(supplier, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning up
+ * existing instances for reuse. For example, a StringBuilder recycler would provide two functions:
+ * a supplier function for constructing a new StringBuilder with a preselected initial capacity and
+ * another function for trimming the StringBuilder to some preselected maximum capacity and setting
+ * its length back to 0 as if it were a fresh StringBuilder.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param cleaner function to reset a recyclable object to a fresh state
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner) {
+ return create(supplier, cleaner, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning recycled
+ * instances lazily or eagerly. The lazy cleaner function is invoked on recycled instances before being
+ * returned by {@link Recycler#acquire()}. The eager cleaner function is invoked on recycled instances
+ * during {@link Recycler#release(Object)}.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param lazyCleaner function to invoke to clean a recycled object before being acquired
+ * @param eagerCleaner function to invoke to clean a recycled object after being released
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> lazyCleaner, Consumer<V> eagerCleaner);
Review Comment:
I'll need @vy to explain the logic behind using lazy cleaning in JTL, then, because when I tried to simplify this into purely eager cleaning, several JTL tests failed related to resolvers. I'm not sure if the tests are incorrect in this situation or if it does indeed break a bunch of resolvers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1094607530
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableMessageFactory.java:
##########
@@ -89,9 +83,21 @@ public static void release(final Message message) { // LOG4J2-1583
}
}
+ @Override
+ public void recycle(final Message message) {
Review Comment:
`release` javadoc states the following
> This flag is used internally to verify that a reusable message is no longer in use and can be reused.
This sounds to me exactly like what recycle does. I am curious why do we need separate `recycle()` and `release()` methods. Can't we simply move the `recycle()` logic to the bottom of the `release()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1094616022
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java:
##########
@@ -270,9 +265,10 @@ protected String serializeToString(final Serializer serializer) {
if (serializer == null) {
return null;
}
- final LoggerConfig rootLogger = getConfiguration().getRootLogger();
+ final LoggerConfig rootLogger = configuration.getRootLogger();
+ final LogEventFactory logEventFactory = configuration.getLogEventFactory();
// Using "" for the FQCN, does it matter?
- final LogEvent logEvent = getLogEventFactory().createEvent(rootLogger.getName(), null, Strings.EMPTY,
+ final LogEvent logEvent = logEventFactory.createEvent(rootLogger.getName(), null, Strings.EMPTY,
Review Comment:
Agreeing with @jvz here. If a method is not merely a simple field getter (which is a shame that it is still called `getBlah`), its result should first better be stored in a self-explanatory variable. This greatly helps while stepping in a debugger.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083170062
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLogger.java:
##########
@@ -492,24 +493,24 @@ public void actualAsyncLog(final RingBufferLogEvent event) {
privateConfigLoggerConfig.getReliabilityStrategy().log(this, event);
}
- @SuppressWarnings("ForLoopReplaceableByForEach") // Avoid iterator allocation
+ @PerformanceSensitive("allocation")
private void onPropertiesPresent(final RingBufferLogEvent event, final List<Property> properties) {
final StringMap contextData = getContextData(event);
- for (int i = 0, size = properties.size(); i < size; i++) {
- final Property prop = properties.get(i);
+ // List::forEach is garbage-free when using an ArrayList or Arrays.asList
Review Comment:
I discovered the temporary allocation due to our existing GC tests. Lambdas compile down to some sort of synthetic static method on the class it was declared in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083707508
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java:
##########
@@ -270,9 +265,10 @@ protected String serializeToString(final Serializer serializer) {
if (serializer == null) {
return null;
}
- final LoggerConfig rootLogger = getConfiguration().getRootLogger();
+ final LoggerConfig rootLogger = configuration.getRootLogger();
+ final LogEventFactory logEventFactory = configuration.getLogEventFactory();
// Using "" for the FQCN, does it matter?
- final LogEvent logEvent = getLogEventFactory().createEvent(rootLogger.getName(), null, Strings.EMPTY,
+ final LogEvent logEvent = logEventFactory.createEvent(rootLogger.getName(), null, Strings.EMPTY,
Review Comment:
I don't see the need for extracting a local variable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1094968023
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2742,11 +2739,8 @@ public LogBuilder atFatal() {
*/
@Override
public LogBuilder always() {
- final DefaultLogBuilder builder = logBuilder.get();
- if (builder.isInUse()) {
- return new DefaultLogBuilder(this);
- }
- return builder.reset(Level.OFF);
+ final DefaultLogBuilder builder = (DefaultLogBuilder) recycler.acquire();
+ return builder.atLevel(Level.OFF);
Review Comment:
...so that we don't need this unchecked cast.
I can open a new issue for this, since this PR is getting huge.
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -84,7 +82,8 @@ public abstract class AbstractLogger implements ExtendedLogger {
private final MessageFactory messageFactory;
private final FlowMessageFactory flowMessageFactory;
private static final ThreadLocal<int[]> recursionDepthHolder = new ThreadLocal<>(); // LOG4J2-1518, LOG4J2-2031
- protected final transient ThreadLocal<DefaultLogBuilder> logBuilder;
+ protected final Recycler<LogBuilder> recycler = LoggingSystem.getRecyclerFactory()
+ .create(() -> new DefaultLogBuilder(this));
Review Comment:
As a future extension we should probably allow any `LogBuilder`...
##########
log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java:
##########
@@ -246,16 +231,15 @@ private void logMessage(final Message message) {
try {
logger.logMessage(level, marker, fqcn, location, message, throwable);
} finally {
- inUse = false;
+ // recycle self
+ this.level = null;
+ this.marker = null;
+ this.throwable = null;
+ this.location = null;
Review Comment:
I fail to understand how can the recycler know that the `LogBuilder` can be used again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1413832681
> the logger acquires a `LogBuilder` from a `Recycler`
@ppkarwasz, does the logger already pool returned `LogBuilder`s? If not, I would rather create a follow-up ticket for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083550342
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/spi/ThreadLocalRecyclerFactoryTest.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class ThreadLocalRecyclerFactoryTest {
Review Comment:
Added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083718747
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/RecyclerFactory.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+/**
+ * Factory for {@link Recycler} strategies. Depending on workloads, different instance recycling strategies may be
+ * most performant. For example, traditional multithreaded workloads may benefit from using thread-local instance
+ * recycling while different models of concurrency or versions of the JVM may benefit from using an object pooling
+ * strategy instead.
+ *
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface RecyclerFactory {
+
+ /**
+ * Creates a new recycler using the given supplier function for initial instances. These instances have
+ * no cleaner function and are assumed to always be reusable.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(final Supplier<V> supplier) {
+ return create(supplier, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning up
+ * existing instances for reuse. For example, a StringBuilder recycler would provide two functions:
+ * a supplier function for constructing a new StringBuilder with a preselected initial capacity and
+ * another function for trimming the StringBuilder to some preselected maximum capacity and setting
+ * its length back to 0 as if it were a fresh StringBuilder.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param cleaner function to reset a recyclable object to a fresh state
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner) {
+ return create(supplier, cleaner, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning recycled
+ * instances lazily or eagerly. The lazy cleaner function is invoked on recycled instances before being
+ * returned by {@link Recycler#acquire()}. The eager cleaner function is invoked on recycled instances
+ * during {@link Recycler#release(Object)}.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param lazyCleaner function to invoke to clean a recycled object before being acquired
+ * @param eagerCleaner function to invoke to clean a recycled object after being released
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> lazyCleaner, Consumer<V> eagerCleaner);
Review Comment:
I believe that only "eager cleaning" should occur. Otherwise we might retain references to objects that could be otherwise GC-ed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1377652058
While I was working on LOG4J2-3228, I noticed what you were referring to in the reusable message classes. Yes, I'd like to make the `Recycler` API available there, though I'll need to consider the SPI here a little more first. I'll continue porting the other changes first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1084344191
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableMessageFactory.java:
##########
@@ -89,9 +83,21 @@ public static void release(final Message message) { // LOG4J2-1583
}
}
+ @Override
+ public void recycle(final Message message) {
Review Comment:
Oh, and I wasn't able to name this method `release` because it conflicts with the static method with the same signature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083176186
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java:
##########
@@ -235,9 +228,8 @@ public Serializer getHeaderSerializer() {
return headerSerializer;
}
- private DefaultLogEventFactory getLogEventFactory() {
- // TODO: inject this
- return DefaultLogEventFactory.newInstance();
+ private LogEventFactory getLogEventFactory() {
+ return configuration != null ? configuration.getLogEventFactory() : DefaultLogEventFactory.newInstance();
Review Comment:
Yes, you're right. I was being lazy here relying on the fact that this method would only be invoked once, but it should update the field, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1384562741
Alright, I think I've figured out one key update to `RecyclerFactory` to make this API natural to use as a replacement for ad hoc `ThreadLocal<StringBuilder>`-style object reuse. This is to allow for two different types of cleaner functions to be supplied: one for cleaning a recycled instance when acquired, and another for cleaning a recycled instance when it's released. There are plenty of areas in Log4j where it's expected that `StringBuilder` instances will be trimmed right after being used, so this helps support that given that other areas that already use `Recycler` expect that the cleaner is lazily applied. Therefore, I've named them "eager" and "lazy" cleaner functions.
I've also gone ahead and abstracted out some `Queue`-related utilities to more simply construct JCTools queue classes indirectly when available. Will have some commits to push shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1094969336
##########
log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java:
##########
@@ -246,16 +231,15 @@ private void logMessage(final Message message) {
try {
logger.logMessage(level, marker, fqcn, location, message, throwable);
} finally {
- inUse = false;
+ // recycle self
+ this.level = null;
+ this.marker = null;
+ this.throwable = null;
+ this.location = null;
Review Comment:
I fail to understand how can the recycler knows that the `LogBuilder` can be used again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1382942363
So I've moved it to the SPI package. As it stands now, there's already a mini configuration spec string you can specify for recyclers based on the existing JTL configuration parsing logic [documented here](https://logging.apache.org/log4j/2.x/manual/json-template-layout.html#recycling-strategy) and [here for how to specifically make it work for JTL](https://logging.apache.org/log4j/2.x/manual/json-template-layout.html#extending-recycler). I expect that to be generally sufficient when using the API by itself without the backend. In the backend, we have the `Injector` API where it's much easier to bind things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1071590544
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/GelfLayout.java:
##########
@@ -690,22 +726,17 @@ static CharSequence formatTimestamp(final long timeMillis) {
if (timeMillis < 1000) {
return "0";
}
- final StringBuilder builder = getTimestampStringBuilder();
- builder.append(timeMillis);
- builder.insert(builder.length() - 3, '.');
+ final StringBuilder builder = new StringBuilder(20);
+ formatTimestampTo(builder, timeMillis);
return builder;
}
- private static final ThreadLocal<StringBuilder> timestampStringBuilder = new ThreadLocal<>();
-
- private static StringBuilder getTimestampStringBuilder() {
Review Comment:
Also got a kick out of how pointless this buffer is when using the buffer provided by `AbstractStringLayout` already. The joys of new APIs!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1398217767
There is one `Recycler` usage case that is not covered by this PR: some objects like `LogBuilder` must be aware of the recycling lifecycle and be able to signal the recycler, when they are available.
In the case of `LogBuilder` I imagine a sequence like this:
- the logger acquires a `LogBuilder` from a `Recycler`,
- the recycler calls `reset(Runnable release)` on the `LogBuilder`,
- when the `log` method on the `LogBuilder` is called, the `LogBuilder` runs the `release` action.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1384566193
@vy care to have a look at my changes to the `Recycler` API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
vy commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1082265434
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableMessageFactory.java:
##########
@@ -89,9 +83,21 @@ public static void release(final Message message) { // LOG4J2-1583
}
}
+ @Override
+ public void recycle(final Message message) {
Review Comment:
How does this new method compare to `release(Message)`?
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/RecyclerFactory.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+/**
+ * Factory for {@link Recycler} strategies. Depending on workloads, different instance recycling strategies may be
+ * most performant. For example, traditional multithreaded workloads may benefit from using thread-local instance
+ * recycling while different models of concurrency or versions of the JVM may benefit from using an object pooling
+ * strategy instead.
+ *
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface RecyclerFactory {
+
+ /**
+ * Creates a new recycler using the given supplier function for initial instances. These instances have
+ * no cleaner function and are assumed to always be reusable.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(final Supplier<V> supplier) {
+ return create(supplier, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning up
+ * existing instances for reuse. For example, a StringBuilder recycler would provide two functions:
+ * a supplier function for constructing a new StringBuilder with a preselected initial capacity and
+ * another function for trimming the StringBuilder to some preselected maximum capacity and setting
+ * its length back to 0 as if it were a fresh StringBuilder.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param cleaner function to reset a recyclable object to a fresh state
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner) {
+ return create(supplier, cleaner, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning recycled
+ * instances lazily or eagerly. The lazy cleaner function is invoked on recycled instances before being
+ * returned by {@link Recycler#acquire()}. The eager cleaner function is invoked on recycled instances
+ * during {@link Recycler#release(Object)}.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param lazyCleaner function to invoke to clean a recycled object before being acquired
+ * @param eagerCleaner function to invoke to clean a recycled object after being released
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> lazyCleaner, Consumer<V> eagerCleaner);
Review Comment:
What is the reason we need to introduce a lazy-cleaner? Why doesn't simply an eager one suffice?
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/spi/ThreadLocalRecyclerFactoryTest.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class ThreadLocalRecyclerFactoryTest {
+ @Test
+ void nestedAcquiresDoNotInterfere() {
+ final Recycler<AtomicInteger> r = ThreadLocalRecyclerFactory.getInstance()
+ .create(AtomicInteger::new, i -> i.set(0));
+ final var recycler = (ThreadLocalRecyclerFactory.ThreadLocalRecycler<AtomicInteger>) r;
+
+ assertThat(recycler.getQueue()).isEmpty();
+ final AtomicInteger first = recycler.acquire();
+ assertThat(recycler.getQueue()).isEmpty();
+ final AtomicInteger second = recycler.acquire();
+ assertThat(recycler.getQueue()).isEmpty();
+ first.set(1);
Review Comment:
It would be nice to check that `first` and `second` are not ~equal~ _the same_ in terms of their object identity.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java:
##########
@@ -235,9 +228,8 @@ public Serializer getHeaderSerializer() {
return headerSerializer;
}
- private DefaultLogEventFactory getLogEventFactory() {
- // TODO: inject this
- return DefaultLogEventFactory.newInstance();
+ private LogEventFactory getLogEventFactory() {
+ return configuration != null ? configuration.getLogEventFactory() : DefaultLogEventFactory.newInstance();
Review Comment:
Shouldn't this rather be an instance field that returned here? Otherwise, if `configuration` is null, we will return a _new_ instance in a _get_ method.
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableParameterizedMessage.java:
##########
@@ -43,12 +45,25 @@ public class ReusableParameterizedMessage implements ReusableMessage, ParameterV
private Object[] varargs;
private Object[] params = new Object[MAX_PARMS];
private Throwable throwable;
- boolean reserved = false; // LOG4J2-1583 prevent scrambled logs with nested logging calls
+
+ private final Recycler<StringBuilder> bufferRecycler; // non-static: LOG4J2-1583
/**
* Creates a reusable message.
*/
public ReusableParameterizedMessage() {
+ this(RecyclerFactories.getDefault());
+ }
+
+ public ReusableParameterizedMessage(final RecyclerFactory recyclerFactory) {
+ bufferRecycler = recyclerFactory.create(
+ () -> {
+ final int currentPatternLength = messagePattern == null ? 0 : messagePattern.length();
+ return new StringBuilder(Math.max(MIN_BUILDER_SIZE, currentPatternLength * 2));
+ },
+ buffer -> buffer.setLength(0),
+ buffer -> StringBuilders.trimToMaxSize(buffer, Constants.MAX_REUSABLE_MESSAGE_SIZE)
Review Comment:
Shouldn't we extract the `Math.max(MIN_BUILDER_SIZE, currentPatternLength * 2)` value above to a `maxSize` variable and use it in both `trimToMaxSize(maxSize)` and `new StringBuilder(maxSize)`?
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/RecyclerFactories.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import org.apache.logging.log4j.util.QueueFactory;
+import org.apache.logging.log4j.util.Queues;
+import org.apache.logging.log4j.util.StringParameterParser;
+
+import static org.apache.logging.log4j.util.Constants.isThreadLocalsEnabled;
+
+public final class RecyclerFactories {
+
+ private RecyclerFactories() {}
+
+ private static int getDefaultCapacity() {
+ return Math.max(
+ 2 * Runtime.getRuntime().availableProcessors() + 1,
+ 8);
+ }
+
+ public static RecyclerFactory getDefault() {
+ return isThreadLocalsEnabled()
+ ? ThreadLocalRecyclerFactory.getInstance()
+ : new QueueingRecyclerFactory(Queues.MPMC.factory(getDefaultCapacity()));
+ }
+
+ public static RecyclerFactory ofSpec(final String recyclerFactorySpec) {
+
+ // TLA-, MPMC-, or ABQ-based queueing factory -- if nothing is specified.
+ if (recyclerFactorySpec == null) {
+ return getDefault();
+ }
+
+ // Is a dummy factory requested?
+ else if (recyclerFactorySpec.equals("dummy")) {
+ return DummyRecyclerFactory.getInstance();
+ }
+
+ // Is a TLA factory requested?
+ else if (recyclerFactorySpec.equals("threadLocal")) {
+ return ThreadLocalRecyclerFactory.getInstance();
+ }
+
+ // Is a queueing factory requested?
+ else if (recyclerFactorySpec.startsWith("queue")) {
+
+ // Determine the default capacity.
+ final int defaultCapacity = getDefaultCapacity();
+
+ return readQueueingRecyclerFactory(recyclerFactorySpec, defaultCapacity);
+ }
+
+ // Bogus input, bail out.
+ else {
+ throw new IllegalArgumentException(
+ "invalid recycler factory: " + recyclerFactorySpec);
+ }
+
+ }
+
+ private static RecyclerFactory readQueueingRecyclerFactory(
+ final String recyclerFactorySpec,
+ final int defaultCapacity) {
+
+ // Parse the spec.
+ final String queueFactorySpec = recyclerFactorySpec.substring(
+ "queue".length() +
+ (recyclerFactorySpec.startsWith("queue:")
+ ? 1
+ : 0));
+ final Map<String, StringParameterParser.Value> parsedValues =
+ StringParameterParser.parse(
+ queueFactorySpec, Set.of("supplier", "capacity"));
+
+ // Read the supplier path.
+ final StringParameterParser.Value supplierValue = parsedValues.get("supplier");
+ final String supplierPath;
+ if (supplierValue == null || supplierValue instanceof StringParameterParser.NullValue) {
+ supplierPath = null;
+ } else {
+ supplierPath = supplierValue.toString();
+ }
+
+ // Read the capacity.
+ final StringParameterParser.Value capacityValue = parsedValues.get("capacity");
+ final int capacity;
+ if (capacityValue == null || capacityValue instanceof StringParameterParser.NullValue) {
+ capacity = defaultCapacity;
+ } else {
+ try {
+ capacity = Integer.parseInt(capacityValue.toString());
+ } catch (final NumberFormatException error) {
+ throw new IllegalArgumentException(
+ "failed reading capacity in queueing recycler " +
+ "factory: " + queueFactorySpec, error);
+ }
+ }
+
+ // Execute the read spec.
+ final QueueFactory queueFactory = Objects.isNull(supplierPath)
+ ? Queues.MPMC.factory(capacity)
+ : Queues.createQueueFactory(queueFactorySpec, supplierPath, capacity);
Review Comment:
You can do something as follows:
```suggestion
// Read the capacity.
final StringParameterParser.Value capacityValue = parsedValues.get("capacity");
final int capacity;
if (capacityValue == null || capacityValue instanceof StringParameterParser.NullValue) {
capacity = defaultCapacity;
} else {
try {
capacity = Integer.parseInt(capacityValue.toString());
} catch (final NumberFormatException error) {
throw new IllegalArgumentException(
"failed reading capacity in queueing recycler " +
"factory: " + queueFactorySpec, error);
}
}
// Read the supplier path.
final StringParameterParser.Value supplierValue = parsedValues.get("supplier");
final String supplierPath;
if (supplierValue == null || supplierValue instanceof StringParameterParser.NullValue) {
supplierPath = Queues.MPMC.factory(capacity);
} else {
supplierPath = supplierValue.toString();
}
// Execute the read spec.
final QueueFactory queueFactory = Queues.createQueueFactory(queueFactorySpec, supplierPath, capacity);
```
Note that
1. `supplierPath` is not nullable anymore
2. `Objects.isNull()` is not meant to be called like that, but it rather aids as a lambda whenever that check is needed
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/RecyclerFactory.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+/**
+ * Factory for {@link Recycler} strategies. Depending on workloads, different instance recycling strategies may be
+ * most performant. For example, traditional multithreaded workloads may benefit from using thread-local instance
+ * recycling while different models of concurrency or versions of the JVM may benefit from using an object pooling
+ * strategy instead.
+ *
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface RecyclerFactory {
+
+ /**
+ * Creates a new recycler using the given supplier function for initial instances. These instances have
+ * no cleaner function and are assumed to always be reusable.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(final Supplier<V> supplier) {
+ return create(supplier, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning up
+ * existing instances for reuse. For example, a StringBuilder recycler would provide two functions:
+ * a supplier function for constructing a new StringBuilder with a preselected initial capacity and
+ * another function for trimming the StringBuilder to some preselected maximum capacity and setting
+ * its length back to 0 as if it were a fresh StringBuilder.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param cleaner function to reset a recyclable object to a fresh state
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner) {
+ return create(supplier, cleaner, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning recycled
+ * instances lazily or eagerly. The lazy cleaner function is invoked on recycled instances before being
+ * returned by {@link Recycler#acquire()}. The eager cleaner function is invoked on recycled instances
+ * during {@link Recycler#release(Object)}.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param lazyCleaner function to invoke to clean a recycled object before being acquired
+ * @param eagerCleaner function to invoke to clean a recycled object after being released
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> lazyCleaner, Consumer<V> eagerCleaner);
+
+ /**
+ * Creates a default cleaner function that does nothing.
+ */
+ static <V> Consumer<V> defaultCleaner() {
Review Comment:
`defaultCleaner()` -> `getDefaultCleaner()`
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/ThreadLocalRecyclerFactory.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.Queue;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import org.apache.logging.log4j.util.Queues;
+
+/**
+ * Recycling strategy that caches instances in a ThreadLocal value to allow threads to reuse objects. This strategy
+ * may not be appropriate in workloads where units of work are independent of operating system threads such as
+ * reactive streams, coroutines, or virtual threads.
+ */
+public class ThreadLocalRecyclerFactory implements RecyclerFactory {
+
+ private static final ThreadLocalRecyclerFactory INSTANCE =
+ new ThreadLocalRecyclerFactory();
+
+ private ThreadLocalRecyclerFactory() {}
+
+ public static ThreadLocalRecyclerFactory getInstance() {
+ return INSTANCE;
+ }
+
+ @Override
+ public <V> Recycler<V> create(
+ final Supplier<V> supplier,
+ final Consumer<V> lazyCleaner,
+ final Consumer<V> eagerCleaner) {
+ return new ThreadLocalRecycler<>(supplier, lazyCleaner, eagerCleaner);
+ }
+
+ // Visible for testing
+ static class ThreadLocalRecycler<V> implements Recycler<V> {
+
+ private final Supplier<V> supplier;
+
+ private final Consumer<V> lazyCleaner;
+
+ private final Consumer<V> eagerCleaner;
+
+ private final ThreadLocal<Queue<V>> holder;
+
+ private ThreadLocalRecycler(
+ final Supplier<V> supplier,
+ final Consumer<V> lazyCleaner,
+ final Consumer<V> eagerCleaner) {
+ this.supplier = supplier;
+ this.lazyCleaner = lazyCleaner;
+ this.eagerCleaner = eagerCleaner;
+ // allow for some reasonable level of recursive calls before we stop caring to reuse things
+ this.holder = ThreadLocal.withInitial(() -> Queues.SPSC.create(8));
Review Comment:
What does happen when the recursion limit (i.e., 8) is exceeded?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Queues.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.util;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Queue;
+import java.util.concurrent.ArrayBlockingQueue;
+
+import org.jctools.queues.MpmcArrayQueue;
+import org.jctools.queues.MpscArrayQueue;
+import org.jctools.queues.SpmcArrayQueue;
+import org.jctools.queues.SpscArrayQueue;
+
+/**
+ * Provides {@link QueueFactory} and {@link Queue} instances for different use cases. When the
+ * <a href="https://jctools.github.io/JCTools/">JCTools</a> library is included at runtime, then
+ * the specialized lock free or wait free queues are used from there. Otherwise, {@link ArrayBlockingQueue}
+ * is provided as a fallback for thread-safety. Custom implementations of {@link QueueFactory} may also be
+ * created via {@link #createQueueFactory(String, String, int)}.
+ */
+@InternalApi
+public enum Queues {
+ /**
+ * Provides a bounded queue for single-producer/single-consumer usage. Only one thread may offer objects
+ * while only one thread may poll for them.
+ */
+ SPSC(Lazy.lazy(JCToolsQueueFactory.SPSC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/single-consumer usage. Any thread may offer objects while only
+ * one thread may poll for them.
+ */
+ MPSC(Lazy.lazy(JCToolsQueueFactory.MPSC::load)),
+ /**
+ * Provides a bounded queue for single-producer/multi-consumer usage. Only one thread may offer objects but
+ * any thread may poll for them.
+ */
+ SPMC(Lazy.lazy(JCToolsQueueFactory.SPMC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/multi-consumer usage. Any thread may offer objects and any thread
+ * may poll for them.
+ */
+ MPMC(Lazy.lazy(JCToolsQueueFactory.MPMC::load));
+
+ private final Lazy<BoundedQueueFactory> queueFactory;
+
+ Queues(final Lazy<BoundedQueueFactory> queueFactory) {
+ this.queueFactory = queueFactory;
+ }
+
+ public QueueFactory factory(final int capacity) {
+ return new ProxyQueueFactory(queueFactory.get(), capacity);
+ }
+
+ public <E> Queue<E> create(final int capacity) {
+ return queueFactory.get().create(capacity);
+ }
+
+ public static QueueFactory createQueueFactory(final String queueFactorySpec,
Review Comment:
`queueFactorySpec` is not needed, you can remove that argument. It needs to be cleaned up from this class and its other usages.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Queues.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.util;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Queue;
+import java.util.concurrent.ArrayBlockingQueue;
+
+import org.jctools.queues.MpmcArrayQueue;
+import org.jctools.queues.MpscArrayQueue;
+import org.jctools.queues.SpmcArrayQueue;
+import org.jctools.queues.SpscArrayQueue;
+
+/**
+ * Provides {@link QueueFactory} and {@link Queue} instances for different use cases. When the
+ * <a href="https://jctools.github.io/JCTools/">JCTools</a> library is included at runtime, then
+ * the specialized lock free or wait free queues are used from there. Otherwise, {@link ArrayBlockingQueue}
+ * is provided as a fallback for thread-safety. Custom implementations of {@link QueueFactory} may also be
+ * created via {@link #createQueueFactory(String, String, int)}.
+ */
+@InternalApi
+public enum Queues {
+ /**
+ * Provides a bounded queue for single-producer/single-consumer usage. Only one thread may offer objects
+ * while only one thread may poll for them.
+ */
+ SPSC(Lazy.lazy(JCToolsQueueFactory.SPSC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/single-consumer usage. Any thread may offer objects while only
+ * one thread may poll for them.
+ */
+ MPSC(Lazy.lazy(JCToolsQueueFactory.MPSC::load)),
+ /**
+ * Provides a bounded queue for single-producer/multi-consumer usage. Only one thread may offer objects but
+ * any thread may poll for them.
+ */
+ SPMC(Lazy.lazy(JCToolsQueueFactory.SPMC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/multi-consumer usage. Any thread may offer objects and any thread
+ * may poll for them.
+ */
+ MPMC(Lazy.lazy(JCToolsQueueFactory.MPMC::load));
+
+ private final Lazy<BoundedQueueFactory> queueFactory;
+
+ Queues(final Lazy<BoundedQueueFactory> queueFactory) {
+ this.queueFactory = queueFactory;
+ }
+
+ public QueueFactory factory(final int capacity) {
+ return new ProxyQueueFactory(queueFactory.get(), capacity);
+ }
+
+ public <E> Queue<E> create(final int capacity) {
+ return queueFactory.get().create(capacity);
+ }
+
+ public static QueueFactory createQueueFactory(final String queueFactorySpec,
+ final String supplierPath,
+ final int capacity) {
+ final int supplierPathSplitterIndex = supplierPath.lastIndexOf('.');
+ if (supplierPathSplitterIndex < 0) {
+ throw new IllegalArgumentException(
+ "invalid supplier in queue factory: " +
+ queueFactorySpec);
+ }
+ final String supplierClassName = supplierPath.substring(0, supplierPathSplitterIndex);
+ final String supplierMethodName = supplierPath.substring(supplierPathSplitterIndex + 1);
+ try {
+ final Class<?> supplierClass = LoaderUtil.loadClass(supplierClassName);
+ final BoundedQueueFactory queueFactory;
+ if ("new".equals(supplierMethodName)) {
+ final Constructor<?> supplierCtor =
+ supplierClass.getDeclaredConstructor(int.class);
+ queueFactory = new ConstructorProvidedQueueFactory(
+ queueFactorySpec, supplierCtor);
+ } else {
+ final Method supplierMethod =
+ supplierClass.getMethod(supplierMethodName, int.class);
+ queueFactory = new StaticMethodProvidedQueueFactory(
+ queueFactorySpec, supplierMethod);
+ }
+ return new ProxyQueueFactory(queueFactory, capacity);
+ } catch (final ReflectiveOperationException | LinkageError | SecurityException error) {
+ throw new RuntimeException(
+ "failed executing queue factory: " +
+ queueFactorySpec, error);
+ }
+ }
+
+ static class ProxyQueueFactory implements QueueFactory {
Review Comment:
I see many package-local classes (for instance, this one) and ctors which could have been private. Any particular reasons for this visibility preference?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/GelfLayout.java:
##########
@@ -468,6 +486,11 @@ private GelfLayout(final Configuration config, final String host, final KeyValue
this.mdcWriter = new FieldWriter(mdcChecker, mdcPrefix);
this.mapWriter = new FieldWriter(mapChecker, mapPrefix);
this.layout = patternLayout;
+ stacktraceRecycler = recyclerFactory.create(
+ () -> new StringBuilderWriter(2048),
+ writer -> writer.getBuilder().setLength(0),
+ writer -> StringBuilders.trimToMaxSize(writer.getBuilder(), 2048)
+ );
Review Comment:
I would rather replace this ad-hoc 2048 value with `AbstractStringLayout.MAX_STRING_BUILDER_SIZE`. (There is one more usage below as well.)
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/spi/ThreadLocalRecyclerFactoryTest.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class ThreadLocalRecyclerFactoryTest {
Review Comment:
Would you mind adding tests stressing the capacity (i.e., 8) too, please?
1. A test stressing the happy path (I'd personally make `nestedAcquiresDoNotInterfere()` a parametrized test accepting `int acquisitionCount` from 1 to 8)
2. A test stressing the excessive acquisition (i.e., trying to acquire more than 8)
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLogger.java:
##########
@@ -492,24 +493,24 @@ public void actualAsyncLog(final RingBufferLogEvent event) {
privateConfigLoggerConfig.getReliabilityStrategy().log(this, event);
}
- @SuppressWarnings("ForLoopReplaceableByForEach") // Avoid iterator allocation
+ @PerformanceSensitive("allocation")
private void onPropertiesPresent(final RingBufferLogEvent event, final List<Property> properties) {
final StringMap contextData = getContextData(event);
- for (int i = 0, size = properties.size(); i < size; i++) {
- final Property prop = properties.get(i);
+ // List::forEach is garbage-free when using an ArrayList or Arrays.asList
Review Comment:
Is it? You need to pass a lambda, which needs to be allocated. I think this needs to be reverted along with the switch from `List.of()` to `Arrays.asList()` change in `LoggerConfig`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083550302
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/ThreadLocalRecyclerFactory.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.Queue;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import org.apache.logging.log4j.util.Queues;
+
+/**
+ * Recycling strategy that caches instances in a ThreadLocal value to allow threads to reuse objects. This strategy
+ * may not be appropriate in workloads where units of work are independent of operating system threads such as
+ * reactive streams, coroutines, or virtual threads.
+ */
+public class ThreadLocalRecyclerFactory implements RecyclerFactory {
+
+ private static final ThreadLocalRecyclerFactory INSTANCE =
+ new ThreadLocalRecyclerFactory();
+
+ private ThreadLocalRecyclerFactory() {}
+
+ public static ThreadLocalRecyclerFactory getInstance() {
+ return INSTANCE;
+ }
+
+ @Override
+ public <V> Recycler<V> create(
+ final Supplier<V> supplier,
+ final Consumer<V> lazyCleaner,
+ final Consumer<V> eagerCleaner) {
+ return new ThreadLocalRecycler<>(supplier, lazyCleaner, eagerCleaner);
+ }
+
+ // Visible for testing
+ static class ThreadLocalRecycler<V> implements Recycler<V> {
+
+ private final Supplier<V> supplier;
+
+ private final Consumer<V> lazyCleaner;
+
+ private final Consumer<V> eagerCleaner;
+
+ private final ThreadLocal<Queue<V>> holder;
+
+ private ThreadLocalRecycler(
+ final Supplier<V> supplier,
+ final Consumer<V> lazyCleaner,
+ final Consumer<V> eagerCleaner) {
+ this.supplier = supplier;
+ this.lazyCleaner = lazyCleaner;
+ this.eagerCleaner = eagerCleaner;
+ // allow for some reasonable level of recursive calls before we stop caring to reuse things
+ this.holder = ThreadLocal.withInitial(() -> Queues.SPSC.create(8));
Review Comment:
Added a test to demonstrate the intended effect.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Queues.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.util;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Queue;
+import java.util.concurrent.ArrayBlockingQueue;
+
+import org.jctools.queues.MpmcArrayQueue;
+import org.jctools.queues.MpscArrayQueue;
+import org.jctools.queues.SpmcArrayQueue;
+import org.jctools.queues.SpscArrayQueue;
+
+/**
+ * Provides {@link QueueFactory} and {@link Queue} instances for different use cases. When the
+ * <a href="https://jctools.github.io/JCTools/">JCTools</a> library is included at runtime, then
+ * the specialized lock free or wait free queues are used from there. Otherwise, {@link ArrayBlockingQueue}
+ * is provided as a fallback for thread-safety. Custom implementations of {@link QueueFactory} may also be
+ * created via {@link #createQueueFactory(String, String, int)}.
+ */
+@InternalApi
+public enum Queues {
+ /**
+ * Provides a bounded queue for single-producer/single-consumer usage. Only one thread may offer objects
+ * while only one thread may poll for them.
+ */
+ SPSC(Lazy.lazy(JCToolsQueueFactory.SPSC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/single-consumer usage. Any thread may offer objects while only
+ * one thread may poll for them.
+ */
+ MPSC(Lazy.lazy(JCToolsQueueFactory.MPSC::load)),
+ /**
+ * Provides a bounded queue for single-producer/multi-consumer usage. Only one thread may offer objects but
+ * any thread may poll for them.
+ */
+ SPMC(Lazy.lazy(JCToolsQueueFactory.SPMC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/multi-consumer usage. Any thread may offer objects and any thread
+ * may poll for them.
+ */
+ MPMC(Lazy.lazy(JCToolsQueueFactory.MPMC::load));
+
+ private final Lazy<BoundedQueueFactory> queueFactory;
+
+ Queues(final Lazy<BoundedQueueFactory> queueFactory) {
+ this.queueFactory = queueFactory;
+ }
+
+ public QueueFactory factory(final int capacity) {
+ return new ProxyQueueFactory(queueFactory.get(), capacity);
+ }
+
+ public <E> Queue<E> create(final int capacity) {
+ return queueFactory.get().create(capacity);
+ }
+
+ public static QueueFactory createQueueFactory(final String queueFactorySpec,
+ final String supplierPath,
+ final int capacity) {
+ final int supplierPathSplitterIndex = supplierPath.lastIndexOf('.');
+ if (supplierPathSplitterIndex < 0) {
+ throw new IllegalArgumentException(
+ "invalid supplier in queue factory: " +
+ queueFactorySpec);
+ }
+ final String supplierClassName = supplierPath.substring(0, supplierPathSplitterIndex);
+ final String supplierMethodName = supplierPath.substring(supplierPathSplitterIndex + 1);
+ try {
+ final Class<?> supplierClass = LoaderUtil.loadClass(supplierClassName);
+ final BoundedQueueFactory queueFactory;
+ if ("new".equals(supplierMethodName)) {
+ final Constructor<?> supplierCtor =
+ supplierClass.getDeclaredConstructor(int.class);
+ queueFactory = new ConstructorProvidedQueueFactory(
+ queueFactorySpec, supplierCtor);
+ } else {
+ final Method supplierMethod =
+ supplierClass.getMethod(supplierMethodName, int.class);
+ queueFactory = new StaticMethodProvidedQueueFactory(
+ queueFactorySpec, supplierMethod);
+ }
+ return new ProxyQueueFactory(queueFactory, capacity);
+ } catch (final ReflectiveOperationException | LinkageError | SecurityException error) {
+ throw new RuntimeException(
+ "failed executing queue factory: " +
+ queueFactorySpec, error);
+ }
+ }
+
+ static class ProxyQueueFactory implements QueueFactory {
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083173630
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/RecyclerFactory.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+/**
+ * Factory for {@link Recycler} strategies. Depending on workloads, different instance recycling strategies may be
+ * most performant. For example, traditional multithreaded workloads may benefit from using thread-local instance
+ * recycling while different models of concurrency or versions of the JVM may benefit from using an object pooling
+ * strategy instead.
+ *
+ * @since 3.0.0
+ */
+@FunctionalInterface
+public interface RecyclerFactory {
+
+ /**
+ * Creates a new recycler using the given supplier function for initial instances. These instances have
+ * no cleaner function and are assumed to always be reusable.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(final Supplier<V> supplier) {
+ return create(supplier, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning up
+ * existing instances for reuse. For example, a StringBuilder recycler would provide two functions:
+ * a supplier function for constructing a new StringBuilder with a preselected initial capacity and
+ * another function for trimming the StringBuilder to some preselected maximum capacity and setting
+ * its length back to 0 as if it were a fresh StringBuilder.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param cleaner function to reset a recyclable object to a fresh state
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ default <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner) {
+ return create(supplier, cleaner, defaultCleaner());
+ }
+
+ /**
+ * Creates a new recycler using the given functions for providing fresh instances and for cleaning recycled
+ * instances lazily or eagerly. The lazy cleaner function is invoked on recycled instances before being
+ * returned by {@link Recycler#acquire()}. The eager cleaner function is invoked on recycled instances
+ * during {@link Recycler#release(Object)}.
+ *
+ * @param supplier function to provide new instances of a recyclable object
+ * @param lazyCleaner function to invoke to clean a recycled object before being acquired
+ * @param eagerCleaner function to invoke to clean a recycled object after being released
+ * @param <V> the recyclable type
+ * @return a new recycler for V-type instances
+ */
+ <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> lazyCleaner, Consumer<V> eagerCleaner);
Review Comment:
I tried that, but that caused a few hard to diagnose test failures in JTL as that seems to expect lazy cleaning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083171871
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableMessageFactory.java:
##########
@@ -89,9 +83,21 @@ public static void release(final Message message) { // LOG4J2-1583
}
}
+ @Override
+ public void recycle(final Message message) {
Review Comment:
Instead of merely clearing the message, this also returns the object back to its pool.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1071590262
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/GelfLayout.java:
##########
@@ -598,12 +636,22 @@ private StringBuilder toText(final LogEvent event, final StringBuilder builder,
if (event.getThrown() != null || layout != null) {
builder.append("\"full_message\":\"");
if (layout != null) {
- final StringBuilder messageBuffer = getMessageStringBuilder();
- layout.serialize(event, messageBuffer);
- JsonUtils.quoteAsString(messageBuffer, builder);
+ final StringBuilder messageBuffer = acquireStringBuilder();
Review Comment:
Interesting little use case where you can now abuse the recursion support in `ThreadLocalRecyclerFactory` to get multiple recyclable objects. Already works as expected with the `QueueingRecyclerFactory` version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083543253
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Queues.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.util;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Queue;
+import java.util.concurrent.ArrayBlockingQueue;
+
+import org.jctools.queues.MpmcArrayQueue;
+import org.jctools.queues.MpscArrayQueue;
+import org.jctools.queues.SpmcArrayQueue;
+import org.jctools.queues.SpscArrayQueue;
+
+/**
+ * Provides {@link QueueFactory} and {@link Queue} instances for different use cases. When the
+ * <a href="https://jctools.github.io/JCTools/">JCTools</a> library is included at runtime, then
+ * the specialized lock free or wait free queues are used from there. Otherwise, {@link ArrayBlockingQueue}
+ * is provided as a fallback for thread-safety. Custom implementations of {@link QueueFactory} may also be
+ * created via {@link #createQueueFactory(String, String, int)}.
+ */
+@InternalApi
+public enum Queues {
+ /**
+ * Provides a bounded queue for single-producer/single-consumer usage. Only one thread may offer objects
+ * while only one thread may poll for them.
+ */
+ SPSC(Lazy.lazy(JCToolsQueueFactory.SPSC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/single-consumer usage. Any thread may offer objects while only
+ * one thread may poll for them.
+ */
+ MPSC(Lazy.lazy(JCToolsQueueFactory.MPSC::load)),
+ /**
+ * Provides a bounded queue for single-producer/multi-consumer usage. Only one thread may offer objects but
+ * any thread may poll for them.
+ */
+ SPMC(Lazy.lazy(JCToolsQueueFactory.SPMC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/multi-consumer usage. Any thread may offer objects and any thread
+ * may poll for them.
+ */
+ MPMC(Lazy.lazy(JCToolsQueueFactory.MPMC::load));
+
+ private final Lazy<BoundedQueueFactory> queueFactory;
+
+ Queues(final Lazy<BoundedQueueFactory> queueFactory) {
+ this.queueFactory = queueFactory;
+ }
+
+ public QueueFactory factory(final int capacity) {
+ return new ProxyQueueFactory(queueFactory.get(), capacity);
+ }
+
+ public <E> Queue<E> create(final int capacity) {
+ return queueFactory.get().create(capacity);
+ }
+
+ public static QueueFactory createQueueFactory(final String queueFactorySpec,
Review Comment:
Oh right I remember why I added it; it was so the exception messages were still the same and could show an invalid queue factory spec string when provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083175122
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Queues.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.util;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Queue;
+import java.util.concurrent.ArrayBlockingQueue;
+
+import org.jctools.queues.MpmcArrayQueue;
+import org.jctools.queues.MpscArrayQueue;
+import org.jctools.queues.SpmcArrayQueue;
+import org.jctools.queues.SpscArrayQueue;
+
+/**
+ * Provides {@link QueueFactory} and {@link Queue} instances for different use cases. When the
+ * <a href="https://jctools.github.io/JCTools/">JCTools</a> library is included at runtime, then
+ * the specialized lock free or wait free queues are used from there. Otherwise, {@link ArrayBlockingQueue}
+ * is provided as a fallback for thread-safety. Custom implementations of {@link QueueFactory} may also be
+ * created via {@link #createQueueFactory(String, String, int)}.
+ */
+@InternalApi
+public enum Queues {
+ /**
+ * Provides a bounded queue for single-producer/single-consumer usage. Only one thread may offer objects
+ * while only one thread may poll for them.
+ */
+ SPSC(Lazy.lazy(JCToolsQueueFactory.SPSC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/single-consumer usage. Any thread may offer objects while only
+ * one thread may poll for them.
+ */
+ MPSC(Lazy.lazy(JCToolsQueueFactory.MPSC::load)),
+ /**
+ * Provides a bounded queue for single-producer/multi-consumer usage. Only one thread may offer objects but
+ * any thread may poll for them.
+ */
+ SPMC(Lazy.lazy(JCToolsQueueFactory.SPMC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/multi-consumer usage. Any thread may offer objects and any thread
+ * may poll for them.
+ */
+ MPMC(Lazy.lazy(JCToolsQueueFactory.MPMC::load));
+
+ private final Lazy<BoundedQueueFactory> queueFactory;
+
+ Queues(final Lazy<BoundedQueueFactory> queueFactory) {
+ this.queueFactory = queueFactory;
+ }
+
+ public QueueFactory factory(final int capacity) {
+ return new ProxyQueueFactory(queueFactory.get(), capacity);
+ }
+
+ public <E> Queue<E> create(final int capacity) {
+ return queueFactory.get().create(capacity);
+ }
+
+ public static QueueFactory createQueueFactory(final String queueFactorySpec,
Review Comment:
This would be for creating an arbitrary `QueueFactory` from the spec string provided from a parsed recycler spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083173063
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/ThreadLocalRecyclerFactory.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.spi;
+
+import java.util.Queue;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import org.apache.logging.log4j.util.Queues;
+
+/**
+ * Recycling strategy that caches instances in a ThreadLocal value to allow threads to reuse objects. This strategy
+ * may not be appropriate in workloads where units of work are independent of operating system threads such as
+ * reactive streams, coroutines, or virtual threads.
+ */
+public class ThreadLocalRecyclerFactory implements RecyclerFactory {
+
+ private static final ThreadLocalRecyclerFactory INSTANCE =
+ new ThreadLocalRecyclerFactory();
+
+ private ThreadLocalRecyclerFactory() {}
+
+ public static ThreadLocalRecyclerFactory getInstance() {
+ return INSTANCE;
+ }
+
+ @Override
+ public <V> Recycler<V> create(
+ final Supplier<V> supplier,
+ final Consumer<V> lazyCleaner,
+ final Consumer<V> eagerCleaner) {
+ return new ThreadLocalRecycler<>(supplier, lazyCleaner, eagerCleaner);
+ }
+
+ // Visible for testing
+ static class ThreadLocalRecycler<V> implements Recycler<V> {
+
+ private final Supplier<V> supplier;
+
+ private final Consumer<V> lazyCleaner;
+
+ private final Consumer<V> eagerCleaner;
+
+ private final ThreadLocal<Queue<V>> holder;
+
+ private ThreadLocalRecycler(
+ final Supplier<V> supplier,
+ final Consumer<V> lazyCleaner,
+ final Consumer<V> eagerCleaner) {
+ this.supplier = supplier;
+ this.lazyCleaner = lazyCleaner;
+ this.eagerCleaner = eagerCleaner;
+ // allow for some reasonable level of recursive calls before we stop caring to reuse things
+ this.holder = ThreadLocal.withInitial(() -> Queues.SPSC.create(8));
Review Comment:
More objects are allocated, but we only track up to 8 of them for reuse. So I suppose the answer is that it would generate some garbage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate Recycler API to log4j-api (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy closed pull request #1194: Migrate Recycler API to log4j-api
URL: https://github.com/apache/logging-log4j2/pull/1194
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1382951353
Looks like some test failures. Still a work in progress I suppose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083174440
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Queues.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.util;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Queue;
+import java.util.concurrent.ArrayBlockingQueue;
+
+import org.jctools.queues.MpmcArrayQueue;
+import org.jctools.queues.MpscArrayQueue;
+import org.jctools.queues.SpmcArrayQueue;
+import org.jctools.queues.SpscArrayQueue;
+
+/**
+ * Provides {@link QueueFactory} and {@link Queue} instances for different use cases. When the
+ * <a href="https://jctools.github.io/JCTools/">JCTools</a> library is included at runtime, then
+ * the specialized lock free or wait free queues are used from there. Otherwise, {@link ArrayBlockingQueue}
+ * is provided as a fallback for thread-safety. Custom implementations of {@link QueueFactory} may also be
+ * created via {@link #createQueueFactory(String, String, int)}.
+ */
+@InternalApi
+public enum Queues {
+ /**
+ * Provides a bounded queue for single-producer/single-consumer usage. Only one thread may offer objects
+ * while only one thread may poll for them.
+ */
+ SPSC(Lazy.lazy(JCToolsQueueFactory.SPSC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/single-consumer usage. Any thread may offer objects while only
+ * one thread may poll for them.
+ */
+ MPSC(Lazy.lazy(JCToolsQueueFactory.MPSC::load)),
+ /**
+ * Provides a bounded queue for single-producer/multi-consumer usage. Only one thread may offer objects but
+ * any thread may poll for them.
+ */
+ SPMC(Lazy.lazy(JCToolsQueueFactory.SPMC::load)),
+ /**
+ * Provides a bounded queue for multi-producer/multi-consumer usage. Any thread may offer objects and any thread
+ * may poll for them.
+ */
+ MPMC(Lazy.lazy(JCToolsQueueFactory.MPMC::load));
+
+ private final Lazy<BoundedQueueFactory> queueFactory;
+
+ Queues(final Lazy<BoundedQueueFactory> queueFactory) {
+ this.queueFactory = queueFactory;
+ }
+
+ public QueueFactory factory(final int capacity) {
+ return new ProxyQueueFactory(queueFactory.get(), capacity);
+ }
+
+ public <E> Queue<E> create(final int capacity) {
+ return queueFactory.get().create(capacity);
+ }
+
+ public static QueueFactory createQueueFactory(final String queueFactorySpec,
+ final String supplierPath,
+ final int capacity) {
+ final int supplierPathSplitterIndex = supplierPath.lastIndexOf('.');
+ if (supplierPathSplitterIndex < 0) {
+ throw new IllegalArgumentException(
+ "invalid supplier in queue factory: " +
+ queueFactorySpec);
+ }
+ final String supplierClassName = supplierPath.substring(0, supplierPathSplitterIndex);
+ final String supplierMethodName = supplierPath.substring(supplierPathSplitterIndex + 1);
+ try {
+ final Class<?> supplierClass = LoaderUtil.loadClass(supplierClassName);
+ final BoundedQueueFactory queueFactory;
+ if ("new".equals(supplierMethodName)) {
+ final Constructor<?> supplierCtor =
+ supplierClass.getDeclaredConstructor(int.class);
+ queueFactory = new ConstructorProvidedQueueFactory(
+ queueFactorySpec, supplierCtor);
+ } else {
+ final Method supplierMethod =
+ supplierClass.getMethod(supplierMethodName, int.class);
+ queueFactory = new StaticMethodProvidedQueueFactory(
+ queueFactorySpec, supplierMethod);
+ }
+ return new ProxyQueueFactory(queueFactory, capacity);
+ } catch (final ReflectiveOperationException | LinkageError | SecurityException error) {
+ throw new RuntimeException(
+ "failed executing queue factory: " +
+ queueFactorySpec, error);
+ }
+ }
+
+ static class ProxyQueueFactory implements QueueFactory {
Review Comment:
Some of it was for accessing from tests. Others were probably from being lazy. I can make this more consistent!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by GitBox <gi...@apache.org>.
jvz commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1376305373
Could be pluggable via dependency injection. That's how I was initially using it, so I suppose that could mean moving the concrete implementations to core. Having access to DI from the API module is related to a new API added in the larger PR that this was separated from.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1083550106
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableParameterizedMessage.java:
##########
@@ -43,12 +45,25 @@ public class ReusableParameterizedMessage implements ReusableMessage, ParameterV
private Object[] varargs;
private Object[] params = new Object[MAX_PARMS];
private Throwable throwable;
- boolean reserved = false; // LOG4J2-1583 prevent scrambled logs with nested logging calls
+
+ private final Recycler<StringBuilder> bufferRecycler; // non-static: LOG4J2-1583
/**
* Creates a reusable message.
*/
public ReusableParameterizedMessage() {
+ this(RecyclerFactories.getDefault());
+ }
+
+ public ReusableParameterizedMessage(final RecyclerFactory recyclerFactory) {
+ bufferRecycler = recyclerFactory.create(
+ () -> {
+ final int currentPatternLength = messagePattern == null ? 0 : messagePattern.length();
+ return new StringBuilder(Math.max(MIN_BUILDER_SIZE, currentPatternLength * 2));
+ },
+ buffer -> buffer.setLength(0),
+ buffer -> StringBuilders.trimToMaxSize(buffer, Constants.MAX_REUSABLE_MESSAGE_SIZE)
Review Comment:
Looked more into this and that's not how this was intended to work. The first value is the initial message size while the second value is the maximum message size. If I made them both the max size, that would end up wasting memory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a diff in pull request #1194: Migrate Recycler API to log4j-api
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#discussion_r1094612493
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLogger.java:
##########
@@ -492,24 +493,24 @@ public void actualAsyncLog(final RingBufferLogEvent event) {
privateConfigLoggerConfig.getReliabilityStrategy().log(this, event);
}
- @SuppressWarnings("ForLoopReplaceableByForEach") // Avoid iterator allocation
+ @PerformanceSensitive("allocation")
private void onPropertiesPresent(final RingBufferLogEvent event, final List<Property> properties) {
final StringMap contextData = getContextData(event);
- for (int i = 0, size = properties.size(); i < size; i++) {
- final Property prop = properties.get(i);
+ // List::forEach is garbage-free when using an ArrayList or Arrays.asList
Review Comment:
I am not able to follow. Are you saying that our GC-tests are returning false negatives and in reality JIT optimizes this piece of code to have no allocations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #1194: Migrate Recycler API to log4j-api
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1194:
URL: https://github.com/apache/logging-log4j2/pull/1194#issuecomment-1413849266
> > the logger acquires a `LogBuilder` from a `Recycler`
>
> @ppkarwasz, does the logger already pool returned `LogBuilder`s? If not, I would rather create a follow-up ticket for this.
Ignore this please. [@jvz says](#issuecomment-1399644043) he has already implemented it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org