[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 5-rd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2015-01-28 18:57:23
Message-ID: 54C93113.7000308 () oracle ! com
[Download RAW message or body]
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@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@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@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@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@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@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@oracle.com wrote:
> > > > > > > > > > > Please, review the fix for:
> > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8008678
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Open webrev:
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 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:
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > >
> > > > > > > > > > > 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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Thank you, Coleen!<br>
Serguei<br>
<br>
On 1/28/15 6:28 AM, Coleen Phillimore wrote:<br>
</div>
<blockquote cite="mid:54C8F1FF.3050301@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<br>
Yes, this looks great. Ship it!<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 1/27/15, 6:57 PM, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:54C825CE.1030105@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">This is 5-th round review of for:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8008678">https://bugs.openjdk.java.net/browse/JDK-8008678</a>
<br>
<br>
There was one (4-rd) private review discussion. The result is
this webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2015/hotspot/8008678-JVMTI-pseudo \
.5/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8008678-JVMTI-pseudo.5/</a><br>
<br>
<br>
Summary:<br>
Current fix is to use the 2-nd LSB bit in the CPSlot to
mark pseudo-strings.<br>
A similar version of webrev was already reviewed by Coleen
and John.<br>
The update is that it includes John's suggestion to
introduce the CPSlot::<span class="new">TagBits to make code
more clean.</span><br>
Also, I've discovered that there is no need to update the
SymbolClosure::load_symbol() in iterator.hpp<br>
because the pseudo-string PCSlot is not used in the
SymbolClosure.<br>
I need a final thumbs up for the push.<br>
<br>
Testing:<br>
Run:<br>
- nsk.jvmti.testlist, nsk.jdi.testlist, vm.mlvm.testlist<br>
- java/lang/instrument, com/sun/jdi and
hotspot/test/runtime tests<br>
- new jtreg test (see webrev) that was written by Filipp
Zhinkin<br>
<br>
\
<br> Thanks,<br>
Serguei<br>
<br>
On 1/16/15 3:01 PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:54B99838.10601@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">John R. suggested to use the
CPSlot(Symbol* ptr) to mark pseudo-strings.<br>
The updated webrev is going to be close to the .3 webrev.<br>
I will send it soon.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 1/16/15 2:53 PM, Coleen Phillimore wrote:<br>
</div>
<blockquote cite="mid:54B99682.8070802@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
This change looks good to me also.<br>
Thanks,<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 1/16/15, 3:07 PM, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:54B96F97.5070204@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Please, review the fix for:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8008678">https://bugs.openjdk.java.net/browse/JDK-8008678</a>
<br>
<br>
<br>
Open webrev:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2015/hotspot/8008678-JVMTI-pseudo \
.3/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8008678-JVMTI-pseudo.3/</a><br>
<br>
<br>
Summary:<br>
Currently, a JVM_CONSTANT_String CP entry having a
NULL reference to Symbol*<br>
indicates that it is a pseudo-string (patched
string).<br>
This creates nasty issues for the constant pool
reconstitution.<br>
<br>
Current suggestion is to avoid having a NULL
reference and retain the original<br>
Symbol* reference for pseudo-strings. The
pseudo-string indication will be<br>
if the Utf8 representation starts from
"CONSTANT_PLACEHOLDER_".<br>
This approach makes the fix much simpler.<br>
<br>
I need a confirmation from the Compiler team that
this won't break any assumptions or invariants. <br>
Big thanks to Coleen for previous round reviews and
good advices!<br>
<br>
<br>
Testing:<br>
Run:<br>
- java/lang/instrument tests<br>
- new jtreg test (see webrev) that was written by
Filipp Zhinkin<br>
<br>
\
<br> Thanks,<br>
Serguei<br>
<br>
<br>
On 12/18/14 2:00 PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:54934E73.4080808@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Coleen,<br>
<br>
Thank you for reviewing!<br>
<br>
<br>
On 12/18/14 11:23 AM, Coleen Phillimore wrote:<br>
</div>
<blockquote cite="mid:5493299F.60005@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Serguei,<br>
<br>
Thank you for making the patches an optional field.<br>
<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo \
.2/src/share/vm/oops/constantPool.hpp.sdiff.html">http://cr.openjdk.java.net/~sspitsyn \
/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/oops/constantPool.hpp.sdiff.html</a><br>
<meta charset="utf-8">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: \
normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; \
word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(238, 238, \
238);"><span class="new" style="color: blue; font-weight: bold;"> 198 if \
(!patched()) {</span> <span class="new" style="color: blue; font-weight: bold;"> 199 \
assert(false, "a pseudo-string map may exists for patched CP only");</span> <span \
class="new" style="color: blue; font-weight: bold;"> 200 return 0;</span> <span \
class="new" style="color: blue; font-weight: bold;"> 201 }</span></pre> Why \
not<br>
assert(patched(), "a pseud-string \
map must exist for patched CP only");<br>
</blockquote>
<br>
Wanted it to be more reliable but it looks pointless.<br>
Will make this change.<br>
<br>
<blockquote cite="mid:5493299F.60005@oracle.com"
type="cite"> <br>
<br>
Why is this? Is this really a ShouldNotReachHere?
should it be false?<br>
<br>
<meta charset="utf-8">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: \
normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; \
word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(238, 238, \
238);"><span class="new" style="color: blue; font-weight: bold;"> 215 \
assert(true, "not found a matching entry in pseudo-string map");</span></pre> \
</blockquote> <br>
<br>
A typo, must be false.<br>
It is the last minute change.<br>
Thanks for the catch!<br>
<br>
<br>
<blockquote cite="mid:5493299F.60005@oracle.com"
type="cite"> <br class="Apple-interchange-newline">
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo \
.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html">http://cr.openjdk.java.net/ \
~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html</a><br>
<br>
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?<br>
</blockquote>
<br>
Thank you for asking this question.<br>
If I understand correctly, the patching comes from the
compiler side for anonymous classes only.<br>
I saw it for LambdaForm's only.<br>
I think, the patching can not be changed with a
retransformation.<br>
But I'm not sure if it can not be changed with a
redefinition.<br>
<br>
But if it can - then it would be safe to merge the
'patched' condition, i.e. make it patched<br>
if either the_class or scratch class is patched.<br>
<br>
<blockquote cite="mid:5493299F.60005@oracle.com"
type="cite"> <br>
Somehow I thought you'd have to save the value of the
cp_patches oops passed in.<br>
<br>
So I was wondering why you can't change this instead:<br>
<br>
bool is_pseudo_string_at(int which) {<br>
// A pseudo string is a string that doesn't have a
symbol in the cpSlot<br>
- return unresolved_string_at(which) == NULL;<br>
+ return
(strncmp(unresolved_string_at(which)->as_utf8(),
"CONSTANT_PLACEHOLDER_"
<meta charset="utf-8">
, strlen("CONSTANT_PLACEHOLDER")) == 0);<br>
}<br>
</blockquote>
<br>
I was thinking about the same but was not sure if it
would work for the compiler team.<br>
We have to ask John about this (added John and Christian
to the cc-list).<br>
This question to John was in my plan! :)<br>
<br>
The beauty of the above approach is that there is no
need to create an intermediate<br>
pseudo-string map and most of the code in from this
webrev is not needed.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote cite="mid:5493299F.60005@oracle.com"
type="cite"> <br>
And the asserts in the other functions below it.<br>
<br>
Coleen<br>
<br>
<br>
<div class="moz-cite-prefix">On 12/17/14, 12:26 PM, <a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
\
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote cite="mid:5491BCB9.10505@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Please, review the
second round fix for:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="https://bugs.openjdk.java.net/browse/JDK-8008678">https://bugs.openjdk.java.net/browse/JDK-8008678</a>
<br>
<br>
Open webrev:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo \
.2/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/</a><br>
<br>
<br>
Summary:<br>
<br>
This fix implements a footprint saving approach
suggested by Coleen.<br>
To be able to reconstitute a class constant
pool, an intermediate pseudo-string map is used.<br>
Now, this field is accounted optionally, only if
the 'cp_patches' is provided in the<br>
ClassFileParser::parseClassFile()
<meta http-equiv="content-type"
content="text/html; charset=utf-8">
before ConstantPool is allocated.<br>
This fix is not elegant, even a little bit ugly,
but it is the only way I see so far.<br>
<br>
Unfortunately, this approach did not help much
to make some other fields (eg., 'operands')
optional.<br>
The problem is that we have to account optional
fields before parsing, at the CP allocation time.<br>
It is possible to re-allocate the ConstantPool
when any InvokeDynamic bytecode is discovered,<br>
but it looks too complicated.<br>
<br>
Testing:<br>
- the unit test from bug report<br>
- nsk.jvmti,testlist, nsk.jdi.testlist, JTREG
java/lang/instrument<br>
- vm.mlvm.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist (in progress)<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/26/14 11:53 AM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
\
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote cite="mid:54762FC9.2010800@oracle.com"
type="cite">Coleen, <br>
<br>
Thank you for looking at this! <br>
I'll check how this can be improved. <br>
It is my concern too. <br>
<br>
Thanks, <br>
Serguei <br>
<br>
On 11/26/14 9:17 AM, Coleen Phillimore wrote: <br>
<blockquote type="cite"> <br>
Serguei, <br>
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. <br>
<br>
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. <br>
<br>
Thanks, <br>
Coleen <br>
<br>
On 11/26/14, 3:59 AM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
\
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: <br>
<blockquote type="cite">Please, review the fix
for: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="https://bugs.openjdk.java.net/browse/JDK-8008678">https://bugs.openjdk.java.net/browse/JDK-8008678</a>
<br>
<br>
<br>
Open webrev: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo \
.1/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.1/</a>
<br>
<br>
<br>
Summary: <br>
The pseudo-strings are currently not
supported in reconstitution of constant pool.
<br>
<br>
This is an explanation from John Rose about
what the pseudo-strings are: <br>
<br>
"We still need "live" oop constants
pre-linked into the constant pool of bytecodes
which <br>
implement some method handles. We use the
anonymous class pseudo-string feature for
that. <br>
The relevant code is here: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/in \
voke/InvokerBytecodeGenerator.java">http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java</a>
<br>
These oops are what "pseudo-strings" are.
<br>
The odd name refers to the fact that, even
though they are random oops, they appear in
the constant pool <br>
where one would expect (because of class
file syntax) to find a string." <br>
... <br>
If you really wanted to reconstitute a
class file for an anonymous class, and <br>
if that class has oop patching
(pseudo-strings), you would need either to (a)
reconstitute the patches array <br>
handed to Unsafe.defineAnonymousClass, or
(b) accept whatever odd strings were there
first, as an approximation. <br>
The "odd strings" are totally
insignificant, and are typically something
like "CONSTANT_PLACEHOLDER_42" <br>
(see
java/lang/invoke/InvokerBytecodeGenerator.java)."
<br>
<br>
<br>
Reconstitution of the ConstantPool is
needed for both the JVMTI GetConstantPool()
and RetransformClasses(). <br>
Finally, it goes to the
ConstantPool::copy_cpool_bytes(). <br>
<br>
The problem is that a pseudo-string is a
patched string that does not have <br>
a reference to the string symbol anymore: <br>
unresolved_string_at(idx) == NULL <br>
<br>
The fix is to create and fill in a map from
JVM_CONSTANT_String cp index to the
JVM_CONSTANT_Utf8 cp index <br>
to be able to restore this assotiation in
the JvmtiConstantPoolReconstituter. <br>
<br>
Testing: <br>
Run: <br>
- java/lang/instrument tests <br>
- new jtreg test (see webrev) that was
written by Filipp Zhinkin <br>
<br>
<br>
Thanks, <br>
Serguei <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic