RFR (S): JDK-8076241: Remove unused methods mod_card_iterate() and non_clean_card_iterate_serial()

[Kim Barrett]

On Apr 2, 2015, at 10:44 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> 
> Hi Kim,
> 
> On 2015-04-01 01:59, Kim Barrett wrote:
>> On Mar 31, 2015, at 2:59 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>>> On 2015-03-31 00:16, Kim Barrett wrote:
>>>> ------------------------------------------------------------------------------
>>>> src/share/vm/memory/cardTableModRefBS.hpp
>>>>  181   // XXX ??? MemRegionClosure above vs OopsInGenClosure below XXX
>>>>  182   // XXX some new_dcto_cl's take OopClosure's, plus as above there are
>>>>  183   // some MemRegionClosures. Clean this up everywhere. XXX
>>>> 
>>>> This untouched comment is referring to the removed
>>>> non_clean_card_iterate_serial function ("above"), so needs to be
>>>> updated. It may be that the comment can be removed entirely now,
>>>> possibly being moved to the technical debt wiki.
>>> Ah. Good point. I removed the comment and added this to the technical debt wiki:
>>> 
>>> "CardTableModRefBS::dirty_card_iterate() works with a MemRegion and a MemRegionClosure. Other similar methods, such as CardTableModRefBS::non_clean_card_iterate_possibly_parallel() and CardTableModRefBS::process_stride() use OopsInGenClosure instead. Can we use a consistent API?”
>> That works for me.
>> 
>>>> ------------------------------------------------------------------------------
>>>> src/share/vm/memory/cardTableModRefBS.hpp
>>>>  382   // Invoke "cl.do_MemRegion" on a set of MemRegions that collectively
>>>>  383   // includes all the modified cards (expressing each card as a
>>>>  384   // MemRegion).  Thus, several modified cards may be lumped into one
>>>>  385   // region.  The regions are non-overlapping, and are visited in
>>>>  386   // *decreasing* address order.  (This order aids with imprecise card
>>>>  387   // marking, where a dirty card may cause scanning, and summarization
>>>>  388   // marking, of objects that extend onto subsequent cards.)
>>>> 
>>>> This comment was the reason I'd deferred removing mod_card_iterate. So
>>>> far as I can tell, this comment is the *only* place where there is any
>>>> discussion whatsoever of why some card iterators scan backward. I
>>>> wanted to find a new and better home for that information before
>>>> deleting the comment.
>>> I see. I couldn't find a card iterator that scans backwards now. Do you have a pointer to one that still scans backwards?
>>> 
>>> I'll push the change with the comment removed. I'm happy to find a new place for the comment if we have an iterators that scan backwards.
>> I think the only one left is ClearNoncleanCardWrapper::do_MemRegion().  But I don’t guarantee there aren’t more.
> 
> Thanks for pointing this out.
> 
> As I mentioned before I'm happy to push a fix to add the comment back. Do you think this would be appropriate?
> 
> diff --git a/src/share/vm/memory/cardTableRS.cpp b/src/share/vm/memory/cardTableRS.cpp
> --- a/src/share/vm/memory/cardTableRS.cpp
> +++ b/src/share/vm/memory/cardTableRS.cpp
> @@ -176,7 +176,10 @@
> bool ClearNoncleanCardWrapper::is_word_aligned(jbyte* entry) {
>   return (((intptr_t)entry) & (BytesPerWord-1)) == 0;
> }
> -
> +// The regions are visited in *decreasing* address order.
> +// This order aids with imprecise card marking, where a dirty
> +// card may cause scanning, and summarization marking, of objects
> +// that extend onto subsequent cards.
> void ClearNoncleanCardWrapper::do_MemRegion(MemRegion mr) {
>   assert(mr.word_size() > 0, "Error");
>   assert(_ct->is_aligned(mr.start()), "mr.start() should be card aligned");
> 
>> 
>> I was trying to build a list of all the card table iterators as part of working on
>> https://bugs.openjdk.java.net/browse/JDK-8060244
>> There were a surprising number, all different; and I’m not sure I found them all either.
> 
> Nice summary in the bug report! I like the idea of making the card iteration more efficient and trying to merge the implementations. Do you have plans to work on this?

I’ve been working on it.  I’ve temporarily put it aside to work on other things, but hope to get back to it soon.  I have code that I was pretty happy with when I put it aside, but since then have had some further ideas, plus ran across the SSE2-based scanner linked to in the bug report.  What I probably should do is mostly ignore the SSE2 approach for now, leaving that sort of thing for a later platform-specific optimization task.  But my recently revised thinking involves a fast find first bit / count trailing zeros operation, which I’d done some looking into as part of my first “starter bug” here (which turned out to be a science project that also got interrupted - sigh).