RFR: 8131330: G1CollectedHeap::verify_dirty_young_list fails with assert

[Kim Barrett]

On Aug 20, 2015, at 11:20 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Thu, 2015-08-20 at 11:00 -0400, Kim Barrett wrote:
>> On Aug 19, 2015, at 4:48 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>> 
>>> New webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8131330/webrev.01/
>>> 
>>> Incremental webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8131330/webrev.01.inc1/
>> 
>> So why isn't the fill_subword helper function declared inline, you
>> might ask?
>> 
>> Because during development it made sense to have it not inlined, and I
>> forgot to reconsider that later.  Sigh!  I'll fix that and post a new
>> webrev this afternoon.
>> 
> 
> some tiny comments:
> 
> - blockOffsetTable.hpp, fill_range(): not sure why this code suddenly
> requires an assert that we are not currently running UseG1GC. The change
> may keep it, it's just confusing because if we are in that code when
> using G1, I doubt the code will get to this position.

We’re deciding whether to use the new memset_with_concurrent_readers,
depending on whether we’re running a concurrent collector.  We could

(1) Test for UseG1GC in the same manner as UseConcMarkSweepGC.
(2) Assert !UseG1GC, because G1 doesn’t use this code.
(3) Ignore UseG1GC and hope G1 continues to not use this code.

I chose (2).

> - This is just how I would comment assembly, but I would prefer if the
> "//" next to the code were aligned throughout a complete block.

I got tired of arguing with emacs auto-placement of comments and didn’t
want to mess with the default comment indent, which is how it came to where
it is.

> Otherwise looks good. Feel free to ignore my suggestions.

Thanks for looking at this.