2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings

[Coleen Phillimore]

Hi Serguei,

Thank you for making the patches an optional field.

http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html

  198     if (!patched()) {
  199       assert(false, "a pseudo-string map may exists for patched CP only");
  200       return 0;
  201     }

Why not
                 assert(patched(), "a pseud-string map must exist for 
patched CP only");


Why is this?   Is this really a ShouldNotReachHere?  should it be false?

  215     assert(true, "not found a matching entry in pseudo-string map");


http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html

Don't you have to move the value of the patched field from the old 
constant pool to the new one?  I hate to ask but is there merging that 
needs to be done?   I don't know how to write this test case though.  Is 
it possible to redefine a class with a constant pool patches with 
another that has constant pool patches?

Somehow I thought you'd have to save the value of the cp_patches oops 
passed in.

So I was wondering why you can't change this instead:

   bool is_pseudo_string_at(int which) {
     // A pseudo string is a string that doesn't have a symbol in the cpSlot
-    return unresolved_string_at(which) == NULL;
+   return (strncmp(unresolved_string_at(which)->as_utf8(), 
"CONSTANT_PLACEHOLDER_" , strlen("CONSTANT_PLACEHOLDER")) == 0);
   }

And the asserts in the other functions below it.

Coleen


On 12/17/14, 12:26 PM, serguei.spitsyn at oracle.com wrote:
> Please, review the second round fix for:
> https://bugs.openjdk.java.net/browse/JDK-8008678
>
> Open webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/
>
>
> Summary:
>
>   This fix implements a footprint saving approach suggested by Coleen.
>   To be able to reconstitute a class constant pool, an intermediate 
> pseudo-string map is used.
>   Now, this field is accounted optionally, only if the 'cp_patches' is 
> provided in the
>   ClassFileParser::parseClassFile() before ConstantPool is allocated.
>   This fix is not elegant, even a little bit ugly, but it is the only 
> way I see so far.
>
>   Unfortunately, this approach did not help much to make some other 
> fields (eg., 'operands') optional.
>   The problem is that we have to account optional fields before 
> parsing, at the CP allocation time.
>   It is possible to re-allocate the ConstantPool when any 
> InvokeDynamic bytecode is discovered,
>   but it looks too complicated.
>
> Testing:
>   - the unit test from bug report
>   - nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument
>   - vm.mlvm.testlist, vm.quick.testlist, 
> vm.parallel_class_loading.testlist (in progress)
>
>
> Thanks,
> Serguei
>
>
> On 11/26/14 11:53 AM, serguei.spitsyn at oracle.com wrote:
>> Coleen,
>>
>> Thank you for looking at this!
>> I'll check how this can be improved.
>> It is my concern too.
>>
>> Thanks,
>> Serguei
>>
>> On 11/26/14 9:17 AM, Coleen Phillimore wrote:
>>>
>>> Serguei,
>>> I had a quick look at this.  I was wondering if we could make the 
>>> pseudo_string_map conditional in ConstantPool and not make all 
>>> classes pay in footprint for this field?  The same thing probably 
>>> could be done for operands too.  There are flags that you can set to 
>>> conditionally add a pointer to base() in this function.
>>>
>>> Typical C++ would subclass ConstantPool to add 
>>> InvokeDynamicConstantPool fields, but this is not typical C++ so the 
>>> trick we use is like the one in ConstMethod.   I think it's worth 
>>> doing in this case.
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 11/26/14, 3:59 AM, serguei.spitsyn at oracle.com wrote:
>>>> Please, review the fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8008678
>>>>
>>>>
>>>> Open webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.1/ 
>>>>
>>>>
>>>>
>>>> Summary:
>>>>    The pseudo-strings are currently not supported in reconstitution 
>>>> of constant pool.
>>>>
>>>>    This is an explanation from John Rose about what the 
>>>> pseudo-strings are:
>>>>
>>>>    "We still need "live" oop constants pre-linked into the constant 
>>>> pool of bytecodes which
>>>>     implement some method handles. We use the anonymous class 
>>>> pseudo-string feature for that.
>>>>     The relevant code is here:
>>>> http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java 
>>>>
>>>>     These oops are what "pseudo-strings" are.
>>>>     The odd name refers to the fact that, even though they are 
>>>> random oops, they appear in the constant pool
>>>>     where one would expect (because of class file syntax) to find a 
>>>> string."
>>>>      ...
>>>>     If you really wanted to reconstitute a class file for an 
>>>> anonymous class, and
>>>>     if that class has oop patching (pseudo-strings), you would need 
>>>> either to (a) reconstitute the patches array
>>>>     handed to Unsafe.defineAnonymousClass, or (b) accept whatever 
>>>> odd strings were there first, as an approximation.
>>>>     The "odd strings" are totally insignificant, and are typically 
>>>> something like "CONSTANT_PLACEHOLDER_42"
>>>>     (see java/lang/invoke/InvokerBytecodeGenerator.java)."
>>>>
>>>>
>>>>    Reconstitution of the ConstantPool is needed for both the JVMTI 
>>>> GetConstantPool() and RetransformClasses().
>>>>    Finally, it goes to the ConstantPool::copy_cpool_bytes().
>>>>
>>>>    The problem is that a pseudo-string is a patched string that 
>>>> does not have
>>>>    a reference to the string symbol anymore:
>>>>        unresolved_string_at(idx) == NULL
>>>>
>>>>    The fix is to create and fill in a map from JVM_CONSTANT_String 
>>>> cp index to the JVM_CONSTANT_Utf8 cp index
>>>>    to be able to restore this assotiation in the 
>>>> JvmtiConstantPoolReconstituter.
>>>>
>>>> Testing:
>>>>   Run:
>>>>    - java/lang/instrument tests
>>>>    - new jtreg test (see webrev) that was written by Filipp Zhinkin
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141218/6fdfc126/attachment-0001.html>