I've added a @PerformanceSensitive annotation in log4j-api along with a few places I could think of that warrant it. Though I'm not sure about what attributes would be useful here.
On 21 February 2016 at 20:17, Remko Popma <[email protected]> wrote: > Haha! :-) > Is that good or bad? :-O > > On Mon, Feb 22, 2016 at 11:04 AM, Gary Gregory <[email protected]> > wrote: > >> You can be the Hemingway of Java code! ;-) >> >> On Sun, Feb 21, 2016 at 6:04 PM, Gary Gregory <[email protected]> >> wrote: >> >>> Well, yeah, sure, as long as it makes sense... >>> >>> >>> On Sun, Feb 21, 2016 at 6:01 PM, Remko Popma <[email protected]> >>> wrote: >>> >>>> You actually end up with really clean code where each method does only >>>> one thing. Single responsibility principle etc. >>>> Clean code is fast! >>>> >>>> On Mon, Feb 22, 2016 at 10:58 AM, Gary Gregory <[email protected]> >>>> wrote: >>>> >>>>> The 35 bytecodes or less limit seems arbitrary. What about IBM JVMs? >>>>> Other vendors'? >>>>> >>>>> I OK with refactoring as long as it makes sense but just doing it for >>>>> an arbitrary byte limit if it makes the code harder to maintain seems >>>>> like it could be... I'm not sure what the word is I'm even looking for >>>>> here! >>>>> >>>>> Gary >>>>> >>>>> On Sun, Feb 21, 2016 at 5:49 PM, Remko Popma <[email protected]> >>>>> wrote: >>>>> >>>>>> I'd say log4j-api util, so it can be used in the log4j-api >>>>>> implementation classes as well. >>>>>> >>>>>> I would not want to over-complicate this, but there are two ways in >>>>>> which methods can be optimized for performance which are "fragile" in >>>>>> that >>>>>> a marker of some kind may be useful: >>>>>> >>>>>> * refactor into small private methods 35 bytecodes or less (these are >>>>>> inlined automatically by Hotspot). I've tried to be consistent in adding >>>>>> a >>>>>> comment to such methods, but writing the same comment get tedious. >>>>>> * reduce GC pressure by replacing methods or constructs that allocate >>>>>> temporary objects with alternatives that don't. However, there are many, >>>>>> many methods and most don't allocate. I would not want to annotate all >>>>>> methods... Hmm, need to think about this one more. >>>>>> >>>>>> >>>>>> On Mon, Feb 22, 2016 at 10:24 AM, Matt Sicker <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Do we need this in log4j-api anywhere, or can we stick it in >>>>>>> log4j-core? >>>>>>> >>>>>>> On 19 February 2016 at 18:29, Remko Popma <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> I like that idea very much. >>>>>>>> >>>>>>>> Sent from my iPhone >>>>>>>> >>>>>>>> On 2016/02/20, at 7:37, Matt Sicker <[email protected]> wrote: >>>>>>>> >>>>>>>> We could use an annotation. That would make it easier to implement >>>>>>>> an aspect-oriented unit test to verify gc-free code paths. >>>>>>>> >>>>>>>> On 19 February 2016 at 16:14, Remko Popma <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Another thing:if you look at the new code, where should I put the >>>>>>>>> comment? There is nothing tricky about the new logic... >>>>>>>>> >>>>>>>>> Gary has a point that it may be good to have some sort of >>>>>>>>> standardized reminder to distinguish performance sensitive >>>>>>>>> methods (executed for each event) from non-sensitive logic. Need to >>>>>>>>> think >>>>>>>>> about that one. (Ideas welcome.) >>>>>>>>> >>>>>>>>> On Saturday, 20 February 2016, Matt Sicker <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> That sounds like a far more robust solution :) >>>>>>>>>> >>>>>>>>>> On 19 February 2016 at 11:18, Remko Popma <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Good point. >>>>>>>>>>> I'm looking at a way to use aspects during unit testing to >>>>>>>>>>> automatically detect if objects are allocated. >>>>>>>>>>> This will help prevent regressions once the no-GC goal has been >>>>>>>>>>> achieved. >>>>>>>>>>> >>>>>>>>>>> On Sat, Feb 20, 2016 at 2:03 AM, Gary Gregory < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> I think that all these perf changes need a method comment that >>>>>>>>>>>> says something like "note that this code carefully does this and >>>>>>>>>>>> not that >>>>>>>>>>>> for performance". >>>>>>>>>>>> >>>>>>>>>>>> It should all be doc'd otherwise it is too easy for someone >>>>>>>>>>>> else to edit the method and undo the intent of the carefully >>>>>>>>>>>> tweaked perf >>>>>>>>>>>> changes. >>>>>>>>>>>> >>>>>>>>>>>> Gary >>>>>>>>>>>> ---------- Forwarded message ---------- >>>>>>>>>>>> From: <[email protected]> >>>>>>>>>>>> Date: Feb 19, 2016 7:41 AM >>>>>>>>>>>> Subject: logging-log4j2 git commit: LOG4J2-1281 >>>>>>>>>>>> LoggerConfig.getProperties() should not allocate on each call. >>>>>>>>>>>> To: <[email protected]> >>>>>>>>>>>> Cc: >>>>>>>>>>>> >>>>>>>>>>>> Repository: logging-log4j2 >>>>>>>>>>>> Updated Branches: >>>>>>>>>>>> refs/heads/master 25a780e95 -> 3458ea944 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> LOG4J2-1281 LoggerConfig.getProperties() should not allocate on >>>>>>>>>>>> each call. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Project: >>>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo >>>>>>>>>>>> Commit: >>>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/3458ea94 >>>>>>>>>>>> Tree: >>>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/3458ea94 >>>>>>>>>>>> Diff: >>>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/3458ea94 >>>>>>>>>>>> >>>>>>>>>>>> Branch: refs/heads/master >>>>>>>>>>>> Commit: 3458ea94491c01a4ce91bcf5bb249c80306fa42c >>>>>>>>>>>> Parents: 25a780e >>>>>>>>>>>> Author: rpopma <[email protected]> >>>>>>>>>>>> Authored: Sat Feb 20 00:41:24 2016 +0900 >>>>>>>>>>>> Committer: rpopma <[email protected]> >>>>>>>>>>>> Committed: Sat Feb 20 00:41:24 2016 +0900 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>>> .../org/apache/logging/log4j/core/config/LoggerConfig.java >>>>>>>>>>>> | 7 ++++--- >>>>>>>>>>>> src/changes/changes.xml >>>>>>>>>>>> | 3 +++ >>>>>>>>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/3458ea94/log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java >>>>>>>>>>>> >>>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>>> diff --git >>>>>>>>>>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java >>>>>>>>>>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java >>>>>>>>>>>> index 2bd25be..1d0f530 100644 >>>>>>>>>>>> --- >>>>>>>>>>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java >>>>>>>>>>>> +++ >>>>>>>>>>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java >>>>>>>>>>>> @@ -127,11 +127,12 @@ public class LoggerConfig extends >>>>>>>>>>>> AbstractFilterable { >>>>>>>>>>>> this.includeLocation = includeLocation; >>>>>>>>>>>> this.config = config; >>>>>>>>>>>> if (properties != null && properties.length > 0) { >>>>>>>>>>>> - this.properties = new HashMap<>(properties.length); >>>>>>>>>>>> + final Map<Property, Boolean> map = new >>>>>>>>>>>> HashMap<>(properties.length); >>>>>>>>>>>> for (final Property prop : properties) { >>>>>>>>>>>> final boolean interpolate = >>>>>>>>>>>> prop.getValue().contains("${"); >>>>>>>>>>>> - this.properties.put(prop, interpolate); >>>>>>>>>>>> + map.put(prop, interpolate); >>>>>>>>>>>> } >>>>>>>>>>>> + this.properties = Collections.unmodifiableMap(map); >>>>>>>>>>>> } else { >>>>>>>>>>>> this.properties = null; >>>>>>>>>>>> } >>>>>>>>>>>> @@ -308,7 +309,7 @@ public class LoggerConfig extends >>>>>>>>>>>> AbstractFilterable { >>>>>>>>>>>> */ >>>>>>>>>>>> // LOG4J2-157 >>>>>>>>>>>> public Map<Property, Boolean> getProperties() { >>>>>>>>>>>> - return properties == null ? null : >>>>>>>>>>>> Collections.unmodifiableMap(properties); >>>>>>>>>>>> + return properties; >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> /** >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/3458ea94/src/changes/changes.xml >>>>>>>>>>>> >>>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>>> diff --git a/src/changes/changes.xml b/src/changes/changes.xml >>>>>>>>>>>> index e579ed4..a006907 100644 >>>>>>>>>>>> --- a/src/changes/changes.xml >>>>>>>>>>>> +++ b/src/changes/changes.xml >>>>>>>>>>>> @@ -24,6 +24,9 @@ >>>>>>>>>>>> </properties> >>>>>>>>>>>> <body> >>>>>>>>>>>> <release version="2.6" date="201Y-MM-DD" description="GA >>>>>>>>>>>> Release 2.6"> >>>>>>>>>>>> + <action issue="LOG4J2-1281" dev="rpopma" type="fix"> >>>>>>>>>>>> + LoggerConfig.getProperties() should not allocate on >>>>>>>>>>>> each call. >>>>>>>>>>>> + </action> >>>>>>>>>>>> <action issue="LOG4J2-1280" dev="rpopma" type="fix"> >>>>>>>>>>>> Logger methods taking Supplier parameters now >>>>>>>>>>>> correctly handle cases where the supplied value is a Message. >>>>>>>>>>>> </action> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Matt Sicker <[email protected]> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Matt Sicker <[email protected]> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> E-Mail: [email protected] | [email protected] >>>>> Java Persistence with Hibernate, Second Edition >>>>> <http://www.manning.com/bauer3/> >>>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >>>>> Spring Batch in Action <http://www.manning.com/templier/> >>>>> Blog: http://garygregory.wordpress.com >>>>> Home: http://garygregory.com/ >>>>> Tweet! http://twitter.com/GaryGregory >>>>> >>>> >>>> >>> >>> >>> -- >>> E-Mail: [email protected] | [email protected] >>> Java Persistence with Hibernate, Second Edition >>> <http://www.manning.com/bauer3/> >>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >>> Spring Batch in Action <http://www.manning.com/templier/> >>> Blog: http://garygregory.wordpress.com >>> Home: http://garygregory.com/ >>> Tweet! http://twitter.com/GaryGregory >>> >> >> >> >> -- >> E-Mail: [email protected] | [email protected] >> Java Persistence with Hibernate, Second Edition >> <http://www.manning.com/bauer3/> >> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >> Spring Batch in Action <http://www.manning.com/templier/> >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> > > -- Matt Sicker <[email protected]>
