Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8

[Jörn Huxhorn]

This looks pretty awesome!

There are just two small things I'd suggest:

Rename
StackTraceFrame[] getCurrentStackTrace()
to
StackTraceFrame[] getStackFrames() (I'd prefer this one…)
or
StackTraceFrame[] getStackTraceFrames() (…while this one is probably more accurate.)
or
StackTraceFrame[] getCurrentStackFrames()
or
StackTraceFrame[] getCurrentStackTraceFrames()

Rename
StackTraceFrame[] getStackTrace(Throwable throwable)
to
StackTraceFrame[] getStackFrames(Throwable throwable) (I'd prefer this one…)
or
StackTraceFrame[] getStackTraceFrames(Throwable throwable)  (…while this one is probably more accurate.)


The rationale for this is that previous get[..]StackTrace methods always returned StackTraceElement[] while get[..]StackFrames methods would then always return a StackTraceFrame[]. So essentially I'd like to establish the notion of either StackFrames or StackTraceFrames.

I did a quick visual review and I didn't find anything horrific or questionable - which obviously wasn't a security audit, just a sanity check.

And, yes, https is broken. I got the patches by using http instead.

Cheer,
Jörn.

On 1. September 2013 at 10:17:51, Nick Williams (nicholas+openjdk at nicholaswilliams.net) wrote:
I have completed and am proposing a patch for replacing sun.reflect.Reflection#getCallerClass(...) with a public API in Java 8. I saw no point in duplicating an issue, even though it's over 10 years old, so I'm indicating that this is a patch for 4851444 (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4851444).  

I welcome and solicit support/+1s and a sponsor. Someone about a month ago had mentioned that they would be willing to be a sponsor if the patch looked good, but I can't remember who it was and I can't find the email. I want to say it was someone with RedHat, but my memory could be faulty, so please don't hold it against me if I'm wrong.  

*Summary of Changes*  
Added the new class java.lang.StackTraceFrame. It's a virtual mirror of StackTraceElement, except that it contains a Class<?> declaringClass property instead of a String className property. Since the list members expressed reluctance to add methods to Thread and Throwable, StackTraceFrame also contains several static methods for retrieving Classes and StackTraceFrames from the stack. These methods are as follows:  

Class<?> getCallerClass(): Retrieves the class of the caller of the method calling getCallerClass(). This is identical to the new Reflection#getCallerClass() added in Java 7u25/8.  

Class<?> getCallerClass(int): Retrieves the class of the caller n frames down the stack. This is identical to the old Reflection#getCallerClass(int) that was deprecated in Java 7u25 and removed in Java 8.  

StackTraceFrame getCallerFrame(): Retrieves the StackTraceFrame of the line of code that called the method calling getCallerClass(). This is similar to the new Reflection#getCallerClass() added in Java 7u25/8, except that it returns a StackTraceFrame.  

StackTraceFrame getCallerFrame(int): Retrieves the StackTraceFrame of the caller n frames down the stack. This is similar to the old Reflection#getCallerClass(int), except that it returns a StackTraceFrame.  

StackTraceFrame[] getCurrentStackTrace(): Functionally equivalent to Thread#getStackTrace(), except that it returns an array of StackTraceFrames.  

StackTraceFrame[] getStackTrace(Throwable throwable): Functionally equivalent to Throwable#getStackTrace(), except that it returns an array of StackTraceFrames. It uses the same save point (backtrace) created when the Throwable is created that Throwable#getStackTrace() uses when it's first called. It caches the array of StackTraceFrames in the Throwable just like the array of StackTraceElements are cached, so that multiple calls for the same Throwable are fast.  

As a result of this change, sun.reflect.CallerSensitive has been moved to java.lang.CallerSensitive.  

I spent considerable time reviewing, revising, considering, and testing these changes. I included several unit tests that establish the proper behavior. I also spent considerable time benchmarking the changes. While benchmarking, I noticed some things. First, getCallerClass() and getCallerClass(int) are as fast as their counterparts in sun.reflect.Reflection, and they easily inline when appropriate. Second, getCallerFrame() and getCallerFrame(int) are /almost/ as fast as the Class versions, but there is some additional overhead for the construction of the StackTraceFrame. This is minuscule (1,000,000 invocations consume around 500 ms total on my machine). At this point, all of the benchmarking was as expected.  

However, I then encountered a surprise. The getCurrentStackTrace() and getStackTrace(Throwable) methods executed (again, 1,000,000 times) in about 70% of the time that Thread#getStackTrace() and Throwable#getStackTrace() did, respectively. Theoretically, they should have executed in the same amount of time, not faster. After extensive analysis, I discovered (what I considered to be) a serious flaw in how the stack trace is filled in within Throwable (which also affects how Thread#getStackTrace() works).  

Instead of simply iterating over the entire save point and filling in the Throwable stack trace in native code (which is what I did when I implemented the StackTraceFrame methods), the Java code in Throwable first called a native method to figure out how deep the stack was, then called another native method once for every frame in the stack to retrieve each element individually. This native method that is called repeatedly iterates over the entire backtrace once for each call, stopping only when it finds the matching element (so it's O(1) for the first call, O(2) for the second call, O(3) for the third call, and so on). While my StackTraceFrame methods were iterating over the backtrace exactly 1 time (O(n)), the Throwable methods were iterating over the backtrace 1+(n/2) times (worse than O(nlogn) but not as bad as O(n^2)). This problem would not have been extremely apparent over small stack traces (the 30% improvement was a stack trace of 6 elements), but over a large (200+ elements) stack traces the performance difference would be huge and noticeable. Seeing a room for improvement, I refactored the code that fills in the stack trace for Throwable, improving its performance accordingly to match the performance of the StackTraceFrame methods.  

I'm very pleased with how this turned out, and both the unit tests and my extensive functional testing show that the new class and its methods are working great. I just need someone willing to review and sponsor my patch!  

*The Code Changes*  
I couldn't get WebRev to run without all kinds of errors. So I ran `hg diff -g` on every repository in the forest with changes. Here are the four patch files for the four repositories that had changes (no other repositories had changes):  

https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8.patch  
https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_jdk.patch  
https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_hotspot.patch  
https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_nashorn.patch  

I believe I have followed all of the procedures as closely as possible. I await feedback and hope for some support on this, so that we can get a public replacement for this method in Java 8. Let me know if you have any questions.  

Thanks!  

Nick