RFR: Properly update roots in code cache

[Roman Kennke]

Am 29.06.2017 um 11:46 schrieb 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

Ok, this looks much better. I pushed exactly as you suggested.

Thanks,
Roman