5-rd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Jan 28 18:57:23 UTC 2015
Thank you, Coleen!
Serguei
On 1/28/15 6:28 AM, Coleen Phillimore wrote:
>
> Yes, this looks great. Ship it!
> Coleen
>
> On 1/27/15, 6:57 PM, serguei.spitsyn at oracle.com wrote:
>> This is 5-th round review of for:
>> https://bugs.openjdk.java.net/browse/JDK-8008678
>>
>> There was one (4-rd) private review discussion. The result is this
>> webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8008678-JVMTI-pseudo.5/
>>
>>
>> Summary:
>> Current fix is to use the 2-nd LSB bit in the CPSlot to mark
>> pseudo-strings.
>> A similar version of webrev was already reviewed by Coleen and John.
>> The update is that it includes John's suggestion to introduce the
>> CPSlot::TagBits to make code more clean.
>> Also, I've discovered that there is no need to update the
>> SymbolClosure::load_symbol() in iterator.hpp
>> because the pseudo-string PCSlot is not used in the SymbolClosure.
>> I need a final thumbs up for the push.
>>
>> Testing:
>> Run:
>> - nsk.jvmti.testlist, nsk.jdi.testlist, vm.mlvm.testlist
>> - java/lang/instrument, com/sun/jdi and hotspot/test/runtime tests
>> - new jtreg test (see webrev) that was written by Filipp Zhinkin
>>
>>
>> Thanks,
>> Serguei
>>
>> On 1/16/15 3:01 PM, serguei.spitsyn at oracle.com wrote:
>>> John R. suggested to use the CPSlot(Symbol* ptr) to mark pseudo-strings.
>>> 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/20150128/980107d8/attachment-0001.html>
More information about the serviceability-dev
mailing list