RFR: 8179302: Pre-resolve constant pool string entries and cache resolved_reference arrays in CDS archive

[Jiangli Zhou]

Hi Thomas,

Thanks a lot for the review!

> On Aug 7, 2017, at 7:52 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Thu, 2017-08-03 at 17:15 -0700, Jiangli Zhou wrote:
>> Here are the updated webrevs.
>> 
>> http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.02/
>> http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.02/
>> 
>> Changes in the updated webrevs include:
>> Merge with Ioi’s recent shared space auto-sizing change (8072061)
>> Addressed all feedbacks from Ioi and Coleen (Thanks for detailed
>> review!)
> 
> - the comment in g1Allocator.hpp:326 needs to be updated. I would merge
> the information from the _open member here. I.e. define what "open" and
> "closed" archive are.

Good catch. I updated the comments as the following:

// G1ArchiveAllocator is used to allocate memory in archive
// regions. Such regions are not scavenged nor compacted by GC.
// There are two types of archive regions, which are
// differ in the kind of references allowed for the contained objects:
//
// - 'Closed' archive region contain no references outside of archive
//   regions. The region is immutable by GC. GC does not mark object
//   header in 'closed' archive region. 
// - An 'open' archive region may contain references pointing to
//   non-archive heap region. GC can adjust pointers and mark object
//   header in 'open' archive region.

> 
> - g1Allocator.inline.hpp:66-72: formatting - parameters should be
> aligned below each other

Fixed.

> 
> - g1CollectedHeap.cpp:665/6: formatting - parameters should be aligned
> below each other

Fixed.

> 
> - g1CollectedHeap.cpp:750 + 756: maybe make a static method to avoid
> repetition here.

I changed the code to be following. New static function is a little overkill since the usage is very limited. :-)

      HeapWord* top;
      HeapRegion* next_region;
      if (curr_region != last_region) {
        top = curr_region->end();
        next_region = _hrm.next_region_in_heap(curr_region);
      } else {
        top = last_address + 1;
        next_region = NULL;
      }
      curr_region->set_top(top);
      curr_region->set_first_dead(top);
      curr_region->set_end_of_live(top);
      curr_region = next_region;
    }
> 
> - G1NoteEndOfConcMarkClosure::doHeapRegion(): would it be too much work
> to make an extra CR out of this change? It is a change that fixes an
> existing bug unrelated to this change after all (not doing remembered
> set cleanup work for archive regions).

Using separate CR to track this sounds good. I just created JDK-8185924 <https://bugs.openjdk.java.net/browse/JDK-8185924>. Since we have been testing the fix with other changes together, I'll integrate them together with both CRs.

> 
> - g1HeapVerifier.cpp:63: I think this change is obsolete since
> is_obj_dead_cond() will always return false. (I.e. some leftover of
> some internal discussion)

You are right. I reverted the change.

> 
> - g1HeapVerifier.cpp: there is a verbose flag passed around. Not sure
> if it should be kept, as it seems to be some code that has been used
> for debugging this feature, but can't be activated anyway without code
> changes.

Removed.

> 
> - heapRegion.inline.hpp:120: please fix the comment of the assert.
> Something like "Closed archive regions should not have references into
> other regions”

Done.

> 
> - heapRegion.inline.hpp:125: I think the existing code of faking open
> archive regions as all-live does not work as implemented. Consider the
> case when a new object in there is made live, and references in there
> set to refer to some object outside this region, and is the only
> reference (and it has not been marked live yet): if there is a
> remembered set entry to that, and it is about to be scanned.
> 
> The current implementation of HeapRegion::is_obj_dead() will consider
> it dead, so we will enter the code path at line 125. Block_size_using
> bitmap() will jump over that object, but the return values of
> is_obj_dead_with_size() method will indicate the caller to not iterate
> over this object anyway, potentially missing that reference.
> 
> HeapRegion::is_obj_dead() needs to return that the object is not dead
> for open archive regions. I think for now the safest way is to add
> !is_open_archive() to the condition calculated there. That will obviate
> the need for that existing hack to the assert too.
> 
> It may have some perf impact though - actually recently there has been
> some effort to remove that is_archive() check from that code (the one
> that is now the is_closed_archive() assert). I do not see an easy way
> to fix this. :( (i.e. there is likely no perf impact vs. jdk9 so it's
> not that bad)
> 
> This suggestion also only works with the assumption laid out in the CR
> that there is no way that a live object can not become dormant again,
> and the objects in the open archive regions are always parsable (never
> contain junk data).

Thank you for the analysis. I changed HeapRegion::is_obj_dead() with added !is_open_archive() condition as you suggested. I’m glad to get rid of the is_open_archive() change from is_obj_dead_with_size() assert.

Thinking more about the case you described above, when an object (A) in the open archive just becomes live, there would be no reference to any other non-archive region at the moment. The object A only contains references to the archive (open or closed) regions initially. Scanning has no issue at the moment. When a new object (B) is allocated in the java heap and the reference is set in A. B is considered live, scanning would update the A->B reference accordingly. Is that correct?

> 
> - HeapRegionType::relabel_as_old(): the code in line 175/176 is
> unreachable and should be removed.

Agreed. Removed.

I’ll send out an updated webrev later. Thank you so much for all the help and contributions!!

Jiangli

> 
> I did not look through runtime code too closely.
> 
> Thanks,
>   Thomas
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170807/59a54c2c/attachment.htm>