RFR 8029178: Parallel class loading test anonymous-simple gets SIGSEGV in Metaspace:,:contains (original) (raw)

Jon Masamitsu jon.masamitsu at oracle.com
Mon Jan 6 11:48:23 PST 2014

Looks good.

Jon

On 01/03/2014 12:00 PM, Coleen Phillmore wrote:

On 1/3/2014 2:38 PM, Jon Masamitsu wrote:

On 1/3/2014 11:00 AM, Coleen Phillmore wrote:

Jon, Thanks for reviewing this. On 1/3/2014 1:09 PM, Jon Masamitsu wrote: Coleen,

http://cr.openjdk.java.net/~coleenp/8029178/src/share/vm/memory/metachunk.hpp.frames.html

146 bool contains(const void* ptr) { return bottom() < ptr && ptr <= top; } I think bottom() points to the first chunk. Also top points to the start of the unallocated space (next place where a chunk will be allocated). So I would think this should be bool contains(const void* ptr) { return bottom() <=ptr && ptr < top; }

Yes, I agree. http://cr.openjdk.java.net/~coleenp/8029178/src/share/vm/memory/metaspace.cpp.frames.html Don't you need some locking to protect the "chunksinuse()" lists? 2382 bool SpaceManager::contains(const void *ptr) { 2383 for (ChunkIndex i = ZeroIndex; i < NumberOfInUseLists; i =_ _nextchunkindex(i))_ _2384 {_ _2385 Metachunk* curr = chunksinuse(i);_ _2386 while (curr != NULL) {_ _2387 if (curr->contains(ptr)) return true; 2388 curr = curr->next(); 2389 } 2390 } 2391 return false; 2392 } 2393 If new chunks are added while this is iterating over them, they are added to the front of the list. This is ok because we would only call this for metadata that is already allocated in one of the chunks. I don't want to add a lock but I do need some memory ordering instructions. I'll move the one that I had on virtual space lists to where we add the chunks. Ok. Do you think it is worth code in setchunksinuse() that checks that the list head is set after the new chunk has been added? Something like setchunksinuse(newchunk) if (chunksinuse() != NULL) assert(newchunk->next == chunksinuse(), "Always add new chunk to list first"); } I think the OrderAccess::storestore() should accomplish this. chunksinuse() shouldn't be null after this though except for at deletion time. So this code seems overly paranoid about the C++ compiler not doing the stores. Actually the C++ compiler could keep all of this in a register and we'd never see things in the right order. Here's a new version with the OrderAccess::storestore(); I reran the parallel class loading tests on this. http://cr.openjdk.java.net/~coleenp/80291782/ <http://cr.openjdk.java.net/%7Ecoleenp/80291782/> Thanks, Coleen Just a thought. Jon The Metaspace/SpaceManager destructors can only be called for the unloading list, which is created at a safepoint and we don't walk those lists in CLDG::contains. Thank you for pointing these out. I'll retest and send out another webrev. Coleen Jon On 1/2/2014 9:25 AM, Coleen Phillmore wrote: Summary: Metaspace::contains cannot look at purged metaspaces while CMS concurrently deallocates them. Removed 2 calls to ismetaspaceobject where the object may be in a deallocated metaspace. Removed walking virtual space lists for determining contains because the virtual space list can change concurrently with the walk. CLDG::contains is slower but no slowdowns with testing were observed. Tested by SQE testbase tests, jtreg tests. Functional testing by parallel class loading tests and nsk/coverage/arguments/arguments008 (ie. calls Method::isvalidmethod) open webrev at http://cr.openjdk.java.net/~coleenp/8029178/ bug link https://bugs.openjdk.java.net/browse/JDK-8029178 Thanks, Coleen

More information about the hotspot-dev mailing list