You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "vy (via GitHub)" <gi...@apache.org> on 2023/03/29 13:33:03 UTC
[PR] Recycler API (logging-log4j2)
vy opened a new pull request, #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401
This work moves `Recycler` et al. from `log4j-layout-template-json` to `log4j-api` and replaces `ThreadLocal` usages with `Recycler`s wherever possible. (It is based on @jvz's draft in #1194.)
h2. Why is this a good idea?
`ThreadLocal`s
* provide one way of caching things
* has significant memory tax on threaded applications
* can cause memory leaks in the presence of multiple class loaders
`Recycler` abstraction addresses these pain points by
* abstracting away the caching mechanism – one can decide to disable it, use a queue with a limited capacity, switch back to old behavior by using a `ThreadLocal`-based implementation, or provide a custom one
* enabling its configuration from a central place
See [JSON Template Layout's documentation](https://logging.apache.org/log4j/2.x/manual/json-template-layout.html#recycling-strategy) on it for details.
h2. What are the change highlights?
* Added `Configuration#getRecyclerStrategy()` and `LoggingSystem.getRecyclerStrategy()`
* `ThreadLocal`s in appenders and layouts are replaced with recyclers
* Added `ReusableLogEvent` interface (implemented by `MutableLogEvent` and `RingBufferLogEvent`)
* Adapted `StringBuilderEncoder`
h2. What further needs to be done?
- [ ] fix failing tests
- [ ] write/update docs
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1433690132
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/util/StringsTest.java:
##########
@@ -54,15 +54,6 @@ public void testEMPTY() {
assertEquals(0, Strings.EMPTY.length());
}
- @Test
- public void testConcat() {
- assertEquals("ab", Strings.concat("a", "b"));
- assertEquals("a", Strings.concat("a", ""));
- assertEquals("a", Strings.concat("a", null));
- assertEquals("b", Strings.concat("", "b"));
- assertEquals("b", Strings.concat(null, "b"));
- }
-
Review Comment:
Yes, because I removed `Strings.concat()`, which wasn't used in the code.
--
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] Recycler API (logging-log4j2)
Posted by "rgoers (via GitHub)" <gi...@apache.org>.
rgoers commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1548085067
I am thinking of when Scoped Variables come into play. Or if thread inheritance is required. don't want to have to go into the code and change the "name" passed to the factory. Instead, I would like to do:
`Recycler recycler = RecyclerRegistry.get("name", spec)`
where name represents the particular instance I want to access and spec is the Specification object. The Spec might contain
private boolean inheritable;
private String threadRelated;
or other attributes. Based on this the factory could switch from using a ThreadLocal Recylcer to a ScopedVariableRecycler once it is available by only updating the Registry logic.
In addition, using a registry means I can access the recycler instance when it is needed. Typically, a class has to declare a ThreadLocal as a class member and methods within that class reference it. With what I am proposing each method simply calls the get method to obtain the Recycler instance. Also, this allows:
1. Different use cases to potentially share the same ThreadLocal (depending on their spec declaration)
2. Intelligent cleanup of all Recycler instances.
Since the Recyclers above are just thin veneer around a ThreadLocal or queue the above two items cannot really be done.
Also, saying static registries are a problem flies in the face that Log4j has a ServiceRegistry, PropertiesUtil (the Property Registry), a Plugin registry, the LoggerContext is a registry of the application's Loggers, and the LoggerContextSelector is a registry of the LoggerContexts. There are probably more but that is just what immediately came to mind.
--
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] Recycler API (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1763515200
Besides the merge conflict, I think you should take a look at the updated DI system to see if it fits in with the lifecycles you need.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1547545535
Thanks for taking time to share your feedback @rgoers, much appreciated! :bow:
> I was actually looking for this today as I will be needing to add another ThreadLocal (as much as I really don't want to). But in looking at this PR again I do have some concerns about it.
>
> 1. It uses a Factory to provide a "specification" (really just a String key) to determine what kind of Recycler to provide. That seems somewhat ugly to me. I'd prefer a Spec object that provides attributes for what the characteristics are.
`RecyclerFactories.ofSpec(String)` static factory method is there to only parse the textual recycler factory specification read from a user- provided property. Components will be automatically provided the global [user-set] `RecyclerFactory` in their `Configuration` instance. For those which need to ~run~ test against a particular RF implementation, they can directly reference them; e.g., `ThreadLocalRecyclerFactory::new`.
> 2. It seems every ThreadLocal usage will simply be replaced by a Recycler instance. Instead, I would prefer that there is a Recylcer registry (i.e. - a Map) that contains all the Recyclers. That allows us to reference a Recycler from anywhere without needing them to be injected or passed around.
I assume you mean a static registry as in `RecyclerFactoryRegistry.getDefaultRecyclerFactory()` – please correct me otherwise.
1. Static registries are proven to be a big problem both at runtime and [parallel] tests.
1. When used at runtime, they make it impossible to provide different values for different components. Something conflicts with the DI principles.
1. When used for tests, they cannot be mocked and/or changed per test. This is one of the main reasons we need to fork a new JVM for almost all tests currently.
1. I am not able to see the benefit a global registry. Components should be agnostic to the provided implementation passed via `Configuration#getRecyclerFactory()`. Mind sharing some uses cases, please?
--
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] Recycler API (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1174447095
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2762,12 +2746,8 @@ public LogBuilder atLevel(final Level level) {
* @since 2.20.0
*/
protected LogBuilder getLogBuilder(Level level) {
- if (Constants.ENABLE_THREADLOCALS) {
- DefaultLogBuilder builder = logBuilder.get();
- if (!builder.isInUse()) {
- return builder.reset(this, level);
- }
- }
- return new DefaultLogBuilder(this, level);
+ DefaultLogBuilder builder = recycler.acquire();
Review Comment:
I thought about introducing a `RecyclerAware` interface with a `setRecycler` method. Classes like the `DefaultLogBuilder` or `ReusableParameterizedMessage` would implement it.
Of course your solution also works.
--
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] Recycler API (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1152172344
##########
log4j-1.2-api/src/test/java/org/apache/log4j/layout/Log4j1SyslogLayoutTest.java:
##########
@@ -71,15 +72,18 @@ static Stream<Arguments> configurations() {
public void testSimpleLayout(String expected, Facility facility, boolean header, boolean facilityPrinting) {
final LogEvent logEvent = createLogEvent();
StringLayout appenderLayout = Log4j1SyslogLayout.newBuilder()
+ .setConfiguration(new DefaultConfiguration())
Review Comment:
Nit-pick, though it shouldn't affect the test: all these layouts can use the same `Configuration` instance.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1554747497
> If our DI system worked properly I might agree with you. It does not.
If so, I think we should pause stacking features on top of it and first fix it. I would appreciate it if you can raise this observation of yours with some more detail in the mailing list. A solution proposal would also come pretty handy.
>> I have provided several examples demonstrating why static registries are bad:
>>
>> Hinders testing
>
> No they don't. So long as you can register mock objects it makes no difference whether the registry has a static anchor or not.
Ralph, I am tired of this. I am trying to be polite, giving you concrete examples, but you are slamming the door with hand waving. Either show me a green CI build with `fork=false, parallel=true` or please stop.
>>Do you have a particular use case in mind that requires, e.g., thread inheritance?
>
>Well of course. I have two.
> 1. Scoped Values. "Scoped values are automatically inherited by all child threads created using the StructuredTaskScope." Given that is the default I am sure we will want a way to disable it for certain use cases while leaving it enabled for others.
This is a concern of the `ThreadLocalRecycler` implementation, not the use-site. TLR needs to make sure that it doesn't return an already acquired object. As a matter of fact, this is what it does in its current form, since values are provided from a queue stored in a TL.
> 1. I am using an InheritableThreadLocal in PropretiesUtil so that when configuration takes place any method called, even on a child thread, will get the correct property sources.
I don't see how your `InheritableThreadLocal` usage in `PropretiesUtil` has to anything to do with a recycler. As a matter of fact, the `InheritableThreadLocal` need there arises only because `PropretiesUtil` is accessed statically everywhere, instead of a singleton bean injected by DI.
>> Do you have a [non-hypothetical] use case in mind for the recycler sharing you mention?
>
> ... no I can't firmly give you an example.
I will postpone the idea of a recycler registry until a need arises. We can introduce one when needed in a backward compatible way.
> I will note that you also didn't address the idea of using a Spec object instead of a simple String that directly names the implementation.
I did that on purpose, since I haven't heard a single use-case where it would be necessary. Until I hear one, I am inclined to save it for future.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1871876557
> How does Recycler ensure thread safety?
>
> org.apache.logging.log4j.util.Unbox，Replace the ThreadLocal in this class with Recycler？
Thread-safety is a part of the recycler contract. All Log4j-provided implementations are thread-safe. @tcmot, if you happen to find a place where they aren't, please submit a bug report.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1433690581
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/QueueFactory.java:
##########
@@ -14,18 +14,19 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.logging.log4j.layout.template.json.util;
+package org.apache.logging.log4j.util;
-import java.util.function.Consumer;
-import java.util.function.Supplier;
+import java.util.Queue;
+/**
+ * A {@link Queue} factory contract.
+ *
+ * @see QueueFactories
+ * @since 3.0.0
+ */
@FunctionalInterface
-public interface RecyclerFactory {
-
- default <V> Recycler<V> create(final Supplier<V> supplier) {
- return create(supplier, ignored -> {});
- }
+public interface QueueFactory {
- <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner);
+ <E> Queue<E> create();
Review Comment:
Fixed the issue in #2104.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1520797593
h3. Progress report
I am done with code changes. I will proceed with the documentation update. `garbagefree.adoc` needs to be refactored (contains either outdated or incorrect information) and adapted for recyclers.
--
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] Recycler API (logging-log4j2)
Posted by "rgoers (via GitHub)" <gi...@apache.org>.
rgoers commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1550731405
> But things have changed, in a positive way: we now have a powerful DI system. Let's put that into use!
Unfortunately, the DI system as it is today is broken. I discovered this when working on context properties and Matt is aware of this. The problem is that we have a bunch of "Constants" that really just check a property when the "constant" is initialized. However, that makes it impossible to make those LoggerContext specific. In addition, if you don't happen to have the "correct" Injector instance you are kind of screwed. I'm not saying these can't be fixed, but the DI system is not going to solve every problem in Log4j. Certainly not the items I am raising here.
> Yes and with any other system run by a decent DI, you shouldn't need static access. JTL contains the most complicated DI usage out there and there is not a single call to static DI.* methods.
Yes, the DI system doesn't have a static anchor. Instead we have many DI instances anchored in many places. Our DI system does have static methods (which I don't have a problem with). Again, the DI system really has nothing to do with what I am asking for.
> I have provided several examples demonstrating why static registries are bad:
>
> Hinders testing
No they don't. So long as you can register mock objects it makes no difference whether the registry has a static anchor or not.
> Hinders composition (e.g., one cannot provide a PropertiesUtil implementation, etc.)
Huh? That would be a horrible mistake. That is like saying you cannot provide a DI implementation. Somethings shouldn't be replaceable.
> It is a bad-practice in a DI-backed system
If our DI system worked properly I might agree with you. It does not.
Note - a "Registry" does not necessarily require to be anchored by a static. It could very well be anchored using the DI system, if that indeed was possible (it currently wouldn't be).
> Scoped Values are indeed on the horizon and we should cater for that. We might indeed need to massage the current Recycler API to accommodate that. In particular, scoped values require a lambda (i.e., ScopedValue.where(var, val).run(Runnable)), whereas our current recycler requires explicit acquire/release calls. These ergonomics are pretty much orthongonal. I will try to flesh out some Recycler API support for this.
Here you are pretty much repeating back to me what I said.
> I have my reservations about this Ralph. Recycler is merely an efficient object pool. I think requesting thread-related functionality would break that assumption and sophisticate the design. Do you have a particular use case in mind that requires, e.g., thread inheritance?
Well of course. I have two.
1. Scoped Values. "Scoped values are automatically inherited by all child threads created using the StructuredTaskScope." Given that is the default I am sure we will want a way to disable it for certain use cases.
2. I am using an InheritableThreadLocal in PropretiesUtil so that when configuration takes place any method called, even on a child thread, will get the correct property sources.
> Do you have a [non-hypothetical] use case in mind for the recycler sharing you mention?
Well, maybe. I simply haven't looked. But we use so many ThreadLocals that I have a suspicion that a few of them share the same lifecycle. In that case, in my mind it makes sense to share the recycler. But again, without inspecting every use of ThreadLocal in Log4j, no I can't firmly give you an example.
> Let me again share a similar example from Spring. Imagine a Tree bean implementing Lifecycle.
I have to stop you right there. The problem in Log4j is that we have several Lifecycles:
1. The entire JVM
2. The application instance (there can be several in a web container or OSGi). Generally, this will correlate to a LoggerContext.
3. The configuration. As you know these can start and stop many times during the lifetime of a LoggerContext.
4. AppenderManagers - These may live as long as the LoggerContext does or as short as each Configuration they are part of, depending on how the configuration changed.
5. Even a LogEvent has a lifecycle of sorts.
Matt made a good attempt to make the DI system deal with all these different lifecycles in an appropriate manner, except it doesn't quite work (which probably isn't the fault of the DI code itself but other stuff that he didn't fix - like constants that shouldn't be constants.
I will not that you also didn't address the idea of using a Spec object instead of a simple String that directly names the implementation.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1433690403
##########
log4j-api/pom.xml:
##########
@@ -55,5 +55,10 @@
<artifactId>org.osgi.resource</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.jctools</groupId>
+ <artifactId>jctools-core</artifactId>
+ <optional>true</optional>
+ </dependency>
Review Comment:
Fixed the issue in #2104.
--
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] Recycler API (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1548264841
That pattern Ralph describes is how resilience4j works. Check out https://resilience4j.readme.io/docs
--
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] Recycler API (logging-log4j2)
Posted by "rgoers (via GitHub)" <gi...@apache.org>.
rgoers commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1537341864
I was actually looking for this today as I will be needing to add another ThreadLocal (as much as I really don't want to). But in looking at this PR again I do have some concerns about it.
1. It uses a Factory to provide a "specification" (really just a String key) to determine what kind of Recycler to provide. That seems somewhat ugly to me. I'd prefer a Spec object that provides attributes for what the characteristics are.
2. It seems every ThreadLocal usage will simply be replaced by a Recycler instance. Instead, I would prefer that there is a Recylcer registry (i.e. - a Map) that contains all the Recyclers. That allows us to reference a Recycler from anywhere without needing them to be injected or passed around.
--
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] Recycler API (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1393751655
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/util/StringsTest.java:
##########
@@ -54,15 +54,6 @@ public void testEMPTY() {
assertEquals(0, Strings.EMPTY.length());
}
- @Test
- public void testConcat() {
- assertEquals("ab", Strings.concat("a", "b"));
- assertEquals("a", Strings.concat("a", ""));
- assertEquals("a", Strings.concat("a", null));
- assertEquals("b", Strings.concat("", "b"));
- assertEquals("b", Strings.concat(null, "b"));
- }
-
Review Comment:
We lost this test.
##########
log4j-1.2-api/src/main/java/org/apache/log4j/builders/layout/HtmlLayoutBuilder.java:
##########
@@ -78,6 +79,7 @@ public Layout parse(final PropertiesConfiguration config) {
private Layout createLayout(final String title, final boolean locationInfo) {
return LayoutWrapper.adapt(HtmlLayout.newBuilder()
+ .setConfiguration(new DefaultConfiguration())
Review Comment:
We should use the `config` parameter of the `parse` methods instead.
There should be no `new DefaultConfiguration()` in any of the `o.a.l.builders` classes.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/BlockingQueueFactory.java:
##########
@@ -23,7 +23,7 @@
*
* @since 2.7
*/
-public interface BlockingQueueFactory<E> {
+public interface BlockingQueueFactory {
Review Comment:
This interface should be removed in favor of `BoundedQueueFactory`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/QueueFactory.java:
##########
@@ -14,18 +14,19 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.logging.log4j.layout.template.json.util;
+package org.apache.logging.log4j.util;
-import java.util.function.Consumer;
-import java.util.function.Supplier;
+import java.util.Queue;
+/**
+ * A {@link Queue} factory contract.
+ *
+ * @see QueueFactories
+ * @since 3.0.0
+ */
@FunctionalInterface
-public interface RecyclerFactory {
-
- default <V> Recycler<V> create(final Supplier<V> supplier) {
- return create(supplier, ignored -> {});
- }
+public interface QueueFactory {
- <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner);
+ <E> Queue<E> create();
Review Comment:
Could we condense this with `BoundedQueueFactory` and `BlockingQueueFactory` from `log4j-core`?
The `create` method should probably also require a type of queue (SPSC or MPSC, I don't believe we need multi-consumer queues right now).
Since these factories might be in separate modules (retrieved by `ServiceLoader`) and we can not use plugin annotation, a `getName()` method might be necessary.
##########
log4j-api/pom.xml:
##########
@@ -55,5 +55,10 @@
<artifactId>org.osgi.resource</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.jctools</groupId>
+ <artifactId>jctools-core</artifactId>
+ <optional>true</optional>
+ </dependency>
Review Comment:
I would rather move the code that depend on it to a new module (`log4j-queue-jctools`?).
Since we are in `log4j-api`, the code could detect the factory through `ServiceLoader`.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java:
##########
@@ -91,12 +91,8 @@ public Builder() {
public Builder(final LogEvent other) {
Objects.requireNonNull(other);
- if (other instanceof RingBufferLogEvent) {
- ((RingBufferLogEvent) other).initializeBuilder(this);
- return;
- }
- if (other instanceof MutableLogEvent) {
- ((MutableLogEvent) other).initializeBuilder(this);
+ if (other instanceof ReusableLogEvent) {
+ ((ReusableLogEvent) other).initializeBuilder(this);
Review Comment:
It is nice to see those `instanceof` disappear, although it would be better to have a centralized logic on how to initialize the builder.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/ReusableLogEvent.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.core;
+
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.ThreadContext;
+import org.apache.logging.log4j.core.impl.Log4jLogEvent;
+import org.apache.logging.log4j.core.time.Instant;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.util.StringMap;
+
+public interface ReusableLogEvent extends LogEvent {
+ /**
+ * Clears all references this event has to other objects.
+ */
+ void clear();
+
+ void setLoggerFqcn(final String loggerFqcn);
+
+ void setMarker(final Marker marker);
+
+ void setLevel(final Level level);
+
+ void setLoggerName(final String loggerName);
+
+ void setMessage(final Message message);
+
+ void setThrown(final Throwable thrown);
+
+ void setTimeMillis(final long timeMillis);
+
+ void setInstant(final Instant instant);
+
+ void setSource(final StackTraceElement source);
+
+ @Override
+ StringMap getContextData();
+
+ void setContextData(final StringMap mutableContextData);
+
+ void setContextStack(final ThreadContext.ContextStack contextStack);
+
+ void setThreadId(final long threadId);
+
+ void setThreadName(final String threadName);
+
+ void setThreadPriority(final int threadPriority);
+
+ void setNanoTime(final long nanoTime);
+
+ /**
+ * Initializes the specified {@code Log4jLogEvent.Builder} from this {@code ReusableLogEvent}.
+ * @param builder the builder whose fields to populate
+ */
+ void initializeBuilder(final Log4jLogEvent.Builder builder);
Review Comment:
I am a big fan of this interface: I would love to just have `Message/ReusableMessage` and `LogEvent/ReusableLogEvent` and logic that transfers data between them that **does not** depend on their concrete implementation.
I understand that doing this is **hard**, because we have a lot of mutable parts:
- location information may be absent and we need to retrieve it while copying from one log event to another,
- all context data implementations implement `StringMap` and are mutable, however sometimes we have a guarantee that the data will be delivered to an appender **before** it is reset,
- sometimes we can just swap data between two reusable messages/events if we know that we are the last user of that message/event.
I think that this interface merits its own Github issue.
--
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] Recycler API (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1168564182
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2762,12 +2746,8 @@ public LogBuilder atLevel(final Level level) {
* @since 2.20.0
*/
protected LogBuilder getLogBuilder(Level level) {
- if (Constants.ENABLE_THREADLOCALS) {
- DefaultLogBuilder builder = logBuilder.get();
- if (!builder.isInUse()) {
- return builder.reset(this, level);
- }
- }
- return new DefaultLogBuilder(this, level);
+ DefaultLogBuilder builder = recycler.acquire();
Review Comment:
Where is the log builder released?
--
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] Recycler API (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1174461445
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2762,12 +2746,8 @@ public LogBuilder atLevel(final Level level) {
* @since 2.20.0
*/
protected LogBuilder getLogBuilder(Level level) {
- if (Constants.ENABLE_THREADLOCALS) {
- DefaultLogBuilder builder = logBuilder.get();
- if (!builder.isInUse()) {
- return builder.reset(this, level);
- }
- }
- return new DefaultLogBuilder(this, level);
+ DefaultLogBuilder builder = recycler.acquire();
Review Comment:
I wrote the `RecyclerAware` idea here: https://github.com/ppkarwasz/logging-log4j2/tree/pkarwasz/recycler-api-3.x
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1173939775
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2762,12 +2746,8 @@ public LogBuilder atLevel(final Level level) {
* @since 2.20.0
*/
protected LogBuilder getLogBuilder(Level level) {
- if (Constants.ENABLE_THREADLOCALS) {
- DefaultLogBuilder builder = logBuilder.get();
- if (!builder.isInUse()) {
- return builder.reset(this, level);
- }
- }
- return new DefaultLogBuilder(this, level);
+ DefaultLogBuilder builder = recycler.acquire();
Review Comment:
@ppkarwasz, good catch. Fixed in c72f4c3e44b59cede15c5d594e95c6dd48d0e0f0. Mind resolving the conversation if you agree with the solution?
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1550194311
## Dependency Injection (DI) vs Static Registries
> Also, saying static registries are a problem flies in the face that Log4j has a ServiceRegistry, PropertiesUtil (the PropertyRegistry), a Plugin registry, the LoggerContext is a registry of the application's Loggers, and the LoggerContextSelector is a registry of the LoggerContexts.
There are several JIRA tickets and mailing list threads from users (and committers, PMC members!) complaining that they cannot test logging without forking a fresh JVM each time. Since we both like Spring, let me continue with an example from there. Why do we need to inject `Environment` every time we need to deal with it rather than using a static `PropertiesUtil` just like Log4j does?
I think hardcoding the functionality in static registries was the right decision when it was first introduced to Log4j; it was (still is!) convenient to use, the core was small, and there weren't a DI system. But things have changed, in a positive way: we now have a powerful DI system. Let's put that into use!
> The DI system is also essentially a registry much as Spring's IOC is. There are probably more but that is just what immediately came to mind.
Yes and with any other system run by a decent DI, you shouldn't need static access. JTL contains the most complicated DI usage out there and there is not a single call to static `DI.*` methods.
> You would have to provide some concrete evidence that registries are a problem for me to buy that argument.
I have provided several examples demonstrating why static registries are bad:
1. Hinders testing
2. Hinders composition (e.g., one cannot provide a `PropertiesUtil` implementation, etc.)
3. It is a bad-practice in a DI-backed system
I am curious to know if there is a flaw in my line of thinking.
> FWIW, it is quite easy to create an intelligently designed registry be able to generate and use Mock objects for testing. For example, this is quite easy to do with Spring.
Definitely! But this is all possible since nothing is statically wired but glued together at runtime by DI.
## Scoped Values
> I am thinking of when Scoped Values come into play.
Scoped Values are indeed on the horizon and we should cater for that. We might indeed need to massage the current Recycler API to accommodate that. In particular, scoped values require a lambda (i.e., `ScopedValue.where(var, val).run(Runnable)`), whereas our current recycler requires explicit acquire/release calls. These ergonomics are pretty much orthongonal. I will try to flesh out some Recycler API support for this.
## Requesting thread-related functionality while creating a recycler
> Or if thread inheritance is required.
I have my reservations about this Ralph. Recycler is merely an efficient object pool. I think requesting thread-related functionality would break that assumption and sophisticate the design. Do you have a particular use case in mind that requires, e.g., thread inheritance?
## Having a recycler registry
> `Recycler recycler = RecyclerRegistry.get("name", spec)`
> ...
> 1. Different use cases to potentially share the same ThreadLocal (depending on their spec declaration)
Do you have a [non-hypothetical] use case in mind for the recycler sharing you mention?
> 1. Intelligent cleanup of all Recycler instances. This could be handled through callbacks that are part of the interface or other mechanism as we desire.
Let me again share a similar example from Spring. Imagine a `Tree` bean implementing `Lifecycle`. Other beans can inject this _shared_ `Tree` bean and they don't need to manage the lifecycle of the `Tree` – Spring will take care of it. But, if the bean is prototype-scoped, that is, every injection point gets a new instance, Spring will not manage the bean's lifecycle anymore. I think what we are dealing with is somewhat analogous. If recyclers will be shared (of which I still need to hear a use case for), I understand the sentiment for having a single registry to shut them down. Otherwise, we can delegate that responsibility to the component creating 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
Re: [PR] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1175759925
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java:
##########
@@ -2762,12 +2746,8 @@ public LogBuilder atLevel(final Level level) {
* @since 2.20.0
*/
protected LogBuilder getLogBuilder(Level level) {
- if (Constants.ENABLE_THREADLOCALS) {
- DefaultLogBuilder builder = logBuilder.get();
- if (!builder.isInUse()) {
- return builder.reset(this, level);
- }
- }
- return new DefaultLogBuilder(this, level);
+ DefaultLogBuilder builder = recycler.acquire();
Review Comment:
I have my reservations for making sure each recycler factory specializes on instances of type `RecyclerAware`, though I couldn't resist the utility of this idea. :star_struck:
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1433688251
##########
log4j-1.2-api/src/main/java/org/apache/log4j/builders/layout/HtmlLayoutBuilder.java:
##########
@@ -78,6 +79,7 @@ public Layout parse(final PropertiesConfiguration config) {
private Layout createLayout(final String title, final boolean locationInfo) {
return LayoutWrapper.adapt(HtmlLayout.newBuilder()
+ .setConfiguration(new DefaultConfiguration())
Review Comment:
Good catch. Fixed in 0dc01025f1cd57019c453fa0439634d92349dfa2.
--
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] Recycler API (logging-log4j2)
Posted by "tcmot (via GitHub)" <gi...@apache.org>.
tcmot commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1868251594
@ppkarwasz
How does Recycler ensure thread safety?
org.apache.logging.log4j.util.Unbox，Replace the ThreadLocal in this class with Recycler？
Thanks.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy merged PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1865764460
> Since we are introducing new interfaces and static register classes in `log4j-api`, I think we should take some time to choose were to "hide" those classes to minimize the disruption to Log4j implementors.
>
> Implementors of the Log4j API need to provide an implementation of each interface in `o.a.l.l.spi`. As far as I understand semantic versioning of packages, each time we make a minor version bump of the package (e.g. we introduce a new implementation of `RecyclerFactory`), they need to check if it does not break their implementation.
>
> So ideally the `o.a.l.l.spi` package should probably only contain interfaces and we should move other classes to other packages. E.g. abstract utility classes could be in `o.a.l.l.spi.support`, while our implementations in `o.a.l.l.spi.internal`.
>
> What do you think?
I have applied this convention in the context of recyclers in #2104. For other packages/functionality, it is a whole different story.
--
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] Recycler API (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#issuecomment-1865838959
@vy,
Thanks. Remark that this is just my understanding of how SPIs should work. We should probably consult with experts and write some sort of policy for future extensions of the API. The old SPIs for BC are a whole different story, as you remark.
--
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] Recycler API (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1433691743
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/ReusableLogEvent.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.core;
+
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.ThreadContext;
+import org.apache.logging.log4j.core.impl.Log4jLogEvent;
+import org.apache.logging.log4j.core.time.Instant;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.util.StringMap;
+
+public interface ReusableLogEvent extends LogEvent {
+ /**
+ * Clears all references this event has to other objects.
+ */
+ void clear();
+
+ void setLoggerFqcn(final String loggerFqcn);
+
+ void setMarker(final Marker marker);
+
+ void setLevel(final Level level);
+
+ void setLoggerName(final String loggerName);
+
+ void setMessage(final Message message);
+
+ void setThrown(final Throwable thrown);
+
+ void setTimeMillis(final long timeMillis);
+
+ void setInstant(final Instant instant);
+
+ void setSource(final StackTraceElement source);
+
+ @Override
+ StringMap getContextData();
+
+ void setContextData(final StringMap mutableContextData);
+
+ void setContextStack(final ThreadContext.ContextStack contextStack);
+
+ void setThreadId(final long threadId);
+
+ void setThreadName(final String threadName);
+
+ void setThreadPriority(final int threadPriority);
+
+ void setNanoTime(final long nanoTime);
+
+ /**
+ * Initializes the specified {@code Log4jLogEvent.Builder} from this {@code ReusableLogEvent}.
+ * @param builder the builder whose fields to populate
+ */
+ void initializeBuilder(final Log4jLogEvent.Builder builder);
Review Comment:
I am resolving this conversation, since, as you also pointed out, this needs a separate ticket.
--
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