RFR(s): 8029255: G1: Reference processing should not enqueue references on the shared SATB queue

[Thomas Schatzl]

Hi,

  some comments:

On Thu, 2013-11-28 at 17:50 +0100, Per Liden wrote:
> Summary: A side effect of fixing JDK-8029162 (G1: Shared SATB queue 
> never enabled) is that the reference processor will start to add 
> references to the shared SATB queue when enqueuing discovered 
> references. This will later cause ConcurrentMark::verify_no_cset_oops() 
> to fail because we now have enqueued references in the SATB qeueue which 
> points into the collection set (which will be empty after the 
> collection). This patch makes sure we avoid doing pre-barriers, by 
> instead doing raw stores followed by a post-barrier.
>
> This patch also removes an unused constructor and the unnecessary 
> caching of the barrier set in the ReferenceProcessor. So use of _bs was 
> replaced by oopDesc::bs(). Further, the cached barrier set was only 

Not using the cached values may cause performance problems if multiple
threads access it and there is a frequently modified global in the same
cache line.
I do not think/hope it is a problem here though.

> set/used if discovered_list_needs_barrier was true, but with this change 
> we will need the barrier set in other cases too.
> 
> Testing done: jprt, kitchensink (10 hours), gcbasher (10 hours)
> 
> http://cr.openjdk.java.net/~pliden/8029255/webrev.0/
> 
> https://bugs.openjdk.java.net/browse/JDK-8029255
> 

I think I understand the reason for the change, but I do not think the
change is good.

Background (for others too, it took me a while to understand the
reference processing code):
The problem why the code cannot use the normal barrier code in all these
cases is that collection uses the next and discovered fields of
java.lang.Reference for its own purpose, putting references from the
cset into the queues.

In this process we loose the previous values too, that's why the code
cannot just apply the normal barrier - before activating the shared
reference queue in 8029162 it was not a problem because the "old"
references were dropped immediately when trying to add.

It seems that this is not an issue in regards to SATB as these fields
are handled somewhat specially always anyway.

I would prefer if setting the "next" field of the reference were
encapsulated into a method like ReferenceProcessor::set_discovered (e.g.
ReferenceProcessor::set_next()), and used instead of inlining
set_raw/apply-post-barrier code all the time.

There is already a missing call to the post-barrier in
referenceProcessor.cpp:366  when storing the self-loop value (That is
benign since it does not add a new object to the object graph, but if
that is kept, the reason should be documented).

Other issues:
- (this is more a question than an issue)
DiscoveredListIterator::make_active() has some special handling for
setting the next field of java.lang.Reference. In particular it does not
call the post-barrier because this will fail CT verification in G1.
What's the difference between that field and the discovered field in
that respect? Also the original code executed the post-barrier on the
next field (e.g. referenceProcessor.cpp:366), so if there were a
problem, it should already occur, shouldn't it?

- there are places where the code for
ReferenceProcessor::set_discovered() is inlined for no apparent reason
(e.g. at least referenceProcessor.cpp:374, and
referenceProcessor.cpp:1256). Could this change (or the one proposed
below) also clean this up?

- with the change in 8029162 I do not think the code that is enabled by
setting _pending_list_uses_discovered_field to false works any more (if
it ever worked with G1, see the very first thought in this list). It
will trigger the same assertion that this fix wants to fix.
If the "old behavior" is not needed any more (I think so), please remove
that code first in another CR. The alternative is to fix it I guess.

- I still think that 8029162 and this are one and the same issue. :)
Given that 8029162 in isolation throws an assertion in debug code
(potential CR), it should be pushed in the same changeset as this one.

Thanks,
Thomas