JDK-8151155 : [REDO] Remove Method::_method_data for C1
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 9
  • Priority: P3
  • Status: Closed
  • Resolution: Won't Fix
  • Submitted: 2016-03-03
  • Updated: 2016-03-12
  • Resolved: 2016-03-11
Related Reports
Cloners :  
Relates :  
Relates :  
Relates :  
Description
This is item #8 from JDK-8138571. Method::_method_data appears to only be needed for C2. It can be removed when only C1 is supported to save footprint.
Comments
There are C1 dependencies on the mdo, causing performance issues if the mdo is forced to null. See comments near the end of JDK-8147978. Closing as "won't fix" since this footprint opt is more trouble than it's worth.
11-03-2016

1. Current c1/c2 compiler detection is rely on presence of certain fields (see VM.java fragment below). So useClientCompiler is set only if neither C2 no JVMCI is present i.e. we can check if (!VM.getVM().isClientCompiler()) { ... }. But please file a followup bug (assign it to me) to fix compiler detection and make it JVMCI aware. // We infer the presence of C1 or C2 from a couple of fields we // already have present in the type database { Type type = db.lookupType("Method"); if (type.getField("_from_compiled_entry", false, false) == null) { // Neither C1 nor C2 is present usingClientCompiler = false; usingServerCompiler = false; } else { // Determine whether C2 is present if (db.lookupType("Matcher", false) != null) { usingServerCompiler = true; } else { usingClientCompiler = true; } } } 2. Correct. We have to put some efforts to make sure that nobody access interpreter*Count directly and make all accessors null-aware with proper comments.
09-03-2016

Two issues with that solution. First, it does not take into account that the fields are available if JVMCI is enabled. Either we need to just always call getCIntegerField and check for the exception, or SA needs to become JVMCI aware and provide something like VM.getVM().isJVMCCompiler(). The other issue is that checks are needed elsewhere to defend against referencing interpreterInvocationCountField or interpreterThrowoutCountField if null. I think the checks are needed here: public int interpreterInvocationCount() { return (int) interpreterInvocationCountField.getValue(this); } public int interpreterThrowoutCount() { return (int) interpreterThrowoutCountField.getValue(this); }
08-03-2016

I think about something like: + if (VM.getVM().isServerCompiler()) { + // interpreter counters is not available in client VM + interpreterInvocationCountField = new CIntField(type.getCIntegerField("_interpreter_invocation_count"), 0); + interpreterThrowoutCountField = new CIntField(type.getCIntegerField("_interpreter_throwout_count"), 0); + } i.e. don't check for field presence, but explicitly address the problem.
07-03-2016

I think the fix in getAddressField should work fine. For the two CIntFields, I'd like to suggest in these two cases returning 0 when they don't exist is OK. These are two fields that have always been present, but without C2 or JVMCI, would never be used and therefore always return 0 anyway. So if we remove them but continue to return 0, that is actually keeping this all behind the scenes from the perspective of the SA user. It also seems easier and cleaner to fix it in this way rather than track down the users of these fields and add extra logic there to print out warning messages.
07-03-2016

For address field, I think, we need not to distinguish between field is NOT PRESENT and filed is NULL so the fix is straightforward (below), but CIntField is different - we can't just return 0 if it's not present so we have to *handle exception* and change the code according to it's semantics e.g. print something like "not available" instead of digital value. diff -r 27b6bff990d5 src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/types/basic/BasicType.java --- a/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/types/basic/BasicType.java Thu Mar 03 17:33:13 2016 +0000 +++ b/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/types/basic/BasicType.java Mon Mar 07 12:54:52 2016 +0300 @@ -279,7 +279,7 @@ public AddressField getAddressField(String fieldName) { // This type can not be inferred (for now), so provide a wrapper - Field field = getField(fieldName); + Field field = getField(fieldName, true, false); if (field == null) { return null; }
07-03-2016

The following tests were failing (see JDK-8151130): serviceability/sa/DeadlockDetectionTest.java serviceability/sa/TestClassLoaderStats.java closed/compiler/c2/4671460/CharArrTest.java closed/compiler/c2/4671460/CharArrTest.java
03-03-2016

The root of the issues seems to be the following in sun.jvm.hotspot.oops.Method.initialize(): methodData = type.getAddressField("_method_data"); I looks like there is only one direct user of methodData, which is Method.getMethodData. The only user of Method.getMethodData() is the printmdo command, which checks if Method.getMethodData() returns null. So it looks like setting methodData = null when it doesn't exist will resolve this issue. I can think two way of doing this: 1. Catch the RuntimeException thrown by the type.getAddressField("_method_data"), and set methodData null when it is caught. My concern here would be if the RuntimeException is for some reason other than the field not being found. Do we really want to continue in that case? Maybe a more specific exception is needed. 2. Detect the COMPILER2_OR_JVMCI_PRESENT condition like we do in vmStructs.cpp, and only only try to get the Method::_method_data field when it is true. ATM I don't know how to do this. I see VM.isClientCompiler() and VM.isServerCompiler(), but nothing to indicate if JVMCI is present. The changes for JDK-8148639 I was just about to send out for review will need something similar. The following is from MethodCounters::initialize() and both of these fields are conditionally present with JDK-8148639: interpreterInvocationCountField = new CIntField(type.getCIntegerField("_interpreter_invocation_count"), 0); interpreterThrowoutCountField = new CIntField(type.getCIntegerField("_interpreter_throwout_count"), 0);
03-03-2016

When redoing the fix, please make sure that the execution time of the closed/compiler/c2/4671460/CharArrTest.java test does not increase.
03-03-2016

The original fix failed and was backed out. I created this issue to re-attempt fixing.
03-03-2016