Shenandoah: Question about asserts

[Per Liden]

Ok, thanks Zhengyu! I'll update my PR to include the removal of the asserts.

cheers,
Per


On 9/9/21 4:22 PM, Zhengyu Gu wrote:
> Hi Per,
> 
> Shenandoah heap dump was designed to be single-threaded. It looks like 
> that JDK-8237354 breaks that.
> 
> Feel free to remove the assertions to unblock your work. I will file a 
> bug to fix Shenandoah heap dump.
> 
> Thanks,
> 
> -Zhengyu
> 
> On 9/9/21 5:48 AM, Per Liden wrote:
>> Hi Shenandoah-folks,
>>
>> When doing some random testing of 
>> https://github.com/openjdk/jdk/pull/5410 I noticed that Shenandoah ran 
>> into a few asserts during heap dumping. The asserts check that we're 
>> executing in the VM thread. However, I wonder if these asserts might 
>> be a bit too aggressive, or if there's a reason these functions must 
>> only be executed by the VM thread? I couldn't see any obvious reason, 
>> so I simply removed the offending asserts (see patch below) and 
>> nothing obvious seems to break (heap dumping tests now pass, etc).
>>
>> So, would you mind if I remove these asserts, or are they really 
>> needed for some reason I've missed?
>>
>> cheers,
>> Per
>>
>>
>> diff --git a/src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp 
>> b/src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp
>> index ca9afd7ad4f..623ca9574a2 100644
>> --- a/src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp
>> +++ b/src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp
>> @@ -355,7 +355,6 @@ 
>> ShenandoahCodeRootsIterator::ShenandoahCodeRootsIterator() :
>>           _par_iterator(CodeCache::heaps()),
>>           _table_snapshot(NULL) {
>>     assert(SafepointSynchronize::is_at_safepoint(), "Must be at 
>> safepoint");
>> -  assert(!Thread::current()->is_Worker_thread(), "Should not be 
>> acquired by workers");
>>     CodeCache_lock->lock_without_safepoint_check();
>>     _table_snapshot = 
>> ShenandoahCodeRoots::table()->snapshot_for_iteration();
>>   }
>> diff --git 
>> a/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp 
>> b/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp
>> index c0469a71d7b..261920a9d7a 100644
>> --- a/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp
>> +++ b/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp
>> @@ -258,7 +258,6 @@ 
>> ShenandoahHeapIterationRootScanner::ShenandoahHeapIterationRootScanner() 
>> :
>>    }
>>
>>    void ShenandoahHeapIterationRootScanner::roots_do(OopClosure* oops) {
>> -   assert(Thread::current()->is_VM_thread(), "Only by VM thread");
>>      // Must use _claim_none to avoid interfering with concurrent CLDG 
>> iteration
>>      CLDToOopClosure clds(oops, ClassLoaderData::_claim_none);
>>      MarkingCodeBlobClosure code(oops, 
>> !CodeBlobToOopClosure::FixRelocations);
>> diff --git 
>> a/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp 
>> b/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp
>> index 3f614d1e208..b2897d380aa 100644
>> --- a/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp
>> +++ b/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp
>> @@ -86,9 +86,8 @@ ShenandoahClassLoaderDataRoots<CONCURRENT, 
>> SINGLE_THREADED>::ShenandoahClassLoad
>>       ClassLoaderDataGraph_lock->lock();
>>     }
>>
>> -  // Non-concurrent mode only runs at safepoints by VM thread
>> +  // Non-concurrent mode only runs at safepoints
>>     assert(CONCURRENT || SafepointSynchronize::is_at_safepoint(), 
>> "Must be at a safepoint");
>> -  assert(CONCURRENT || Thread::current()->is_VM_thread(), "Can only 
>> be done by VM thread");
>>   }
>>
>>   template <bool CONCURRENT, bool SINGLE_THREADED>
>>
>