RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only (original) (raw)

Volker Simonis volker.simonis at gmail.com
Wed May 3 14:39:29 UTC 2017

Fine with me!

Regards, Volker

On Wed, May 3, 2017 at 3:02 PM, Ioi Lam <ioi.lam at oracle.com> wrote:

Andrew replied me off-list that he tested the aarch64 part and was happy about it. Thanks Andrew.

So if there if no further comment, I will push the code as is. Thanks - Ioi

On 4/29/17 8:30 AM, Ioi Lam wrote:

I've updated the patch to include Volker's ppc/s390 port as well as his comments. I've also included an updated patch (untested) for aarch64 for Andrew Haley to test: Full patch http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v04/ Delta from the previous version http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v04.delta/ Thanks - Ioi On 4/28/17 4:30 AM, Ioi Lam wrote:

On 4/25/17 8:06 AM, Volker Simonis wrote: On Mon, Apr 24, 2017 at 2:18 PM, Ioi Lam <ioi.lam at oracle.com> wrote: Hi Volker,

On 4/21/17 12:02 AM, Volker Simonis wrote: Hi Ioi, thanks once again for considering our ports! Please find the required additions for ppc64/s390x in the following webrew (which is based upon your latest v03 patch): http://cr.openjdk.java.net/~simonis/webrevs/2017/8171392ppc64s390x/ Thanks for the patch. I will integrate it and post an updated webrev. @Martin/@Lucy: could you please have a look at my ppc64/s390x assembly code. I did some tests and I think it should be correct, but maybe you still find some improvements :) Besides that, I have some general questions/comments regarding your change: 1. In constantPool.hpp, why don't you declare the 'nameindex' and 'resolvedklassindex' fields with type 'jushort'? As far as I can see, they can only hold 16-bit values anyway. It would also save you some space and several asserts (e.g. in unresolvedklassatput():

274 assert((nameindex & 0xffff0000) == 0, "must be"); 275 assert((resolvedklassindex & 0xffff0000) == 0, "must be"); I think the HotSpot convention is to use ints as parameter and return types, for values that are actually 16-bits or less, like here in constantPool.hpp: void fieldatput(int which, int classindex, int nameandtypeindex) { tagatput(which, JVMCONSTANTFieldref); intataddr(which) = ((jint) nameandtypeindex<<16) |_ _classindex;_ _}_ _I am not sure what the reasons are. It could be that the parameters_ _usually_ _need to be computed arithmetically, and it's much easier for the caller_ _of_ _the method to use ints -- otherwise you will get lots of compiler_ _warnings_ _which would force you to use lots of casting, resulting in code that's_ _hard_ _to read and probably incorrect._ _OK, but you could still use shorts in the the object to save space,_ _although I'm not sure how much that will save in total. But if nobody_ _else cares, I'm fine with the current version._ _The CPKlassSlot objects are stored only on the stack, so the savings is_ _not worth the trouble of adding extract type casts._ _Inside the ConstantPool itself, the nameindex and resolvedklassindex_ _are stored as a pair of 16-bit values._ _2. What do you mean by:_ _106 // ... will be changed to support compressed pointers_ _107 Array<Klass*> resolvedklasses; Sorry the comment isn't very clear. How about this? 106 // Consider using an array of compressed klass pointers to // save space on 64-bit platforms. 107 Array<Klass*>* resolvedklasses; Sorry I still didn't get it? Do you mean you want to use array of "narrowKlass" (i.e. unsigned int)? But using compressed class pointers is a runtime decision while this is a compile time decision. I haven't figured out how to do it yet :-) Most likely, it will be something like: union { Array<Klass*>* X; Array* Y; } resolvedklasses; and you need to decide at run time whether to use X or Y. - Ioi 3. Why don't we need the call to "releasetagatput()" in "klassatput(int classindex, Klass* k)"? "klassatput(int classindex, Klass* k)" is used from "ClassFileParser::fillinstanceklass() and before your change that function used the previous version of "klassatput(int classindex, Klass* k)" which did call "releasetagatput()". Good catch. I'll add the following, because the class is now resolved. releasetagatput(classindex, JVMCONSTANTUnresolvedClass); 4. In ConstantPool::copyentryto() you've changed the behavior for tags JVMCONSTANTClass, JVMCONSTANTUnresolvedClass, JVMCONSTANTUnresolvedClassInError. Before, the resolved klass was copied to the new constant pool if one existed but now you always only copy a classindex to the new constant pool (even if a resolved klass existed). Is that OK? E.g. won't this lead to a new resolving for the new constant pool and will this have performance impacts or other side effects? I think Coleen has answered this in a separate mail :-) Thanks - Ioi Thanks again for doing this nice change and best regards, Volker On Sun, Apr 16, 2017 at 11:33 AM, Ioi Lam <ioi.lam at oracle.com> wrote: Hi Lois, I have updated the patch to include your comments, and fixes the handling of anonymous classes. I also added some more comments regarding the tempresolvedklassindex: (delta from last webrev)

http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v03.delta/ (full webrev) http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v03/ Thanks - Ioi On 4/15/17 2:31 AM, Lois Foltan wrote: On 4/14/2017 11:30 AM, Ioi Lam wrote:

On 4/14/17 1:31 PM, Ioi Lam wrote: HI Lois, Thanks for the review. Please see my comments in-line. On 4/14/17 4:32 AM, Lois Foltan wrote: Hi Ioi, Looks really good. A couple of comments: src/share/vm/classfile/classFileParser.cpp: * line #5676 - I'm not sure I completely understand the logic surrounding anonymous classes. Coleen and I discussed earlier today and I came away from that discussion with the idea that the only classes being patched currently are anonymous classes. Line 5676 ... 5676 if (isanonymous()) { 5677 maxnumpatchedklasses ++; // for patching the class index 5678 } corresponds to 5361 ik->setname(classname); 5362 5363 if (isanonymous()) { 5364 // I am well known to myself 5365 patchclass(ik->constants(), thisclassindex, ik, ik->name()); // eagerly resolve 5366 } Even though the class is "anonymous", it actually has a name. ik->name() probably is part of the constant pool, but I am not 100% sure. Also, I would need to search the constant pool to find the index for ik->name(). So I just got lazy here and use the same logic in ConstantPool::patchclass() to append ik->name() to the end of the constant pool. "Anonymous" actually means "the class cannot be looked up by name in the SystemDictionary". I think we need a better terminology :-) I finally realized why we need the "eagerly resolve" on line 5365. I'll modify the comments to the following: // thisclassindex is a CONSTANTClass entry that refers to this // anonymous class itself. If this class needs to refer to its own methods or // fields, it would use a CONSTANTMethodRef, etc, which would reference // thisclassindex. However, because this class is anonymous (it's // not stored in SystemDictionary), thisclassindex cannot be resolved // with ConstantPool::klassatimpl, which does a SystemDictionary lookup. // Therefore, we must eagerly resolve thisclassindex now. So, Lois is right. Line 5676 is not necessary. I will revise the code to do the "eager resolution" without using ClassFileParser::patchclass. I'll post the updated code later. Thanks Ioi for studying this and explaining! Look forward to seeing the updated webrev. Lois Thanks - Ioi So a bit confused as why the check on line #5676 and a check for a java/lang/Class on line #5684. 5683 Handle patch = cppatchat(i); 5684 if (javalangString::isinstance(patch()) || javalangClass::isinstance(patch())) { 5685 // We need to append the names of the patched classes to the end of the constant pool, 5686 // because a patched class may have a Utf8 name that's not already included in the 5687 // original constant pool. 5688 // 5689 // Note that a String in cppatchat(i) may be used to patch a Utf8, a String, or a Class. 5690 // At this point, we don't know the tag for index i yet, because we haven't parsed the 5691 // constant pool. So we can only assume the worst -- every String is used to patch a Class. 5692 maxnumpatchedklasses ++; Line 5684 checks for all objects in the cppatch array. Later, when ClassFileParser::patchconstantpool() is called, any objects that are either Class or String could be treated as a Klass: 724 void ClassFileParser::patchconstantpool(ConstantPool* cp, 725 int index, 726 Handle patch, 727 TRAPS) { ... 732 switch (cp->tagat(index).value()) { 733 734 case JVMCONSTANTUnresolvedClass: { 735 // Patching a class means pre-resolving it. 736 // The name in the constant pool is ignored. 737 if (javalangClass::isinstance(patch())) { 738 guaranteeproperty(!javalangClass::isprimitive(patch()), 739 "Illegal class patch at %d in class file %s", 740 index, CHECK); 741 Klass* k = javalangClass::asKlass(patch()); 742 patchclass(cp, index, k, k->name()); 743 } else { 744 guaranteeproperty(javalangString::isinstance(patch()), 745 "Illegal class patch at %d in class file %s", 746 index, CHECK); 747 Symbol* const name = javalangString::assymbol(patch(), CHECK); 748 patchclass(cp, index, NULL, name); 749 } 750 break; 751 } Could the isanonymous() if statement be combined into the loop? I think the answer is no. At line 5365, there is no guarantee that ik->name() is in the cppatch array. 5365 patchclass(ik->constants(), thisclassindex, ik, ik->name()); // eagerly resolve Also why not do this calculation in the rewriter or is that too late? Line 5676 and 5684 need to be executed BEFORE the constant pool and the associated tags array is allocated (both of which are fixed size, and cannot be expanded), which is way before the rewriter is run. At this point, we don't know what cp->tagat(index) is (line #732), so the code needs to make a worst-case estimate on how long the CP/tags should be. * line #5677, 5692 - a nit but I think the convention is to not have a space after the variable name and between the post increment operator. Fixed. src/share/vm/classfile/constantPool.hpp: I understand the concept behind invalidresolvedklassindex, but it really is not so much invalid as temporary for class redefinition purposes, as you explain in ConstantPool::allocateresolvedklasses. Please consider renaming to tempunresolvedklassindex. And whether you choose to rename the field or not, please add a one line comment ahead of ConstantPool::tempunresolvedklassatput that only class redefinition uses this currently. Good idea. Will do. Thanks - Ioi Great change, thanks! Lois On 4/13/2017 4:56 AM, Ioi Lam wrote: Hi Coleen, Thanks for the comments. Here's a delta from the last patch

http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v02/ In addition to your requests, I made these changes: [1] To consolidate the multiple extracthigh/low code, I've added CPKlassSlot, so the code is cleaner: CPKlassSlot kslot = thiscp->klassslotat(which); int resolvedklassindex = kslot.resolvedklassindex(); int nameindex = kslot.nameindex(); [2] Renamed ConstantPool::issharedquick() to ConstantPool::isshared(). The C++ compiler should be able to pick this function over MetaspaceObj::isshared(). [3] Massaged the CDS region size set-up code a little to pass internal tests, because RO/RW ratio has changed. I didn't spend too much time picking the "right" sizes, as this code will be obsoleted soon with JDK-8072061 Thanks - Ioi On 4/13/17 6:40 AM, coleen.phillimore at oracle.com wrote: This looks really good!

http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v01/src/share/vm/oops/constantPool.cpp.udiff.html + // Add one extra element to tags for storing ConstantPool::flags(). + Array* tags = MetadataFactory::newwriteablearray(loaderdata, length+1, 0, CHECKNULL); ... + assert(tags->length()-1 == length, "invariant"); // tags->at(length) is flags() I think this is left over, since flags didn't get moved after all. + Klass** adr = thiscp->resolvedklasses()->adrat(resolvedklassindex); + OrderAccess::releasestoreptr((Klass* volatile *)adr, k); + // The interpreter assumes when the tag is stored, the klass is resolved + // and the Klass* is a klass rather than a Symbol*, so we need + // hardware store ordering here. + thiscp->releasetagatput(which, JVMCONSTANTClass); + return k; The comment still refers to the switch between Symbol* and Klass* in the constant pool. The entry in the Klass array should be NULL. + int nameindex = extracthighshortfromint(*intataddr(which)); Can you put klassnameindexat() in the constantPool.hpp header file (so it's inlined) and have all the places where you get nameindex use this function? So we don't have to know in multiple places that extracthighshortfromint() is where the name index is. And in constantPool.hpp, for unresolvedklassatput() add a comment about what the new format of the constant pool entry is {nameindex, resolvedklassindex}. I'm happy to see this work nearing completion! The "constant" pool should be constant! thanks, Coleen On 4/11/17 2:26 AM, Ioi Lam wrote: Hi,please review the following change https://bugs.openjdk.java.net/browse/JDK-8171392 http://cr.openjdk.java.net/~iklam/jdk10/8171392makeconstantpoolreadonly.v01/ Summary:* * Before: + ConstantPool::klassat(i) finds the Klass from the i-th slot of ConstantPool. + When a klass is resolved, the ConstantPool is modified to store the Klass pointer. After: + ConstantPool::klassat(i) finds the at this->resolvedklasses->at(i) + When a klass is resolved, resolvedklasses->at(i) is modified. In addition: + I moved resolvedreferences and referencemap from ConstantPool to ConstantPoolCache. + Now flags is no longer modified for shared ConstantPools. As a result, none of the fields in shared ConstantPools are modified at run time, so we can move them into the RO region in the CDS archive. Testing:* * - Benchmark results show no performance regression, despite the extra level of indirection, which has a negligible overhead for the interpreter. - RBT testing for tier2 and tier3. Ports:* * - I have tested only the Oracle-support ports. For the aarch64, ppc and s390 ports, I have added some code without testing (it's probably incomplete) - Port owners, please check if my patch work for you, and I can incorporate your changes in my push. Alternatively, you can wait for my push and provide fixes (if necessary) in a new changeset, and I will be happy to sponsor it. Thanks - Ioi

More information about the hotspot-dev mailing list