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 2021/08/29 15:50:42 UTC
[GitHub] [logging-log4j2] carterkozak opened a new pull request #573: Reduce PatternLayout branching
carterkozak opened a new pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573
Prefer fewer conditionals based on types known ahead of time.
Serializer extends Serializer2 with a default implementation
of the encode method -- all of our implementations of
the Serializer interface also implement Serializer2, so the
conditional path was never used. This may allow more code to
be inlined.
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713390780
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/impl/LogEventFactory.java
##########
@@ -28,8 +28,21 @@
/**
*
*/
-public interface LogEventFactory {
+public interface LogEventFactory extends LocationAwareLogEventFactory {
Review comment:
@rgoers I'd appreciate your input on this part of the change. Now that we're using java 8 we can simplify call-sites to invoke the location-aware implementation. It's possible though unlikely that implementations could exist specifically to avoid capturing location information, but our logger configs support configuring that 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] carterkozak commented on pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-916958889
Thanks for the links! I'm planning to benchmark the individual changes as well as end-to-end impact. I'd meant to open this as a draft, this is currently very much an exploratory exercise :-)
I've pulled out the most impactful pieces into separate commits:
https://github.com/apache/logging-log4j2/commit/5078db0e90e2862d2027457f0e95ccb8dbd221fb
https://github.com/apache/logging-log4j2/commit/eff5f185a24c348b25d9d95f1871861c15622284
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713391653
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
##########
@@ -88,9 +88,10 @@ public boolean requiresLocation() {
return false;
}
- public interface Serializer {
+ public interface Serializer extends Serializer2 {
Review comment:
Similarly, this collapses types to avoid unnecessary instanceof checks
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713154271
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
Review comment:
Should this be a CharValue with a SpaceCharValue subclass?
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713155432
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
Review comment:
Great idea!
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r706235288
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/LevelPatternConverter.java
##########
@@ -123,7 +120,7 @@ private static String left(final Level level, final int length) {
*/
@Override
public void format(final LogEvent event, final StringBuilder output) {
- output.append(levelMap == null ? event.getLevel().toString() : levelMap.get(event.getLevel()));
+
Review comment:
This method should be removed, it's effectively abstract but moving the base class from final -> abstract upset revapi.
--
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 #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-917236154
Initial benchmarks on this branch look promising -- writing through a RandomAccessFile appender to `/dev/null` `-Dlog4j2.enable.direct.encoders=false -Dlog4j2.formatMsgNoLookups=true` I'm seeing a 10-13% performance increase (with nolookups enabled). Rerunning with lookups enabled (but not used in my message string) didn't appear to meaningfully impact results.
I still need to do more precise/targetted benchmarking.
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713153477
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
+ private static final Space INSTANCE = new Space();
+
+ @Override
+ void format(StringBuilder output) {
Review comment:
Use final where we can IMO.
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713155032
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
Review comment:
"Noop" would reflect more what is happening here it feels to me.
--
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] rgoers commented on pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-923720165
I cloned your repo and checked out this branch. When I run the build all the RollingFileAppender tests fail and RollingAppenderDeleteScriptFri13thTest is permanently stuck in the while loop.
--
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 #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-939088609
Testing locally passes, CI builds are also passing. Planning to merge this 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] carterkozak merged pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak merged pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713214204
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
+ private static final Space INSTANCE = new Space();
+
+ @Override
+ void format(StringBuilder output) {
Review comment:
Updated with final parameter modifiers
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
Review comment:
That's fair, I think it's helpful to avoid field access overhead for the hottest types, but it's not clear if there are enough non-space single-character literals to be helpful.
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713156919
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
Review comment:
Thanks. We probably might not even need the subclass if we keep a constant for SPACE.
--
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 #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-924387654
> imagine the results may depend on the distinct number and shape of formatter patterns used in an application, as that may push C2 to optimize differently for monomorphic, bimorphic, or megamorphic calls and some of the potential pitfalls of class hierarchy optimizations
@schlosna In this case the additional subclasses shouldn't impact overall performance because the PatternLayout operates with an array of LogEventPatternConverter instances based on the input pattern. There are dozens of types that may be used, we're exchanging some for others to reduce the variables introduced into the system.
Running with java 11:
This PR:
```
$ java -jar log4j-perf/target/benchmarks.jar ".*PatternLayoutBenchmark.serializableMCDEx" -f 1 -wi 4 -i 4 -w 4s -r 4s -t 1 -bm avgt
...
Benchmark Mode Cnt Score Error Units
PatternLayoutBenchmark.serializableMCDEx avgt 4 84.154 ± 10.530 ns/op
```
(second run after running both branches a first time, to verify the results are reproducible)
```
Benchmark Mode Cnt Score Error Units
PatternLayoutBenchmark.serializableMCDEx avgt 4 87.192 ± 16.008 ns/op
```
Compared to the 2.x branch:
```
java -jar log4j-perf/target/benchmarks.jar ".*PatternLayoutBenchmark.serializableMCDEx" -f 1 -wi 4 -i 4 -w 4s -r 4s -t 1 -bm avgt
...
Benchmark Mode Cnt Score Error Units
PatternLayoutBenchmark.serializableMCDEx avgt 4 152.972 ± 2.614 ns/op
```
Second run of release-2.x after the second run of the PR branch:
```
Benchmark Mode Cnt Score Error Units
PatternLayoutBenchmark.serializableMCDEx avgt 4 154.212 ± 8.509 ns/op
```
--
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 #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
schlosna commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-916568200
I would definitely be interested to see benchmark comparisons for before & after. I imagine the results may depend on the distinct number and shape of formatter patterns used in an application, as that [may push C2 to optimize differently for monomorphic, bimorphic, or megamorphic calls](https://shipilev.net/jvm/anatomy-quarks/16-megamorphic-virtual-calls/) and [some of the potential pitfalls of class hierarchy optimizations](https://shipilev.net/blog/2015/black-magic-method-dispatch/#_conclusion) (though logging tends to be a hot path in many apps, so worth the experimentation).
--
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 #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-924095311
Sorry, Ralph. I've updated this branch with a fix. I'm going to PR something into the release branch separately to improve messaging/behavior in that case because it's a little spooky.
`.core.appender.rolling.PatternProcessor` parses patterns and executes (simplified):
```java
List<PatternConverter> parsed = ...;
ArrayPatternConverter[] converters = parsed.toArray(new ArrayPatternConverter[0]);
```
Calling `toArray(subtype)` succeeds based on runtime type info, however it requires all subtypes to implement ArrrayPatternConverter. I think we should either:
1. Fail loudly and eagerly with an error message that states the ArrayPatternConverter requirement and the type of the invalid converter
2. Allow non-array converters, but pass a null value.
--
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] rgoers commented on pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-939362471
I ran the build again and everything passed for me. The failures I saw I have gotten randomly before so I am sure it wasn’t anything you did.
Ralph
> On Oct 8, 2021, at 1:08 PM, Carter Kozak ***@***.***> wrote:
>
>
> Testing locally passes, CI builds are also passing. Planning to merge this shortly.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/logging-log4j2/pull/573#issuecomment-939088609>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5MX5NPG3G4RAJOX4PBYLUF5FU7ANCNFSM5DALVZIA>.
> Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
schlosna commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r705830363
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/LevelPatternConverter.java
##########
@@ -123,7 +120,7 @@ private static String left(final Level level, final int length) {
*/
@Override
public void format(final LogEvent event, final StringBuilder output) {
- output.append(levelMap == null ? event.getLevel().toString() : levelMap.get(event.getLevel()));
+
Review comment:
Is this supposed to be empty, or should this method be removed and handled by super?
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713151635
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/PatternParser.java
##########
@@ -669,20 +668,20 @@ private int finalizeConverter(final char c, final String pattern, final int star
msg.append("] starting at position ");
}
- msg.append(Integer.toString(i));
+ msg.append(i);
msg.append(" in conversion pattern.");
Review comment:
I do not think we should hide boxing and unboxing if we are making all of these performance based changes. I would think we would want to see as much of the mechanics as possible.
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713156919
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
+@PerformanceSensitive("allocation")
+abstract class SimpleLiteralPatternConverter extends LogEventPatternConverter implements ArrayPatternConverter {
+
+ private SimpleLiteralPatternConverter() {
+ super("SimpleLiteral", "literal");
+ }
+
+ static LogEventPatternConverter of(String literal, boolean convertBackslashes) {
+ String value = convertBackslashes ? OptionConverter.convertSpecialChars(literal) : literal;
+ return of(value);
+ }
+
+ static LogEventPatternConverter of(String literal) {
+ if (literal == null || literal.isEmpty()) {
+ return Empty.INSTANCE;
+ }
+ if (" ".equals(literal)) {
+ return Space.INSTANCE;
+ }
+ return new StringValue(literal);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final LogEvent ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final Object ignored, final StringBuilder output) {
+ format(output);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public final void format(final StringBuilder output, final Object... args) {
+ format(output);
+ }
+
+ abstract void format(final StringBuilder output);
+
+ @Override
+ public final boolean isVariable() {
+ return false;
+ }
+
+ @Override
+ public final boolean handlesThrowable() {
+ return false;
+ }
+
+ private static final class Empty extends SimpleLiteralPatternConverter {
+ private static final Empty INSTANCE = new Empty();
+
+ @Override
+ void format(StringBuilder output) {
+ // no-op
+ }
+ }
+
+ private static final class Space extends SimpleLiteralPatternConverter {
Review comment:
Thanks. We probably don't even need the subclass if we keep a constant for SPACE.
--
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 #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#issuecomment-916965179
I imagine there's something clever (aka risky/unmaintainable/fun) we could do with layouts using bytecode generation to avoid some amount of overhead. Definitely want to explore all other options first and validate that we fully grok the causes, however in benchmarking I found a hand-written layout performed ~15-20% better than PatternLayout with identical output.
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713152109
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/SimpleLiteralPatternConverter.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.pattern;
+
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.util.OptionConverter;
+import org.apache.logging.log4j.util.PerformanceSensitive;
+
+
+/**
+ * Formats a string literal without substitution.
+ *
+ * This is an effectively-sealed internal type.
+ */
Review comment:
And since tag?
--
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 change in pull request #573: Reduce PatternLayout branching
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #573:
URL: https://github.com/apache/logging-log4j2/pull/573#discussion_r713154918
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/PatternParser.java
##########
@@ -669,20 +668,20 @@ private int finalizeConverter(final char c, final String pattern, final int star
msg.append("] starting at position ");
}
- msg.append(Integer.toString(i));
+ msg.append(i);
msg.append(" in conversion pattern.");
Review comment:
This actually uses the `append(int)` overload, avoiding boxing and string allocation entirely :-)
https://github.com/openjdk/jdk/blob/b4b121018d16e531f3a51ff75ae37fdc374d530b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java#L811-L834
--
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