RFR (M) 8138762: Refactor setup of evacuation closures in G1

Erik Helin erik.helin at oracle.com
Tue Oct 6 13:36:54 UTC 2015


Hi Mikael,

first of all, thanks for a really nice patch! I think the patch
essentially is good to go, just a few comments inline.

On 2015-10-02, Mikael Gerdin wrote:
> I've split the change into two different webrevs.
> The first one moves a bunch of code out from g1CollectedHeap.cpp to become
> externally visible.
> I also noticed that g1CodeBlobClosure.hpp is missing include guards.
> To make room for the closure set in the par scan state I remove the unused
> g1 rem set pointer in this first webrev.
> 
> http://cr.openjdk.java.net/~mgerdin/8138762_1/webrev.0/

Looks good, Reviewed.

> The second webrev contains the actual change.
> I decided to separate the interface headers and the actual implementations
> of the closure sets in two separate files.
> g1RootClosureSet.hpp contains the interface and g1EvacuationClosures.hpp
> contains the implementations used by a G1 evacuation pause.
> 
> http://cr.openjdk.java.net/~mgerdin/8138762_2/webrev.0/

- g1CollectedHeap.cpp:
  The variable trace_metadata can be initialized with the
  expression:
  `bool trace_metadata =
    _g1h->collector_state()->during_initial_mark_pause() &&
    ClassUnloadingWithConcurrentMark;`

- g1RootProcessor.cpp:
  Could you update the assert comments for:
  +    assert(closures->second_pass_weak_clds() != NULL, "should be
  null");
  +    assert(closures->second_pass_weak_clds() == NULL, "should not
  null");

- g1RootClosureSet.hpp:
  Could you rename the interface (and the file) to g1RootClosures?
  Correspondingly, rename G1EvacuationRootClosureSet to
  G1RootEvacuationClosures?

- g1EvacuationClosures.hpp:
  Rename G1ClosureSet to G1SharedClosures?
  I'm not too fond of having comments before bool arguments,
  like /* process_only_dirty_klasses */. Personally I would remove them,
  but if you want to keep them, that is fine by me as well.

Thanks,
Erik

> The full webrev is available at:
> http://cr.openjdk.java.net/~mgerdin/8138762_full/webrev.0/
> 
> Testing:
> JPRT
> Performance (aurora.se G1 workloads on Solaris x64)
> Kitchensink,runThese,vm.gc.testlist,vm.g1classunload.testlist
> 
> I appreciate any feedback, if you have suggestions for different class names
> I'm happy to change them, I had some trouble coming up with good ones.
> 
> Thanks
> /Mikael



More information about the hotspot-gc-dev mailing list