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 2022/07/23 05:54:17 UTC
[GitHub] [logging-log4j2] schlosna opened a new pull request, #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
schlosna opened a new pull request, #974:
URL: https://github.com/apache/logging-log4j2/pull/974
Fixes [LOG4J2-3560](https://issues.apache.org/jira/browse/LOG4J2-3560)
While reviewing some JFR profiles, I notice significant allocations of `Object[]` due to use of some `isEnabled` conditions that ends up using empty varargs invoking `org.apache.logging.log4j.core.Filter#filter(Logger, Level, Marker, String, Object...)`, for example the following call trace:
```
at org.apache.logging.log4j.core.Logger$PrivateConfig.filter(Level, Marker, String)
at org.apache.logging.log4j.core.Logger.isEnabled(Level, Marker, String)
```
We can avoid allocation on this hot path when invoking `Filter#filter(Logger, Level, Marker, String, Object...)` by reusing an empty object array for the varargs.
--
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] schlosna commented on a diff in pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on code in PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#discussion_r928090389
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java:
##########
@@ -53,6 +53,9 @@ public class Logger extends AbstractLogger implements Supplier<LoggerConfig> {
private static final long serialVersionUID = 1L;
+ // Used to avoid allocations for empty varargs
+ private static final Object[] EMPTY_VARARGS = new Object[0];
Review Comment:
Thanks for the quick review! 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] ppkarwasz commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1193080506
I wonder if the `Filter` interface isn't just missing a `filter(Logger, Level, Marker, String, Throwable)` method.
Alternatively we can call the `filter(Logger, Level, Marker, Object, Throwable)` method instead of the one with the `Object` array.
--
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] schlosna commented on a diff in pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on code in PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#discussion_r931203262
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java:
##########
@@ -299,7 +300,7 @@ public Map<String, Appender> getAppenders() {
public Iterator<Filter> getFilters() {
final Filter filter = privateConfig.loggerConfig.getFilter();
if (filter == null) {
- return new ArrayList<Filter>().iterator();
Review Comment:
this was a drive by fixup while modifying this file to avoid allocation when there's no filters. I can revert if desired
--
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] carterkozak commented on a diff in pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
carterkozak commented on code in PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#discussion_r931213679
##########
src/changes/changes.xml:
##########
@@ -33,6 +33,9 @@
<action issue="LOG4J2-3550" dev="rgoers" type="fix" due-to="DongjianPeng">
SystemPropertyAribiter was assigning the value as the name.
</action>
+ <action issue="LOG4J2-3560" dev="schlosna" type="fix">
Review Comment:
I'll take care of this update prior to merging, the dev attribute reflects the committer who has signed off on the change while `due-to` provides attribution for the work if it wasn't generated by the committer.
```suggestion
<action issue="LOG4J2-3560" dev="ckozak" type="fix" due-to="David Schlosnagle">
```
--
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] schlosna commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1193133992
I added a unit test that covers the filter code path, but I'm not quite sure how to hook into the garbage generation tests. Do `GcFreeAsynchronousLoggingTest` and `GcFreeSynchronousLoggingTest` automatically pick these up or do tests/test class need to be tagged in some fashion? Do I need to use `GcFreeLoggingTestUtil` directly?
--
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] schlosna commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1197104264
Excellent, GC test updates look good to me, thanks for adding @carterkozak for pushing this through.
--
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] carterkozak commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
carterkozak commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1195501428
I'll take care of getting this integrated, and verify the efficacy of the provided test. I should have time later today -- thanks @schlosna :-)
I agree with @ppkarwasz that we may be missing an overload on the filter interface, which should be easier to add now that we support java 8 compilation target with default methods. I'll look into adding this API as a separate follow-up to this bugfix.
--
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] garydgregory commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1193120128
We have tests that explicitly check for generated garbage already, so a similar test would be nice.
--
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] schlosna commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1193067015
@carterkozak I'd be interested in your thoughts on 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] schlosna commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1195792248
Thanks @carterkozak !
--
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] garydgregory commented on a diff in pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#discussion_r928086798
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java:
##########
@@ -53,6 +53,9 @@ public class Logger extends AbstractLogger implements Supplier<LoggerConfig> {
private static final long serialVersionUID = 1L;
+ // Used to avoid allocations for empty varargs
+ private static final Object[] EMPTY_VARARGS = new Object[0];
Review Comment:
Use can can use the short cut {} on the right-hand side.
--
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] schlosna commented on a diff in pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on code in PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#discussion_r931203262
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java:
##########
@@ -299,7 +300,7 @@ public Map<String, Appender> getAppenders() {
public Iterator<Filter> getFilters() {
final Filter filter = privateConfig.loggerConfig.getFilter();
if (filter == null) {
- return new ArrayList<Filter>().iterator();
Review Comment:
this was a drive by fixup while modifying this file to avoid allocation when there's no filters
--
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] carterkozak commented on pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
carterkozak commented on PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#issuecomment-1196972153
I've pushed a couple minor edits, including an update to `GcFreeLoggingTestUtil` which I've verified on `release-2.x` catches this bug, and applied to this branch passes as expected.
Test failure on release-2.x matches our expectations:
```
org.opentest4j.AssertionFailedError: pattern mismatch at line 1: FATAL o.a.l.l.c.GcFreeSynchronousLoggingTest [main] value1 {aKey=value1, key2I just allocated the object [Ljava.lang.Object;@32b260fa of type java/lang/Object whose size is 16 ==>
Expected :true
Actual :false
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
at org.apache.logging.log4j.core.GcFreeLoggingTestUtil.lambda$runTest$1(GcFreeLoggingTestUtil.java:241)
at java.util.Iterator.forEachRemaining(Iterator.java:116)
at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:647)
at org.apache.logging.log4j.core.GcFreeLoggingTestUtil.runTest(GcFreeLoggingTestUtil.java:226)
at org.apache.logging.log4j.core.GcFreeSynchronousLoggingTest.testNoAllocationDuringSteadyStateLogging(GcFreeSynchronousLoggingTest.java:33)
...etc
```
--
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] carterkozak merged pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
carterkozak merged PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974
--
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] schlosna commented on a diff in pull request #974: LOG4J2-3560: Logger$PrivateConfig.filter(Level, Marker, String) varargs does not allocate
Posted by GitBox <gi...@apache.org>.
schlosna commented on code in PR #974:
URL: https://github.com/apache/logging-log4j2/pull/974#discussion_r931343813
##########
src/changes/changes.xml:
##########
@@ -33,6 +33,9 @@
<action issue="LOG4J2-3550" dev="rgoers" type="fix" due-to="DongjianPeng">
SystemPropertyAribiter was assigning the value as the name.
</action>
+ <action issue="LOG4J2-3560" dev="schlosna" type="fix">
Review Comment:
Sounds good!
--
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