3-rd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jan 16 20:07:51 UTC 2015
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/0d43de0e/attachment-0001.html>
More information about the serviceability-dev
mailing list