RFR: 8130931: refactor CardTableModRefBS[ForCTRS]

[Kim Barrett]

On Jul 13, 2015, at 4:47 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> On Fri, 2015-07-10 at 20:23 -0400, Kim Barrett wrote:
>> Please review this refactoring of CardTableModRefBS and
>> CardTableModRefBSForCTRS.  The changes are in two stages, with
>> separate webrevs to make it easier to review.
>> 
>> The first stage eliminates some friend declarations from
>> CardTableModRefBS.
>> 
>> - BytecodeInterpreter doesn't need to be a friend.
>> - CheckForUnmarkedOops changed to use public byte_for_const().
> 
> Actually the whole member _unmarked_card can be removed.

Good point. Here’s the diff for that change.  I can produce a new webrev if needed.

diff -r 53eb5948d161 src/share/vm/gc/parallel/cardTableExtension.cpp
--- a/src/share/vm/gc/parallel/cardTableExtension.cpp	Mon Jul 20 02:03:42 2015 -0400
+++ b/src/share/vm/gc/parallel/cardTableExtension.cpp	Mon Jul 20 02:10:34 2015 -0400
@@ -40,7 +40,6 @@
   PSYoungGen*         _young_gen;
   CardTableExtension* _card_table;
   HeapWord*           _unmarked_addr;
-  const jbyte*        _unmarked_card;
 
  protected:
   template <class T> void do_oop_work(T* p) {
@@ -50,7 +49,6 @@
       // Don't overwrite the first missing card mark
       if (_unmarked_addr == NULL) {
         _unmarked_addr = (HeapWord*)p;
-        _unmarked_card = _card_table->byte_for_const(p);
       }
     }
   }

> 
>> - SharkBuilder changed to use public dirty_card_val().
>> - GuaranteeNotModClosure does not exist.
>> - CardTableRS changed to be a friend of CardTableModRefBSForCTRS. 
>>  Changed some references to static members of CardTableModRefBS
>>  to work around bugs in clang and Visual Studio [1].
>> 
>> Only the VMStructs friend declaration is retained.
>> 
>> The second stage moves some functionality that is specific to or only
>> used with the ForCTRS class from the base class to the ForCTRS
>> subclass. It also moves the ForCTRS class into its own files, separate
>> from the base class.
> 
> I would prefer if the parCardTableModRefBS.cpp file would be removed,
> its contents moved to the new CardTableModRefBSForCTRS.cpp as it only
> contains CardTableModRefBSForCTRS methods (and both seem only used for
> CMS).
> Iow, is there some reason to keep this file, or is this part of the
> "left for later" task?

Yes, this is part of the “left for later” task.  I don’t want to move those functions
from one file to another, only to later have them moved (more or less) back.

> 
>> We could further split below CardTableModRefBSForCTRS, with support
>> for parallel card scanning (used only by CMS) being in another class.
>> That split already partially exists in the source code structure, with
>> some relevant functions being defined in CMS-specific
>> parCardTableModRefBS.cpp. That restructuring is being left for later.
> 
> Looks good otherwise. Thanks for the cleanup.

Thanks for looking at it.