2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Dec 18 23:20:28 UTC 2014
On 12/18/14 3:01 PM, Coleen Phillimore wrote:
>
> On 12/18/14, 5:23 PM, serguei.spitsyn at oracle.com wrote:
>> 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.
>>
>> Forgot to answer this good question.
>>
>> If the cp_patches can be changed with a class redefinition (it,
>> probably, can)
>> then it needs to be saved and merged too.
>> Can we separate this cp_patches merge issue into another bug or RFE?
>> The need still has to be clarified though, I'll check this with John.
>
> Yes, it seems good to have a follow up bug for this once the
> requirement is understood. It may be a fair bit of work.
Ok, thanks!
Serguei
>
> Thanks,
> Coleen
>
>>
>>
>> Thanks,
>> Serguei
>>
>>
>>>>
>>>> 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/20141218/2e8252ee/attachment-0001.html>
More information about the serviceability-dev
mailing list