CRR: 7046558 G1: concurrent marking optimizations (S)

[Tony Printezis]

Hi all,

While testing these changes last night I hit a rare failure (the 
_heap_end field would sometimes become inconsistent with what's in 
G1CollectedHeap and is_in_g1_heap() would return the wrong result; 
thankfully the assert I added caught that). So, I'm going to back out of 
the introduction of the is_in_g1_heap() method and revert back to using 
G1CH::is_in_g1_reserved(). We can look at maybe re-introducing it in the 
future.

Here are the latest (and hopefully final!) webrevs:

Change 1: 
http://cr.openjdk.java.net/~tonyp/7046558/webrev.3/webrev.0.G1CMOptSpecial/
Change 2: 
http://cr.openjdk.java.net/~tonyp/7046558/webrev.3/webrev.1.G1CMOptInline/
Change 3: 
http://cr.openjdk.java.net/~tonyp/7046558/webrev.3/webrev.2.G1CMOptMisc/
Complete: http://cr.openjdk.java.net/~tonyp/7046558/webrev.3/webrev.all/

Only Change 3 is affected by this. Testing ran successfully for 10+ 
hours. Let me know if you're planning to look at it, otherwise I'm going 
to push it at the first available opportunity.

Tony

On 06/07/2011 04:35 PM, Tony Printezis wrote:
> Hi all,
>
> All set thanks to Stefan and a second code review from John 
> Cuthbertson. In case anyone wants to have a look, here's the updated 
> webrev(s):
>
> Complete:
> http://cr.openjdk.java.net/~tonyp/7046558/webrev.2/webrev.all/
>
> Change 1 (closure specialization):
> http://cr.openjdk.java.net/~tonyp/7046558/webrev.2/webrev.0.G1CMOptSpecial/ 
>
>
> Change 2 (method inlining):
> http://cr.openjdk.java.net/~tonyp/7046558/webrev.2/webrev.1.G1CMOptInline/ 
>
>
> Change 3 (misc optimizations):
> http://cr.openjdk.java.net/~tonyp/7046558/webrev.2/webrev.2.G1CMOptMisc/
>
> The only one that's changed compared to the previous version is the 
> last change. I introduced and use the is_in_g1_heap() method in 
> ConcurrentMark to avoid calling into the G1 heap. I also added a 
> comment near the start of the claim_region() method which addresses an 
> issue that John Cuthbertson brought up.
>
> Tony
>
> On 06/02/2011 04:25 AM, Tony Printezis wrote:
>> Stefan,
>>
>> Thanks for the code review! Good catch about the circular dependency. 
>> I re-organized things a bit: I moved scan_object() from the 
>> .inline.hpp to the .cpp file (it's only called from there so it 
>> should be inlined anyway) so now I don't need to include 
>> g1OopClosures.inline.hpp in concurrentMark.inline.hpp.
>>
>> Here's the updated webrev:
>>
>> All included:
>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.1/webrev.all/
>>
>> Change 1:
>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.1/webrev.G1CMOptSpecial/ 
>>
>>
>> Change 2:
>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.1/webrev.G1CMOptInline/
>>
>> Change 3:
>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.1/webrev.G1CMOptMisc/
>>
>> Tony
>>
>> On 6/1/2011 9:28 AM, Stefan Karlsson wrote:
>>> Note that you have introduced a circular dependency between the 
>>> files: concurrentMark.inline.hpp and g1OopClosures.inline.hpp.
>>>
>>> Otherwise, this looks good.
>>>
>>> StefanK
>>>
>>> On 06/01/2011 02:19 PM, Tony Printezis wrote:
>>>> Hi,
>>>>
>>>> Could I please have a couple of code reviews for the following change:
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.0/webrev.all/
>>>>
>>>> It includes a few optimizations in the concurrent marking code. The 
>>>> fact that webrev shows that almost 400 lines of code have been 
>>>> changed is actually a bit misleading given that the changes are 
>>>> actually relatively simple. Here's a breakdown to make your life a 
>>>> bit easier:
>>>>
>>>> Change 1: Specialize the G1CMOopClosure to avoid the virtual call 
>>>> to do_oop() every time we visit an oop during marking (the closure 
>>>> used to be called CMOopClosure, I prefixed its name with G1 since 
>>>> it's been moved out of the concurrentMark.cpp file). Note: I didn't 
>>>> change much code here, just moved things around.
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.0/webrev.G1CMOptSpecial/ 
>>>>
>>>>
>>>> Change 2 (builds on Change 1): Move methods 
>>>> CM::deal_with_reference() and CM::push() to the .inline.hpp file, 
>>>> without changing them, to make sure they get inlined, as they are 
>>>> called from the fast path. You can either check line by line that 
>>>> they are unchanged. Or you can take my word for it. :-)
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.0/webrev.G1CMOptInline/ 
>>>>
>>>>
>>>> Change 3 (builds on Change 2): A few minor performance improvements 
>>>> here and there (plus a couple of cosmetic changes to introduce some 
>>>> missing curly brackets).
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7046558/webrev.0/webrev.G1CMOptMisc/
>>>>
>>>> I kept track of the separate patches to be able to evaluate them 
>>>> separately so I thought I'd present the changes separately to make 
>>>> the code review process a bit easier. I will of course push them as 
>>>> a single changeset.
>>>>
>>>> I was not quite sure how to evaluate the performance impact of the 
>>>> changes given that marking runs concurrently and not always in a 
>>>> deterministic fashion (from a performance point-of-view!!!). So I 
>>>> did the evaluation using collect and analyze. When looking at 
>>>> time-based profiles, I see that the above changes decrease the time 
>>>> we spend in the marking code (i.e., how long we spend in the 
>>>> CMConcurrentMarkingTask::work(int i) method) by between 7%-10%. The 
>>>> improvement is similar when looking at cycle counts. And I also see 
>>>> a nice decrease in L2 cache misses. Each separate Change does 
>>>> improve on the previous one, but the Change with the biggest impact 
>>>> was 1.
>>>>
>>>> Tony
>>>>
>>>