RFR (S/M): 8184348: Merge G1ConcurrentMark::par_mark() and G1ConcurrentMark::grayRoot()

[Mikael Gerdin]

Hi Thomas,

On 2017-07-14 16:34, Thomas Schatzl wrote:
> Hi again,
> 
> On Fri, 2017-07-14 at 15:18 +0200, Aleksey Shipilev wrote:
>> On 07/14/2017 02:20 PM, Thomas Schatzl wrote:
>>>
>>> Not completely sure what you are referring to, but I split some
>>> very
>>> long asserts across lines.
>> Yes, I meant that, sorry for not being clear. Any webrev that
>> requires me to scroll horizontally on 2560-pixel wide screen triggers
>> me!
> 
> I noticed that too :)
> 
>>>>
>>>> *) So, mark_reference_grey used to be called from
>>>> G1CMSATBBufferClosure on
>>>> objects below TAMS, but now it would get called on objects past
>>>> TAMS
>>>> too?
>>> CMTask::make_reference_grey() now calls
>>> G1ConcurrentMark::mark_in_next_bitmap(), not
>>> ConcurrentMark::par_mark()
>>> which does not exist any more:
>>> G1ConcurrentMark::mark_in_next_bitmap()
>>> in the first check filters out marking attempts above nTAMS
>>> (g1ConcurrentMark.inline.hpp:47 now), returning false, which makes
>>> make_reference_grey() exit immediately in that case. This seems to
>>> achieve the same effect.
>> Ah, I missed that part! I agree this part is fine then.
>>
>>>
>>> If you are worried whether there is a performance difference
>>> because maybe now we do more work in some cases, all paths
>>> previously leading to the former G1ConcurrentMark::par_mark() did
>>> the nTAMS check in one way or another already (of course in
>>> inconsistent fashion) so there should be no change here.
>> No, I am not worried. SATB-heavy workloads have problems way beyond
>> bitmap marking :)
>>
>>>
>>> New webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/8184348/webrev.1/ (full)
>> Looks good to me.
> 
> Thanks. Unfortunately, after re-appyling and fixing other changes based
> on this one I noticed that I missed one opportunity to refactor in
> G1CMTask::deal_with_reference(). I would like to add this to this
> changeset still... sorry.
> 
> There is some note about some perf optimization that mentions that it
> is advantagous to do the nTAMS check before determining the heap
> region; however I do not think this is an issue.
> 
> Quickly comparing runs of a fairly large and reference-intensive
> workload (BigRAMTester with 20g heap e.g. attached to JDK-8152438),
> marking cycles with the latest webrev.2 are at least as fast as without
> any of this RFR's changes.
> 
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8184348/webrev.1_to_2 (diff)
> http://cr.openjdk.java.net/~tschatzl/8184348/webrev.2 (full)

Looks good to me.
/Mikael

> 
> Thanks,
>    Thomas
>