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 23:10:13 UTC 2015
On 1/16/15 3:03 PM, Coleen Phillimore wrote:
>
> On 1/16/15, 6:01 PM, serguei.spitsyn at oracle.com wrote:
>> John R. suggested to use the CPSlot(Symbol* ptr) to mark pseudo-strings.
>
> I was sort of wondering about this along the same lines. You're
> setting the second bit, right? :)
I'm not sure yet.
I'll check if setting the first bit would not create an ambiguity.
Otherwise, will set the second one.
Thanks,
Serguei
> Coleen
>
>> The updated webrev is going to be close to the .3 webrev.
>> I will send it soon.
>>
>> Thanks,
>> Serguei
>>
>> On 1/16/15 2:53 PM, Coleen Phillimore wrote:
>>>
>>> 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/5c0f2904/attachment-0001.html>
More information about the serviceability-dev
mailing list