RFR (M): 8157952: Parallelize Memory Pretouch

[sangheon]

Hi Thomas,

On 09/08/2016 04:47 AM, Thomas Schatzl wrote:
> Hi Jon,
>
>    a new webrev is available at http://cr.openjdk.java.net/~tschatzl/815
> 7952/webrev.1/ (full) and http://cr.openjdk.java.net/~tschatzl/8157952/
> webrev.0_to_1/ (diff).
Looks good in general.

http://cr.openjdk.java.net/~tschatzl/8157952/webrev.1/src/share/vm/runtime/globals.hpp.frames.html
Please set a range or constraint for the new flag.
Probably range(1, SIZE_MAX / ParallelGCThreads) ?
I recommend to run 
'runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java' for 
all platforms.

Very minor comments, so you can ignore below.
http://cr.openjdk.java.net/~tschatzl/8157952/webrev.1/src/share/vm/gc/g1/g1RegionToSpaceMapper.cpp.frames.html
line 70, 72
Can we set a new variable to avoid repeating '(size_t)start_idx * 
_pages_per_region'?

line 117
Probably 'size_t const NoPage'?

http://cr.openjdk.java.net/~tschatzl/8157952/webrev.1/src/share/vm/gc/g1/heapRegionManager.cpp.frames.html
http://cr.openjdk.java.net/~tschatzl/8157952/webrev.1/src/share/vm/gc/g1/heapRegionManager.hpp.frames.html
Updating copyright year.

Thanks,
Sangheon


> Testing:
> jprt, local testing
>
> On Wed, 2016-09-07 at 14:37 -0700, Jon Masamitsu wrote:
>> Thomas,
>> http://cr.openjdk.java.net/~tschatzl/8157952/webrev/src/share/vm/gc/g
>> 1/g1_globals.hpp.frames.html
>> Could we change G1PreTouchChunkSize to PreTouchChunkSize?  It
>> seems like something we would want to do for other GC's at some
>> point.
> I changed it to PreTouchParallelChunkSize for now.
>
>> http://cr.openjdk.java.net/~tschatzl/8157952/webrev/src/share/vm/gc/s
>> hared/workgroup.hpp.frames.html
>>
>>   64     _gc_id(Universe::is_fully_initialized() ? GCId::current_raw()
>> : 0)
>> What prompted the above change?
> The problem is that GCId::current_raw() is retrieved from the current
> NamedThread data structure. At initialization we are not running in a
> NamedThread. I found though that passing a GcId::undefined() here
> (using an additional constructor) allows logging at startup.
>
> This version is much nicer now. Thanks again for pushing me about that.
>
>> http://cr.openjdk.java.net/~tschatzl/8157952/webrev/src/share/vm/gc/g
>> 1/g1PageBasedVirtualSpace.cpp.frames.html
>>
>> I don't think there is anything G1 specific about class
>> G1PretouchTask.  Could it be renamed PretouchTask and put into its
>> own file for easier reuse?  If you think that is premature, ignore
>> this comment.
> As mentioned earlier, it takes much more work than that to be reusable,
> particular for parallel gc.
>
>> Pretouch() allows for the case where the work is done by the
>> current thread (only 1 chunk or null pretouch_gang).
>>    259 void G1PageBasedVirtualSpace::pretouch(size_t start_page,
>> size_t size_in_pages, WorkGang* pretouch_gang) {
>> Pretouching is so slow that I would not think it made a difference if
>> parallel worker were used instead of the current thread.  Simpler to
>> just always require a gang and use 1 worker in the degenerate case?
> Fixed.
>
> Thanks,
>    Thomas
>