RFR: 8186706: ArchivedObjectCache obj_hash() is broken
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Aug 25 18:53:40 UTC 2017
On 8/25/17 2:08 PM, Jiangli Zhou wrote:
>
>> On Aug 25, 2017, at 5:49 AM, coleen.phillimore at oracle.com
>> <mailto:coleen.phillimore at 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 at oracle.com
>>>> <mailto:coleen.phillimore at 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.
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/
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>>> On Aug 24, 2017, at 10:22 AM, Ioi Lam <ioi.lam at 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 at 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/
>>>>>>>>>
>>>>>>>>> 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
>
More information about the hotspot-runtime-dev
mailing list