RFR: Properly update roots in code cache

[Aleksey Shipilev]

On 06/29/2017 11:35 AM, Roman Kennke wrote:
> Am 29.06.2017 um 08:11 schrieb Aleksey Shipilev:
>> On 06/28/2017 11:01 PM, Roman Kennke wrote:
>>> With concurrent code cache evacuation, we now have to make sure we
>>> properly update refs in the code cache at the start of full-gc. The
>>> SCM::update_roots() only asserts that code cache roots point to
>>> to-space, which is not correct under conc codecache evac. This patch
>>> changes the code so that it properly updates code cache roots in full GC:
>>>
>>> http://cr.openjdk.java.net/~rkennke/fixfullupdatecodecache/webrev.00/
>>> <http://cr.openjdk.java.net/%7Erkennke/fixfullupdatecodecache/webrev.00/>
>> Ah. Makes sense.
>>
>> But the patch looks fragile for two reasons:
>>
>>  a) The change is within the ASSERT block, meaning release is still broken
>>  b) It relies on cancelled_concgc to detect we are in full GC path, which might
>> get accidentally reset when we are in full GC already
>>
>> I would rather split ShenandoahConcurrentMark::update_roots to normal and Full
>> GC versions, and act accordingly. Or maybe keep it single, but accept the
>> boolean flag, e.g.:
>>
>> void ShenandoahConcurrentMark::update_roots(bool full_gc) {
>>   ShenandoahGCPhase phase(full_gc ?
>>                           ShenandoahCollectorPolicy::full_gc_roots :
>>                           ShenandoahCollectorPolicy::final_update_refs_roots);
>>   ...
>>   if (full_gc) {
>>     ...do unconditional root update...
>>   } else {
>>     ...do what we have now...
>>   }
>> }
> Like this:
> http://cr.openjdk.java.net/~rkennke/fixfullupdatecodecache/webrev.01/

That ASSERT block gets confusing. Suggestion:

    CodeBlobClosure* code_blobs;
    CodeBlobToOopClosure update_blobs(...);
#ifdef ASSERT
    AssertToSpaceClosure assert_to_space_oops;
    CodeBlobToOopClosure assert_to_space(...);
#endif
    if (_full_gc) {
      code_blobs = &update_blobs;
    } else {
      code_blobs = DEBUG_ONLY(&assert_to_space) NOT_DEBUG(NULL);
    }

Thanks,
-Aleksey