[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR: 8186706: ArchivedObjectCache obj_hash() is broken
From: Jiangli Zhou <jiangli.zhou () oracle ! com>
Date: 2017-08-25 19:12:32
Message-ID: ACADE5A2-6067-4EFC-90B5-1983FCDE8839 () oracle ! com
[Download RAW message or body]
> On Aug 25, 2017, at 11:53 AM, coleen.phillimore@oracle.com wrote:
>
>
>
> On 8/25/17 2:08 PM, Jiangli Zhou wrote:
> >
> > > On Aug 25, 2017, at 5:49 AM, coleen.phillimore@oracle.com \
> > > <mailto:coleen.phillimore@oracle.com> wrote:
> > >
> > >
> > > On 8/24/17 7:32 PM, Jiangli Zhou wrote:
> > > > Hi Coleen,
> > > >
> > > > Thanks for reviewing this!
> > > >
> > > > > On Aug 24, 2017, at 1:31 PM, coleen.phillimore@oracle.com \
> > > > > <mailto:coleen.phillimore@oracle.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > I'm glad that you changed this to CHEAP allocated.
> > > > >
> > > > > + CDS_JAVA_HEAP_ONLY(_archive_object_cache = new (ResourceObj::C_HEAP, \
> > > > > mtInternal)ArchivedObjectCache(););
> > > > >
> > > > > This should probably be mtClass and not mtInternal.
> > > > Ok. I'll use mtClass.
> > > >
> > > > > The other question I had was that I expected you to use obj->hash() since \
> > > > > the objects probably should (?) have a hashcode installed when in the \
> > > > > archive.
> > > > Do you mean identity_hash()? That should also work for this use case. \
> > > > Initially I wanted to use a simply hash code so went with computation using \
> > > > object address. Yes, we compute the identity_hash for archived object at dump \
> > > > time. I updated the webrev:
> > >
> > > The only thing to worry about is if identity_hash() can go to a safepoint here. \
> > > But don't the strings already have an identity hash installed in the markOop?
> >
> > Not all strings have identity hash installed yet. During object archiving, we \
> > compute identity hash for all archived object right before we copy the objects. \
> > The change causes the identity hash being computed slightly earlier, but still \
> > during the object archiving. Object archiving is guarded by NoSafepointVerifier.
>
>
> Okay, I keep not remembering why it's safe to call identity_hash() but this doesn't \
> change the situation, since you're calling it under the same NSV.
> One thing that might remind me is if you add something like:
>
> static unsigned obj_hash(oop const& p) {
> - unsigned hash = (unsigned)((uintptr_t)&p);
> - return hash ^ (hash >> LogMinObjAlignment);
> + assert(p->mark()->has_bias_pattern, "this object should never have been \
> locked"); // so identity_hash won't safepoint + unsigned hash = \
> (unsigned)p->identity_hash(); + return hash;
> }
>
> The change looks good, especially since you're already adding the hash code.
I'll add the assert and double check with my tests.
Thanks!
Jiangli
>
> Thanks,
> Coleen
>
> > Thanks,
> > Jiangli
> >
> > >
> > > thanks,
> > > Coleen
> > >
> > > >
> > > > http://cr.openjdk.java.net/~jiangli/8186706/webrev.02/ \
> > > > <http://cr.openjdk.java.net/%7Ejiangli/8186706/webrev.02/>
> > > > Thanks,
> > > > Jiangli
> > > >
> > > > > Thanks,
> > > > > Coleen
> > > > >
> > > > >
> > > > > On 8/24/17 3:53 PM, Jiangli Zhou wrote:
> > > > > > Hi Ioi,
> > > > > >
> > > > > > ResourceObj only allows delete for C_HEAP objects, we need to allocate \
> > > > > > _archive_object_cache as C_HEAP object also. Otherwise, we would hit the \
> > > > > > following assert.
> > > > > > void ResourceObj::operator delete(void* p) {
> > > > > > assert(((ResourceObj *)p)->allocated_on_C_heap(),
> > > > > > "delete only allowed for C_HEAP objects");
> > > > > >
> > > > > > Here is the updated webrev that allocates/deallocates the \
> > > > > > _archive_object_cache table and nodes as C_HEAP objects. \
> > > > > > http://cr.openjdk.java.net/~jiangli/8186706/webrev.01/ \
> > > > > > <http://cr.openjdk.java.net/~jiangli/8186706/webrev.01/>
> > > > > > Thanks,
> > > > > > Jiangli
> > > > > >
> > > > > > > On Aug 24, 2017, at 10:22 AM, Ioi Lam <ioi.lam@oracle.com> \
> > > > > > > <mailto:ioi.lam@oracle.com> wrote:
> > > > > > > Hi Jiangli,
> > > > > > >
> > > > > > > The Nodes need to be deallocated in the ResourceHashtable destructor.
> > > > > > >
> > > > > > > ~ResourceHashtable() {
> > > > > > > if (ALLOC_TYPE == C_HEAP) {
> > > > > > > Node* const* bucket = _table;
> > > > > > > while (bucket < &_table[SIZE]) {
> > > > > > > Node* node = *bucket;
> > > > > > > while (node != NULL) {
> > > > > > > Node* cur = node;
> > > > > > > node = node->_next;
> > > > > > > delete cur;
> > > > > > > }
> > > > > > > ++bucket;
> > > > > > > }
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > > The problem with ResourceHashtable is that by default ALLOC_TYPE = \
> > > > > > > ResourceObj::RESOURCE_AREA, but if your call path looks like this:
> > > > > > >
> > > > > > > ResourceHashtable<...> table;
> > > > > > > ...
> > > > > > > {
> > > > > > > ResourceMark rm;
> > > > > > > ...
> > > > > > > {
> > > > > > > table.put(....);
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > The Node in the table will end up being invalid.
> > > > > > >
> > > > > > > In your case, the code path between the allocation of the \
> > > > > > > ResourceHashtable and the call to MetaspaceShared::archive_heap_object \
> > > > > > > covers a few files. There's currently no ResourceMark in between. \
> > > > > > > However, in the future, someone could potentially put in a ResourceMark \
> > > > > > > and cause erratic failures.
> > > > > > > So, since your're fixing the hashtable code, I think it will be a good \
> > > > > > > idea to change the ALLOC_TYPE = ResourceObj::C_HEAP. However, when \
> > > > > > > doing that, it's a good idea to do the proper clean up by invoking the \
> > > > > > > ~ResourceHashtable() destructor via the delete operator.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > - Ioi
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 8/23/17 10:27 PM, Jiangli Zhou wrote:
> > > > > > > > Hi Ioi,
> > > > > > > >
> > > > > > > > The table was not changed to be allocated as ResourceObj::C_HEAP. I \
> > > > > > > > see ‘ALLOC_TYPE' only applies to the Nodes in the \
> > > > > > > > ResourceHashtable.
> > > > > > > > Thanks,
> > > > > > > > Jiangli
> > > > > > > >
> > > > > > > > > On Aug 23, 2017, at 6:03 PM, Ioi Lam <ioi.lam@oracle.com> \
> > > > > > > > > <mailto:ioi.lam@oracle.com> wrote:
> > > > > > > > > Hi Jiangli,
> > > > > > > > >
> > > > > > > > > Since the table is allocated as ResourceObj::C_HEAP, it's better to \
> > > > > > > > > delete it afterwards to avoid memory leaks
> > > > > > > > > {
> > > > > > > > > NoSafepointVerifier nsv;
> > > > > > > > >
> > > > > > > > > // Cache for recording where the archived objects are copied to
> > > > > > > > > MetaspaceShared::create_archive_object_cache();
> > > > > > > > >
> > > > > > > > > tty->print_cr("Dumping String objects to closed archive heap region \
> > > > > > > > > ..."); NOT_PRODUCT(StringTable::verify());
> > > > > > > > > // The string space has maximum two regions. See \
> > > > > > > > > FileMapInfo::write_archive_heap_regions() for details. \
> > > > > > > > > _string_regions = new GrowableArray<MemRegion>(2); \
> > > > > > > > > StringTable::write_to_archive(_string_regions);
> > > > > > > > > tty->print_cr("Dumping objects to open archive heap region ...");
> > > > > > > > > _open_archive_heap_regions = new GrowableArray<MemRegion>(2);
> > > > > > > > > MetaspaceShared::dump_open_archive_heap_objects(_open_archive_heap_regions);
> > > > > > > > >
> > > > > > > > > + MetaspaceShared::create_archive_object_cache();
> > > > > > > > >
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > + static void delete_archive_object_cache() {
> > > > > > > > > + CDS_JAVA_HEAP_ONLY(delete _archive_object_cache; \
> > > > > > > > > _archive_object_cache = NULL;); + }
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > > - Ioi
> > > > > > > > >
> > > > > > > > > On 8/23/17 4:24 PM, Jiangli Zhou wrote:
> > > > > > > > > > Please review the following webrev that fixes the \
> > > > > > > > > > ArchivedObjectCache obj_hash() issue. The patch was from Ioi \
> > > > > > > > > > (thanks!). I will count myself as a reviewer.
> > > > > > > > > > bug: \
> > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8186706?filter=14921 \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > \
> > > > > > > > > > webrev: http://cr.openjdk.java.net/~jiangli/8186706/webrev.00/ \
> > > > > > > > > > <http://cr.openjdk.java.net/~jiangli/8186706/webrev.00/>
> > > > > > > > > > ArchivedObjectCache obj_hash() computes hash using incorrect \
> > > > > > > > > > address. The fix is to use the correct oop address. The default \
> > > > > > > > > > ResourceHashtable size is 256, which is too small when large \
> > > > > > > > > > number of objects are archived. The table is now changed to use a \
> > > > > > > > > > much larger (15889) size. The ArchivedObjectCache issue was \
> > > > > > > > > > noticed when one test times out on slower linux arm64 machine. \
> > > > > > > > > > With the fix the test finishes without timeout.
> > > > > > > > > > Tested with tier4-comp tests.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Jiangli
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic