3-rd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Jan 16 22:53:54 UTC 2015
This change looks good to me also.
Thanks,
Coleen
On 1/16/15, 3:07 PM, 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/2015/hotspot/8008678-JVMTI-pseudo.3/
>
>
> Summary:
> Currently, a JVM_CONSTANT_String CP entry having a NULL reference
> to Symbol*
> indicates that it is a pseudo-string (patched string).
> This creates nasty issues for the constant pool reconstitution.
>
> Current suggestion is to avoid having a NULL reference and retain
> the original
> Symbol* reference for pseudo-strings. The pseudo-string indication
> will be
> if the Utf8 representation starts from "CONSTANT_PLACEHOLDER_".
> This approach makes the fix much simpler.
>
> I need a confirmation from the Compiler team that this won't break
> any assumptions or invariants.
> Big thanks to Coleen for previous round reviews and good advices!
>
>
> Testing:
> Run:
> - java/lang/instrument tests
> - new jtreg test (see webrev) that was written by Filipp Zhinkin
>
>
> Thanks,
> Serguei
>
>
> On 12/18/14 2:00 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Coleen,
>>
>> Thank you for reviewing!
>>
>>
>> On 12/18/14 11:23 AM, Coleen Phillimore wrote:
>>>
>>> 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");
>>
>> Wanted it to be more reliable but it looks pointless.
>> Will make this change.
>>
>>>
>>>
>>> Why is this? Is this really a ShouldNotReachHere? should it be false?
>>>
>>> 215 assert(true, "not found a matching entry in pseudo-string map");
>>
>>
>> A typo, must be false.
>> It is the last minute change.
>> Thanks for the catch!
>>
>>
>>>
>>> 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?
>>
>> Thank you for asking this question.
>> If I understand correctly, the patching comes from the compiler side
>> for anonymous classes only.
>> I saw it for LambdaForm's only.
>> I think, the patching can not be changed with a retransformation.
>> But I'm not sure if it can not be changed with a redefinition.
>>
>> But if it can - then it would be safe to merge the 'patched'
>> condition, i.e. make it patched
>> if either the_class or scratch class is patched.
>>
>>>
>>> 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);
>>> }
>>
>> I was thinking about the same but was not sure if it would work for
>> the compiler team.
>> We have to ask John about this (added John and Christian to the cc-list).
>> This question to John was in my plan! :)
>>
>> The beauty of the above approach is that there is no need to create
>> an intermediate
>> pseudo-string map and most of the code in from this webrev is not needed.
>>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> 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/20150116/2f9dd926/attachment-0001.html>
More information about the serviceability-dev
mailing list