RFR [S][5/7]: 8197573: Remove concurrent cleanup and secondary free list handling

Thomas Schatzl thomas.schatzl at oracle.com
Tue Mar 27 08:58:19 UTC 2018


Hi Sangheon,

  thanks for your review.

On Mon, 2018-03-26 at 15:38 -0700, sangheon.kim wrote:
> Hi Thomas,
[..]
> > 
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1_to_2/ (diff)
> > http://cr.openjdk.java.net/~tschatzl/8197573/webrev.2/ (full)
> 
> webrev.2 looks good. I only have minor nits.
> 
> ===============================
> src/hotspot/share/gc/g1/concurrentMarkThread.cpp
>   362       if (!cm()->has_aborted()) {
> ...
>   364       }
>   365
>   366       if (!cm()->has_aborted()) {
> - Any reason for having 2 if clause at line:362 and 366?
> 

The delay might take a long while, so the abort state might have
changed already. So it does not hurt checking to avoid going into the
VM operation.

> ===============================
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp (from original file)
> 1030   // following two methods.
> - Do you know which method is referring originally? I guess 
> wait_while_free_regions_coming() and _cm->root_regions()->abort()?
> And this is why you didn't remove the comment above 
> wait_while_free_regions_coming(), right?

As far as I understand the two methods were
wait_while_free_regions_coming() and wait_until_scan_finished(). The
latter is still there so I only adapted the comment.

I removed the comment in the new webrev and filed JDK-8200291. We
should not have todo-lists in the code.

> 
> ===============================
> src/hotspot/share/gc/g1/heapRegionSet.cpp
> - Copyright year update
> 

Fixed. New webrevs (just the comment removals and the copyright
update):

http://cr.openjdk.java.net/~tschatzl/8197573/webrev.2_to_3/ (diff)
http://cr.openjdk.java.net/~tschatzl/8197573/webrev.3/ (full)

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list