Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, On 14/03/2017 06:03, Ralph Goers wrote: I have fixed the test that located the caller’s Class object with StackWalker. It is now faster than Reflection.getCallerClass() was. I have also created a test to get the Caller’s StackTraceElement with StackWalker. It is now much, much faster

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

I have fixed the test that located the caller’s Class object with StackWalker. It is now faster than Reflection.getCallerClass() was. I have also created a test to get the Caller’s StackTraceElement with StackWalker. It is now much, much faster than walking the Throwable was in Java 8, so

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, I have filed https://bugs.openjdk.java.net/browse/JDK-8176593 for Throwable::getStackTrace performance regression. > On Mar 13, 2017, at 1:57 PM, Daniel Fuchs wrote: > > Hi Ralph, > > On 13/03/17 19:06, Ralph Goers wrote: >> OK - I will do that when I fix

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, On 13/03/17 19:06, Ralph Goers wrote: OK - I will do that when I fix the other bug. But if you can suggest some other way to bail out of the loop after I have found the correct StackFrame that would help as well. I'd suggest using filter + findFirst, possibly with a statefull

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Mar 13, 2017, at 12:04 PM, Daniel Fuchs wrote: > > Hi Ralph, > > I see one issue in your benchmark: > > StackWalker walker = > StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); > > will return a new StackWalker every time it's called. > I'd

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, I see one issue in your benchmark: StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); will return a new StackWalker every time it's called. I'd suggest to create the StackWalker only once and put it in a private (or package) static. This will

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Mar 13, 2017, at 11:54 AM, Ralph Goers wrote: > > >> On Mar 13, 2017, at 11:52 AM, Ralph Goers wrote: >> >>> >>> On Mar 13, 2017, at 11:32 AM, Daniel Fuchs wrote: >>> >>> Hi Ralph, >>> >>> On 13/03/2017

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Mar 13, 2017, at 11:52 AM, Ralph Goers wrote: > >> >> On Mar 13, 2017, at 11:32 AM, Daniel Fuchs wrote: >> >> Hi Ralph, >> >> On 13/03/2017 04:25, Ralph Goers wrote: >>> Sorry for not getting back sooner but I finally found some time

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Mar 13, 2017, at 11:32 AM, Daniel Fuchs wrote: > > Hi Ralph, > > On 13/03/2017 04:25, Ralph Goers wrote: >> Sorry for not getting back sooner but I finally found some time to follow up. >> >> I took a look at >>

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, On 13/03/2017 04:25, Ralph Goers wrote: Sorry for not getting back sooner but I finally found some time to follow up. I took a look at https://www.sitepoint.com/deep-dive-into-java-9s-stack-walking-api/ and

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

On 2017-03-13 16:56, Mandy Chung wrote: On Mar 12, 2017, at 9:25 PM, Ralph Goers wrote: 1. Walking the Throwable StackTraceElements is significantly faster in Java 8 than Java 9. I am not sure what benchmark Brent used but my results differ. I’ll be surprised

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Mar 12, 2017, at 9:25 PM, Ralph Goers wrote: > > Sorry for not getting back sooner but I finally found some time to follow up. > > I took a look at > https://www.sitepoint.com/deep-dive-into-java-9s-stack-walking-api/ >

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Sorry for not getting back sooner but I finally found some time to follow up. I took a look at https://www.sitepoint.com/deep-dive-into-java-9s-stack-walking-api/ and modified the benchmarks that were used there to add a few

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, On 11/05/16 21:00, Ralph Goers wrote: I am not at all familiar with how the stack is actually managed. I was hoping it was just an array and that the index into it was being kept track of. Since we know that we will only be adding more stack elements I would think that deferring

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

I am not at all familiar with how the stack is actually managed. I was hoping it was just an array and that the index into it was being kept track of. Since we know that we will only be adding more stack elements I would think that deferring use of that index wouldn’t be a problem. But since I

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi, On 11/05/16 18:57, Ralph Goers wrote: But my point is that if obtaining the StackFrame and Class could be done so quickly that it wouldn’t add any noticeable overhead we could do that in every Logger.info, debug, etc. call. If we could just get the stack frame index so that we could obtain

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

OK, I will try to give that a try over the next few days and try to see how much overhead it incurs. Ralph > On May 11, 2016, at 9:43 AM, Mandy Chung wrote: > > >> On May 11, 2016, at 9:24 AM, Ralph Goers wrote: >> >> Currently

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

It happens in situations like Foo:method1—>Logger.log—>DBAppender.append—>Hibernate.something—>Logger.log. It is actually pretty common when you create appenders that want to interact with third party libraries. For example, an Appender might use Netty to interact with sockets, Thrift to

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On May 11, 2016, at 9:24 AM, Ralph Goers wrote: > > Currently StackWalker has a getCallerClass method, but not a > getCallerStackFrame. Having that method could also solve the problem, but > only if it is efficient enough that it could be called for every log

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, On 11/05/16 18:14, Ralph Goers wrote: You are correct that most of the time it would be faster to start from frame 0. However, the problem we have with walking the stack from frame 0 is that it is possible to have a situation like

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Currently StackWalker has a getCallerClass method, but not a getCallerStackFrame. Having that method could also solve the problem, but only if it is efficient enough that it could be called for every log event, since when we would be calling it we wouldn’t even know if the user required it. In

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Yes, we need the StakWalker API for other parts of our code where we need the Class so we can determine what ClassLoader to use. For creating a LogEvent with location information we primarily need the class name, method name and line number. However, the “extended” Throwable also returns the

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Ralph, On 10/05/16 19:40, Ralph Goers wrote: The benchmark logs events using Log4j 2’s asynchronous loggers. In the process of doing that it captures the location information using the code below: // LOG4J2-1029 new Throwable().getStackTrace is faster than

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

I should mention - if you actually want to run the test you should checkout the source and then build it. Just run mvn clean install. Once you have done that the log4j-perf module has all the performance tests. Most of the tests have the command line you can use to run the test. Ralph > On

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Here is the link to the source - https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;a=blob;f=log4j-perf/src/main/java/org/apache/logging/log4j/perf/jmh/AsyncAppenderLog4j2LocationBenchmark.java;h=c306ac1a0f475d772b6ccb8afa527dc037d7c646;hb=HEAD

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On May 10, 2016, at 5:17 PM, Ralph Goers wrote: > > OK - I try that. My earlier comparison had shown that StackWalker was much > faster but since walking the Throwable is so much slower I’m now wondering > how much better than Java 7 & 8 it will be. > > Any

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

OK - I try that. My earlier comparison had shown that StackWalker was much faster but since walking the Throwable is so much slower I’m now wondering how much better than Java 7 & 8 it will be. Any idea why walking the Throwable is so much slower? Ralph > On May 10, 2016, at 5:01 PM, Mandy

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

This walks the stack to find a frame with a matching class. Throwable::getStackTrace is taking the entire stack trace. I suggest to try StackWalker and avoids the overhead of dumping the entire stack trace. Mandy > On May 10, 2016, at 10:40 AM, Ralph Goers wrote:

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

The benchmark logs events using Log4j 2’s asynchronous loggers. In the process of doing that it captures the location information using the code below: // LOG4J2-1029 new Throwable().getStackTrace is faster than Thread.currentThread().getStackTrace(). final StackTraceElement[] stackTrace = new

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

What does your benchmark call? Mandy > On May 10, 2016, at 9:49 AM, Ralph Goers wrote: > > I just ran one of the Log4j performance tests that specifically captures > location information. To run the test I do > > java -jar log4j-perf/target/benchmarks.jar >

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

I just ran one of the Log4j performance tests that specifically captures location information. To run the test I do java -jar log4j-perf/target/benchmarks.jar ".*AsyncAppenderLog4j2LocationBenchmark.*" -f 1 -wi 10 -i 20 -t 4 -si true And the results are: java version "1.7.0_80 Benchmark

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

The new test looks good, Mandy. Thanks, -Brent On 05/04/2016 12:36 PM, Mandy Chung wrote: This version includes a new test: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153912/webrev.02/ Daniel - StackFrameInfo class is package-private class In the future this may become a final class

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

This version includes a new test: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153912/webrev.02/ Daniel - StackFrameInfo class is package-private class In the future this may become a final class once we decide what to do with locals and operands in a future release. I don’t worry about

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Mandy, This looks good to me. Regarding final methods, I believe it might be prudent to make toStackTraceElement() final - and possibly also all the getters that call it. It would be imprudent to try to override any of these methods in a subclass without looking at the implementation in the

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Apr 28, 2016, at 11:37 AM, Brent Christian > wrote: > > Hi, Mandy > > It looks good to me. A few minor things: > > StackFrameInfo.java: > > Should getByteCodeIndex() be final ? > It’s a package-private class and so no subclass outside this package anyway.

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi, Mandy It looks good to me. A few minor things: StackFrameInfo.java: Should getByteCodeIndex() be final ? StackWalker.java, line 136: * change ';' to ',' JavaLangInvokeAccess.java, line 40: For the isNative() docs, I would prefer "Returns true if..." to "Tests if..." Some test

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Updated webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153912/webrev.01/index.html I added a new StackFrame::getByteCodeIndex method to return bci and updatedgetFileName and getLineNumber to have the same returned type as in StackTraceElement. From usage perspective, the caller

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

If you record all stack frames, I can believe Throwable is faster because of a recent optimization JDK-8150778 that has been made in jdk9 to improve the Throwable::getStackTraceElements method. Mandy > On Apr 13, 2016, at 11:49 AM, Ralph Goers wrote: > > I did a

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

I did a raw test of StackWalker by itself and the performance was much better than using a Throwable to get the location information. However, I haven’t tested how it will be implemented in Log4j. We still support Java 7 (and will for some time) so we have to find a way to support using

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

It is good to know Log4J is planning to use StackWalker. Thanks for the feedback. I will reconsider. One thing to mention is the patch went in jdk9/hs-rt that will show up in jdk9/dev some time that changes the implementation to create StackTraceElement to get filename and line number. The

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

I had planned on using StackWalker to generate the location information for every logging event. It seems that this change would thus cause the creation of a new StackTraceElement for every logger event. That seems wasteful. Log4j is currently in the process of trying to reduce the number of

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

> On Apr 12, 2016, at 1:34 AM, Rémi Forax wrote: > > Hi Mandy, > I really don't like this patch. > > Being forced to call toStackElement to get the line number is counter > intuitive. > I would prefer the two methods to not return Optional but an int and a String > with

Re: Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Hi Mandy, I really don't like this patch. Being forced to call toStackElement to get the line number is counter intuitive. I would prefer the two methods to not return Optional but an int and a String with the same convention as StackElement if the point of this patch is to remove the

Review request 8153912: StackFrame::getFileName and StackFrame::getLineNumber not needed

Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153912/webrev.00/index.html StackFrame::getFileName and StackFrame::getLineNumber are originally proposed with the view of any stack walking code can migrate to the StackWalker API without the use of StackTraceElement. File name