Hi Nick,
Thanks for the patch.
JEP 176 [1] describes the caller-sensitive method and the need for a
mechanical checking of caller-sensitive methods. Also Peter Levart in
[2] explained the change in MethodHandles.Lookup related to @CS. I
assume you understand the rationale behind and the potential security
issues. In general defining caller-sensitive API is discouraged.
Defining a SE supported @CallerSensitive and also getCallerClass API
poses the risk of "encouraging" developers to implement more @CS methods
while not fully understand its implication. It was a non-goal of JEP
176 to provide @CallerSensitive as a public API and I would suggest not
to define it as a public API in JDK 8.
While I'll take the time to look at your patch, I would like to restart
the discussion from the use cases (in which led to what you summarized
the need of your proposed API [3]):
1. Groovy 1.x and 2.x use the sun.reflect.Reflection.getCallerClass(int
depth) method to:
* emulates the way ResourceBundle.getBundle(String, Locale) works.
Groovy runtime introduces intermediary stack frame between a caller
and the call to getBundle, these frames needs to be filtered out;
when looking for the caller classloader.
* support the annotation @Grab, @Grab allows to specify a dependency
between a code and a module (using apache ivy under the hood). The
resolution is done at runtime and require a specific Classloader
(the GroovyClassLoader), getCallerClass is used to find the class
loader of the caller, again filtering out the intermediary stack frame.
Groovy 3.x has a different implementation that doesn't need to do stack
walk to filter its runtime frames and find the caller.
2. Log4j
Log4j offers "extended stack trace" patterns that requires access to a
Class object when exceptions or stack traces are logged to provide
additional information for troubleshooting such as the implementation
version of the Package, code source, etc. It currently uses the
sun.reflect.Reflection.getCallerClass(int depth) method to find the
Class object matching the StackTraceElement in a Throwable or stack
trace. If the sun.reflect.Reflection.getCallerClass(int depth) method
doesn't exist, it will use SecurityManager.getClassContext().
This approach is not reliable since the HotSpot implementation of
getCallerClass filters out the frames corresponding to reflection
machinery (its intent is to find the true caller) whereas a stack trace
contains all Java frames up to MaxJavaStackTraceDepth (default is 1024).
When there is no Class object matching a StackTraceElement, it will fall
back and load the class (does Log4j know which class loader to use?)
Log4j currently works in the absence of the
sun.reflect.Reflection.getCallerClass(int depth) method but performance
is a very major issue.
3. APIs on behalf of the caller
For example, locating a resource on behalf of the caller class to avoid
explicit additional Class and/or ClassLoader parameters.
Please correct/add if I miss anything.
More will be discussed tomorrow.
Mandy
[1] http://openjdk.java.net/jeps/176
[2]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019397.html
[3]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019334.html
On 9/1/2013 1:16 AM, Nick Williams 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
