Gary, looks like the code in ReflectionUtil already has a comment with a reference to the log4j-perf benchmark. (Nice, Matt!) The duplication in this case is just "new Throwable()", so I think we're good.
On Thu, Nov 12, 2015 at 1:16 AM, Matt Sicker <[email protected]> wrote: > I know I used that in ReflectionUtil because of the log4j-perf test that > demonstrates the difference between Throwable and Thread. > > On 11 November 2015 at 10:11, Gary Gregory <[email protected]> wrote: > >> I suggest you add a comment there too or preferably refactor the >> duplication. >> >> Gary >> On Nov 11, 2015 8:05 AM, "Remko Popma" <[email protected]> wrote: >> >>> I agree I should have added a ref to the JIRA to document the reason for >>> doing it this way. I added a comment just now. >>> >>> About moving this to a util class, I'd be fine with that. >>> FYI, the other place that uses {{new Throwable().getStackTrace()}} is >>> ReflectionUtil#getEquivalentStackTraceElement. >>> >>> >>> On Thu, Nov 12, 2015 at 12:48 AM, Gary Gregory <[email protected]> >>> wrote: >>> >>>> I think this needs a code comment to avoid the code being changed in the >>>> future to undo the improvement. >>>> >>>> IMO all perf changes like this need good docs with a ref to the Jira. >>>> Who >>>> knows how this kind of stuff will play out on top on Java 8 and 9. >>>> >>>> If we do this kind of call in different places, we should abstract it >>>> out >>>> as well in a util method which can be amply documented. >>>> >>>> Gary >>>> >>>> On Nov 11, 2015 5:48 AM, <[email protected]> wrote: >>>> >>>> > Repository: logging-log4j2 >>>> > Updated Branches: >>>> > refs/heads/master 381acc0e3 -> 65adfab1b >>>> > >>>> > >>>> > LOG4J2-1029 Performance improvement when gathering location >>>> information >>>> > >>>> > - Use new Throwable().getStackTrace() instead of >>>> > Thread.currentThread().getStackTrace() >>>> > >>>> > Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo >>>> > Commit: >>>> > http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/65adfab1 >>>> > Tree: >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/65adfab1 >>>> > Diff: >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/65adfab1 >>>> > >>>> > Branch: refs/heads/master >>>> > Commit: 65adfab1b6afcbffde767b5d8a3213341c84c1eb >>>> > Parents: 381acc0 >>>> > Author: rpopma <[email protected]> >>>> > Authored: Wed Nov 11 22:48:42 2015 +0900 >>>> > Committer: rpopma <[email protected]> >>>> > Committed: Wed Nov 11 22:48:42 2015 +0900 >>>> > >>>> > ---------------------------------------------------------------------- >>>> > .../java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java | >>>> 2 +- >>>> > src/changes/changes.xml | >>>> 3 +++ >>>> > 2 files changed, 4 insertions(+), 1 deletion(-) >>>> > ---------------------------------------------------------------------- >>>> > >>>> > >>>> > >>>> > >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/65adfab1/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java >>>> > ---------------------------------------------------------------------- >>>> > diff --git >>>> > >>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java >>>> > >>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java >>>> > index d74d6e2..a6c2087 100644 >>>> > --- >>>> > >>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java >>>> > +++ >>>> > >>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java >>>> > @@ -542,7 +542,7 @@ public Log4jLogEvent(final String loggerName, >>>> final >>>> > Marker marker, final String >>>> > if (fqcnOfLogger == null) { >>>> > return null; >>>> > } >>>> > - final StackTraceElement[] stackTrace = >>>> > Thread.currentThread().getStackTrace(); >>>> > + final StackTraceElement[] stackTrace = new >>>> > Throwable().getStackTrace(); >>>> > StackTraceElement last = null; >>>> > for (int i = stackTrace.length - 1; i > 0; i--) { >>>> > final String className = stackTrace[i].getClassName(); >>>> > >>>> > >>>> > >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/65adfab1/src/changes/changes.xml >>>> > ---------------------------------------------------------------------- >>>> > diff --git a/src/changes/changes.xml b/src/changes/changes.xml >>>> > index c82cc9d..e6b2f97 100644 >>>> > --- a/src/changes/changes.xml >>>> > +++ b/src/changes/changes.xml >>>> > @@ -42,6 +42,9 @@ >>>> > <action issue="LOG4J2-1187" dev="ggregory" type="add"> >>>> > Support use case for >>>> > java.sql.DriverManager.setLogStream(PrintStream). >>>> > </action> >>>> > + <action issue="LOG4J2-1029" dev="rpopma" type="fix" >>>> due-to="Stefan >>>> > Leonhartsberger"> >>>> > + Performance improvement when gathering location information. >>>> > + </action> >>>> > <action issue="LOG4J2-1172" dev="rpopma" type="fix"> >>>> > Fixed ThreadLocal leak [AsyncLogger$Info] on Tomcat when >>>> using >>>> > AsyncLoggerContextSelector. >>>> > </action> >>>> > >>>> > >>>> >>>> --001a11419fd83af63a052445b36d >>>> Content-Type: text/html; charset=UTF-8 >>>> Content-Transfer-Encoding: quoted-printable >>>> >>>> <p dir=3D"ltr">I think this needs a code comment to avoid the code >>>> being ch= >>>> anged in the future to undo the improvement. </p> >>>> <p dir=3D"ltr">IMO all perf changes like this need good docs with a ref >>>> to = >>>> the Jira. Who knows how this kind of stuff will play out on top on Java >>>> 8 a= >>>> nd 9. </p> >>>> <p dir=3D"ltr">If we do this kind of call in different places, we >>>> should ab= >>>> stract it out as well in a util method which can be amply >>>> documented.</p> >>>> <p dir=3D"ltr">Gary</p> >>>> <div class=3D"gmail_quote">On Nov 11, 2015 5:48 AM, <<a >>>> href=3D"mailto:= >>>> [email protected]">[email protected]</a>> wrote:<br >>>> type=3D"attribution"= >>>> ><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 >>>> .8ex;border-left:1= >>>> px #ccc solid;padding-left:1ex">Repository: logging-log4j2<br> >>>> Updated Branches:<br> >>>> =C2=A0 refs/heads/master 381acc0e3 -> 65adfab1b<br> >>>> <br> >>>> <br> >>>> LOG4J2-1029 Performance improvement when gathering location >>>> information<br> >>>> <br> >>>> - Use new Throwable().getStackTrace() instead of<br> >>>> Thread.currentThread().getStackTrace()<br> >>>> <br> >>>> Project: <a href=3D" >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/r= >>>> epo" rel=3D"noreferrer" target=3D"_blank"> >>>> http://git-wip-us.apache.org/repo= >>>> s/asf/logging-log4j2/repo</a><br> >>>> Commit: <a href=3D" >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/co= >>>> mmit/65adfab1" rel=3D"noreferrer" target=3D"_blank"> >>>> http://git-wip-us.apach= >>>> e.org/repos/asf/logging-log4j2/commit/65adfab1</a><br> >>>> Tree: <a href=3D" >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree= >>>> /65adfab1" rel=3D"noreferrer" target=3D"_blank"> >>>> http://git-wip-us.apache.or= >>>> g/repos/asf/logging-log4j2/tree/65adfab1</a><br> >>>> Diff: <a href=3D" >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff= >>>> /65adfab1" rel=3D"noreferrer" target=3D"_blank"> >>>> http://git-wip-us.apache.or= >>>> g/repos/asf/logging-log4j2/diff/65adfab1</a><br> >>>> <br> >>>> Branch: refs/heads/master<br> >>>> Commit: 65adfab1b6afcbffde767b5d8a3213341c84c1eb<br> >>>> Parents: 381acc0<br> >>>> Author: rpopma <<a href=3D"mailto:[email protected]";> >>>> [email protected]</= >>>> a>><br> >>>> Authored: Wed Nov 11 22:48:42 2015 +0900<br> >>>> Committer: rpopma <<a href=3D"mailto:[email protected] >>>> ">[email protected]= >>>> g</a>><br> >>>> Committed: Wed Nov 11 22:48:42 2015 +0900<br> >>>> <br> >>>> >>>> ----------------------------------------------------------------------<br> >>>> =C2=A0.../java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java=C2=A0 >>>> = >>>> =C2=A0 | 2 +-<br> >>>> =C2=A0src/changes/changes.xml=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 >>>> =C2= >>>> =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 >>>> =C2=A0 = >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 3 +++<br> >>>> =C2=A02 files changed, 4 insertions(+), 1 deletion(-)<br> >>>> >>>> ----------------------------------------------------------------------<br> >>>> <br> >>>> <br> >>>> <a href=3D" >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/65adf= >>>> >>>> ab1/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEve= >>>> nt.java" rel=3D"noreferrer" target=3D"_blank"> >>>> http://git-wip-us.apache.org/= >>>> >>>> repos/asf/logging-log4j2/blob/65adfab1/log4j-core/src/main/java/org/apache/= >>>> logging/log4j/core/impl/Log4jLogEvent.java</a><br> >>>> >>>> ----------------------------------------------------------------------<br> >>>> diff --git >>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Lo= >>>> g4jLogEvent.java >>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/i= >>>> mpl/Log4jLogEvent.java<br> >>>> index d74d6e2..a6c2087 100644<br> >>>> --- >>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogE= >>>> vent.java<br> >>>> +++ >>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogE= >>>> vent.java<br> >>>> @@ -542,7 +542,7 @@ public Log4jLogEvent(final String loggerName, final >>>> Mar= >>>> ker marker, final String<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (fqcnOfLogger =3D=3D null) {<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return null;<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}<br> >>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 final StackTraceElement[] stackTrace =3D >>>> Threa= >>>> d.currentThread().getStackTrace();<br> >>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 final StackTraceElement[] stackTrace =3D >>>> new T= >>>> hrowable().getStackTrace();<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0StackTraceElement last =3D null;<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (int i =3D stackTrace.length - 1; >>>> i &= >>>> gt; 0; i--) {<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0final String className >>>> =3D = >>>> stackTrace[i].getClassName();<br> >>>> <br> >>>> <a href=3D" >>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/65adf= >>>> ab1/src/changes/changes.xml" rel=3D"noreferrer" target=3D"_blank"> >>>> http://gi= >>>> >>>> t-wip-us.apache.org/repos/asf/logging-log4j2/blob/65adfab1/src/changes/chan= >>>> ges.xml</a><br> >>>> >>>> ----------------------------------------------------------------------<br> >>>> diff --git a/src/changes/changes.xml b/src/changes/changes.xml<br> >>>> index c82cc9d..e6b2f97 100644<br> >>>> --- a/src/changes/changes.xml<br> >>>> +++ b/src/changes/changes.xml<br> >>>> @@ -42,6 +42,9 @@<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0<action issue=3D"LOG4J2-1187" >>>> dev= >>>> =3D"ggregory" type=3D"add"><br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Support use case for >>>> java.sql.DriverManag= >>>> er.setLogStream(PrintStream).<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0</action><br> >>>> +=C2=A0 =C2=A0 =C2=A0 <action issue=3D"LOG4J2-1029" >>>> dev=3D&quo= >>>> t;rpopma" type=3D"fix" due-to=3D"Stefan >>>> Leonhartsberger= >>>> "><br> >>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Performance improvement when gathering >>>> locatio= >>>> n information.<br> >>>> +=C2=A0 =C2=A0 =C2=A0 </action><br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0<action issue=3D"LOG4J2-1172" >>>> dev= >>>> =3D"rpopma" type=3D"fix"><br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Fixed ThreadLocal leak >>>> [AsyncLogger$Info]= >>>> on Tomcat when using AsyncLoggerContextSelector.<br> >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0</action><br> >>>> <br> >>>> </blockquote></div> >>>> >>>> --001a11419fd83af63a052445b36d-- >>>> >>> >>> > > > -- > Matt Sicker <[email protected]> >
