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

Thomas Schatzl thomas.schatzl at oracle.com
Wed Aug 9 11:59:07 UTC 2017


Hi Jiangli,

  going to look at the new webrev in the other email again, answering
some questions here.

Thanks for considering all these suggestions.

On Mon, 2017-08-07 at 16:39 -0700, Jiangli Zhou wrote:
> Hi Thomas,
> 
> Thanks a lot for the review!
> 
> > On Aug 7, 2017, at 7:52 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> 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

better: other "closed" 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.
> 
[...]
> > - 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. :-)

Okay :)
> > - 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. Since we have been testing the fix with other changes
> together, I'll integrate them together with both CRs.

Since you assigned this to me, do you want me to post the RFR?

> > 
> > - 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.

Thanks.

> > - 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?

  that is exactly the case this suggested change covers. Otherwise the
not-yet-marked object would be skipped, and the reference not updated.

As mentioned I will look again at the new webrev.

Thomas



More information about the hotspot-gc-dev mailing list