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
Tue Jan 27 23:57:02 UTC 2015
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/20150127/d8acd356/attachment-0001.html>
More information about the serviceability-dev
mailing list