On 4/1/13 5:24 PM, John Rose wrote:
On Mar 27, 2013, at 10:35 AM, Mandy Chung <mandy.ch...@oracle.com
<mailto:mandy.ch...@oracle.com>> wrote:
This is the JDK change for JEP 176: JEP 176: Mechanical Checking of
Caller-Sensitive Methods [1]. Christian has posted the webrev for
the hotspot VM change a couple weeks ago [2].
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7198429/webrev.00/
<http://cr.openjdk.java.net/%7Emchung/jdk8/webrevs/7198429/webrev.00/>
This is very good work. It makes certain aspects of the system much
easier to understand.
I have reviewed it all, with the following comments:
Thanks for the review.
The transform moves caller sensitivity into a clearly visible
position. In many cases, the call to Reflection.getCallerClass now
precedes the check of the security manager. For some customer codes,
this may cause a performance regression, although it will probably be
in the noise in most cases.
Agree. I have restored the cases that should find caller class lazily.
If necessary, we can replace some uses of R.getCC() by something like
(S.getSecurityManager() == null ? null : R.getCC()), recovering the
original laziness of the stack check. This won't be necessary
everywhere. Note that the server compiler can usually remove the
stack walk. We may need to put something similar into the client
compiler.
In the case of the reflective calls (Field, Constructor, Method), the
check of the caller class is gated by both AccessibleObject.override
and quickCheckMemberAccess, and you have preserved this for
Constructor and Method (which are probably the most important
performance case). Consider gating it also for Field, as noted below.
Per-file comments:
-- Class.java
(See below about checking for checkMemberAccess override.)
Done.
-- ClassLoader.java
Did you consider using Class.cast to get a runtime check instead of
@SuppressWarnings("unchecked")?
Good point - I have modified it to do runtime check using Class.asSubclass.
-- MethodHandleNatives.java
Was this deletion intentional or was it something Eclipse or Netbeans did?
-import static java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP;
It was unintentional and I fixed it differently when I realized the
reference of IMPL_LOOKUP. I now have restored to the original import
static to be consistent with other files.
Good cleanups!
You can remove this since rAPC is static (error in original code):
case "registerAsParallelCapable":
return canBeCalledVirtual(mem, java.lang.ClassLoader.class);
Done.
Note that the subsequent call to canBeCalledVirtual will disqualify
mem because it is static.
Yes.
-- MethodHandles.Lookup
Now that stack walking is out of the picture, make all the
constructors private, and remove this scary comment:
* <p>
* Also, don't make it private, lest javac interpose
* an access$N method.
Done.
I like this comment, and the manual inlining technique (in this very
special case):
// Inlined SecurityManager.checkMemberAccess
Since it tends to get lost in the previous comment block, I suggest
you put it after the open brace of the inlined body.
Also, both parameters should be inlined at the top of the block, not
just "which", so the rest of the block can a more or less verbatim copy.
+ { // Inlined SecurityManager.checkMemberAccess
+ final Class<?> clazz = refc/defc;
+ final int which = Member.PUBLIC/PRIVATE;
I considered that too and made the change. I actually took out the
redundant check (which != PUBLIC) since we know the value.
As previously noted, smgr.getClass() == SecurityManager.class is too
strict. Suggest hoisting the nasty logic into a boolean, and using it
twice:
boolean standardCMA = (smgr.getClass() == SecurityManager.class);
if (!standardCMA) try {
standardCMA = smgr.getClass().getDeclaredMethod("checkMemberAccess",
...).getDeclaringClass() == SecurityManager.class;
} catch (ReflectiveOperationException ex) { throw new InternalError(ex); }
if (standardCMA) { // // Inlined SecurityManager.checkMemberAccess
final Class<?> clazz = refc/defc;
final int which = Member.PUBLIC/PRIVATE;
...
} else {
smgr.checkMemberAccess(...);
}
OK. I modified a little. For subclass case, I can simply check if
Class.getDeclaredMethod finds the method; if exists, it's overrided;
otherwise, NoSuchMethodException thrown indicates using the standard one.
(And a similar comment for Class.java.)
Done.
Lots of trouble for a corner case!
-- Field.java
Consider making R.getCC more lazy as follows:
+ return getFieldAccessor(obj, needsMemberAccessCheck() ?
Reflection.getCallerClass() : clazz).get(obj);
+ private boolean needsMemberAccessCheck() {
+ return !override && !Reflection.quickCheckMemberAccess(clazz,
modifiers);
+ }
(This might affect the performance of serialization.)
-- CallerSensitiveFinder.java
This is a remarkable test. Does it statically determine whether the
annotations are missing?
Yes - that's the purpose to catch if any use of
Reflection.getCallerClass() but not missing @CS annotation.
Does it process all of rt.jar? How long does it take to complete?
It processes all JRE classes (i.e. rt.jar, jar files in JAVA_HOME/lib
and JAVA_HOME/lib/ext).
I have made it done the minimal necessary work only. It takes 6 seconds
on my MacMini (2GHz core i7 and 8GB memory) to parse 24243 classes. It
takes 5-14 seconds on the jprt machines and here are some sample timing:
linux_i586 jdk_lang took 14 mins and CallerSensitiveFinder took 8.5 sec
macosx_x64 jdk_lang took 20.5 mins and CallerSensitiveFinder took
14.5 sec
solaris_i586 jdk_lang took 15.5 mins and CallerSensitiveFinder took
10 sec
windows_x64 jdk_lang took 16.5 mins and CallerSensitiveFinder took
10 sec
And (I have to ask, though I'm sure the answer will be yes) have you
run it on broken inputs (such as
boot.GetCallerClass.missingCallerSensitiveAnnotation) and verified
that it barks at them?
I ran it on an older JDK that calls Reflection.getCallerClass(int) and
no @sun.reflect.CallerSensitive exist.
The test is currently tailored-made to only parses JRE classes. I can
easily extend it to parse additional classes like boot.GetCallerClass
that is a good test case to this tool itself. Will do so.
I'll post a new webrev once I finish the testing.
thanks for the detailed review.
Mandy
