RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Fri Dec 21 02:57:12 UTC 2018
Hi Kishor,
> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.14/
> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.13_to_14/
Webrev.14 still looks good.
Thank you for addressing all my requests!
Thanks,
Sangheon
On 12/20/18 6:50 PM, Kharbas, Kishor wrote:
> Some explanation for nv-dimm file handling in latest webrev,
>
> In earlier webrevs, the file opened in nv-dimm for mapping part of reserved heap space was not closed explicitly. It did not cause my functional issue, as desired lifetime of the file is same as lifetime of the VM.
> Added code to close the file in latest webrev as a good coding practice. Closing the file descriptor is ok, since the memory mapped reserved space prevents the file to deleted from the filesystem.
>
> Thanks,
> Kishor
>
>> -----Original Message-----
>> From: Kharbas, Kishor
>> Sent: Thursday, December 20, 2018 5:19 PM
>> To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>; Stefan
>> Johansson <stefan.johansson at oracle.com>; 'hotspot-gc-
>> dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev
>> runtime' <hotspot-runtime-dev at openjdk.java.net>
>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Kharbas,
>> Kishor <kishor.kharbas at intel.com>
>> Subject: RE: RFR(M): 8211424: Allocation of old generation of java heap on
>> alternate memory devices - ParallelOld
>>
>> Hi Sangheon,
>> Thanks for the review.
>> I have a minor update to properly close temporary nv-dimm file.
>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.14/
>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.13_to_14/
>>
>> Thanks
>> Kishor
>>> -----Original Message-----
>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>> Sent: Thursday, December 20, 2018 1:25 PM
>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
>>> <stefan.johansson at oracle.com>; 'hotspot-gc-dev at openjdk.java.net'
>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev runtime' <hotspot-
>>> runtime-dev at openjdk.java.net>
>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of java
>>> heap on alternate memory devices - ParallelOld
>>>
>>> Hi Kishor,
>>>
>>> On 12/20/18 12:40 AM, Kharbas, Kishor wrote:
>>>> Thanks Stefan,
>>>>
>>>> I have incorporated the comment in the new webrev at :
>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.13
>>>>
>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.12_to_13
>>> Webrev.13 looks good.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>> Regards,
>>>> Kishor
>>>>
>>>>> -----Original Message-----
>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>>> Sent: Monday, December 17, 2018 3:07 AM
>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>> sangheon.kim at oracle.com; 'hotspot-gc-dev at openjdk.java.net'
>>>>> <hotspot-gc- dev at openjdk.java.net>; 'Hotspot dev runtime'
>>>>> <hotspot-runtime- dev at openjdk.java.net>
>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of java
>>>>> heap on alternate memory devices - ParallelOld
>>>>>
>>>>> Hi Kishor,
>>>>>
>>>>> Same problem here as for G1, the current patch doesn't build.
>>>>> Please add this patch to your changeset:
>>>>> http://cr.openjdk.java.net/~sjohanss/8211425/Final-Comment-Par/
>>>>>
>>>>> Cheers,
>>>>> Stefan
>>>>>
>>>>> On 2018-12-15 03:02, Kharbas, Kishor wrote:
>>>>>> Thank you for the review.
>>>>>>
>>>>>> I have new webrev with suggested changes at :
>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.12/
>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.11_to_12
>>>>>> Thanks
>>>>>> Kishor
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>>>>> Sent: Friday, December 14, 2018 9:57 AM
>>>>>>> To: Stefan Johansson <stefan.johansson at oracle.com>; Kharbas,
>>>>>>> Kishor <kishor.kharbas at intel.com>; 'hotspot-gc-
>> dev at openjdk.java.net'
>>>>>>> <hotspot- gc-dev at openjdk.java.net>; 'Hotspot dev runtime'
>>>>>>> <hotspot-runtime- dev at openjdk.java.net>
>>>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of
>>>>>>> java heap on alternate memory devices - ParallelOld
>>>>>>>
>>>>>>> Hi Kishor,
>>>>>>>
>>>>>>> On 12/14/18 2:30 AM, Stefan Johansson wrote:
>>>>>>>> Hi Kishor,
>>>>>>>>
>>>>>>>> Thanks for addressing my comments, but I have one more :)
>>>>>>>>
>>>>>>>> On 2018-12-14 08:59, Kharbas, Kishor wrote:
>>>>>>>>> New webrev with small incremental change to make young sizing
>>>>>>>>> logs similar to G1.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.11
>>>>>>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
>>>>>>>> --------------------------------------------------------------
>>>>>>>> 37 char calc_str[100];
>>>>>>>> 38 calc_str[0] = 0;
>>>>>>>>
>>>>>>>> Instead of using a raw char buffer we have the FormatBuffer
>>>>>>>> class that can be used. If you look in
>>>>>>>> G1CollectedHeap::do_collection_pause_at_safepoint there is an
>>>>>>>> example on how you can use it. I think such change would both
>>>>>>>> help readability and robustness.
>>>>>>> I like this suggestion.
>>>>>>>
>>>>>>> In addition to this:
>>>>>>> -------------------------
>>>>>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
>>>>>>> 36 // Check the available dram memory to limit NewSize and
>>>>> MaxNewSize.
>>>>>>> And then
>>>>>>> 37 void HeterogeneousGenerationSizer::initialize_flags() {
>>>>>>> - Incomplete comment at line 36?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Stefan
>>>>>>>>
>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.10_to_1
>>>>>>>>> 1
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Kishor
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Kharbas, Kishor
>>>>>>>>>> Sent: Thursday, December 13, 2018 1:01 PM
>>>>>>>>>> To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>;
>>> Stefan
>>>>>>>>>> Johansson <stefan.johansson at oracle.com>; 'hotspot-gc-
>>>>>>>>>> dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>;
>>>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>
>>>>>>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>;
>>>>> Kharbas,
>>>>>>>>>> Kishor <kishor.kharbas at intel.com>
>>>>>>>>>> Subject: RE: RFR(M): 8211424: Allocation of old generation of
>>>>>>>>>> java heap on alternate memory devices - ParallelOld
>>>>>>>>>>
>>>>>>>>>> Hi Sagheon,
>>>>>>>>>>
>>>>>>>>>> I have a new webrev at -
>>>>>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.10/
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.09_to_1
>>>>>>> 0
>>>>>>>>>> /
>>>>>>>>>>
>>>>>>>>>> Comments inline.
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>> Sent: Thursday, December 13, 2018 11:04 AM
>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>>>>>>>>>> Johansson <stefan.johansson at oracle.com>; 'hotspot-gc-
>>> dev at openjdk.java.net'
>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev runtime'
>>>>>>>>>>> <hotspot- runtime-dev at openjdk.java.net>
>>>>>>>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of
>>>>>>>>>>> java heap on alternate memory devices - ParallelOld
>>>>>>>>>>>
>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>
>>>>>>>>>>> On 12/12/18 7:59 PM, Kharbas, Kishor wrote:
>>>>>>>>>>>> Hi Sangheon, Stefan,
>>>>>>>>>>>>
>>>>>>>>>>>> Here is an updated webrev -
>>>>>>>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.07_to_0
>>>>>>> 8
>>>>>>>>>>>> /
>>>>>>>>>>>>
>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.08/
>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>
>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
>>>>>>>>>>> 36 julong young_size_limit = os::physical_memory();
>>>>>>>>>>> - This variable is not used.
>>>>>>>>>> [Kharbas, Kishor] Fixed.
>>>>>>>>>>> 58 log_info(gc, ergo)("Setting MaxNewSize to "
>>> SIZE_FORMAT "
>>>>>>>>>>> which is 80% of dram memory. Dram usage can be lowered by
>>>>>>>>>>> setting MaxNewSize to a lower value",
>>>>>>>>>>> (size_t)reasonable_max);
>>>>>>>>>>> - Later I saw you fixed this compile error on webrev.09, good.
>>>>>>>>>>>
>>>>>>>>>> [Kharbas, Kishor] Fixed. And I changed log message according
>>>>>>>>>> to Stefan's comment.
>>>>>>>>>>
>>>>>>>>>>> Again, hope to have more manual testing with young gen size
>>>>>>>>>>> related options added.
>>>>>>>>>>>
>>>>>>>>>> [Kharbas, Kishor] I tested different cases with different
>>>>>>>>>> combinations of values for MaxNewSize, NewSize, MaxRAM,
>>>>>>>>>> MaxRAMPercentage/Fraction, etc.
>>>>>>>>>>
>>>>>>>>>> Thank you
>>>>>>>>>> Kishor
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Sangheon
>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Kishor
>>>>>>>>>>>>
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>>>> Sent: Wednesday, December 12, 2018 4:34 PM
>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>>>>>>>>>>>> Johansson <stefan.johansson at oracle.com>; 'hotspot-gc-
>>>>> dev at openjdk.java.net'
>>>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev runtime'
>>>>>>>>>>>>> <hotspot- runtime-dev at openjdk.java.net>
>>>>>>>>>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation
>>>>>>>>>>>>> of java heap on alternate memory devices - ParallelOld
>>>>>>>>>>>>>
>>>>>>>>>>>>> One more!
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>>>
>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
>>>>>>>>>>>>> 56 log_warning(gc, ergo)("Setting MaxNewSize to "
>>>>>>>>>>>>> SIZE_FORMAT "
>>>>>>>>>>>>> based on dram memory available", reasonable_max);
>>>>>>>>>>>>> 62 log_warning(gc, ergo)("Setting NewSize to "
>>>>>>>>>>>>> SIZE_FORMAT "
>>>>>>>>>>>>> based on dram memory available", reasonable_max);
>>>>>>>>>>>>> - You need to type-cast 'reasonable_max' to 'size_t' for
>>>>>>>>>>>>> both
>>> lines.
>>>>>>>>>>>>> Fails compiling on Mac.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Sangheon
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 12/12/18 3:12 PM, sangheon.kim at oracle.com wrote:
>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 12/12/18 12:51 PM, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>> New webrev and comments inline.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.07/
>>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.06_to_0
>>>>>>>>>>> 7/
>>>>>>>>>>>>>> I have some comments.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/generationSizer.hpp
>>>>>>>>>>>>>> 46 bool is_hetero_heap() const;
>>>>>>>>>>>>>> - 'virtual'?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/parallelArguments.cpp
>>>>>>>>>>>>>> 100 return
>>>>>>>>>>>>>> create_heap_with_policy<ParallelScavengeHeap,
>>>>>>>>>>>>>> GenerationSizer>();
>>>>>>>>>>>>>> - Missing 2 spaces at line 100.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>>>>
>>>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
>>>>>>>>>>>>>> 2 * Copyright (c) 2018, Oracle and/or its affiliates.
>>>>>>>>>>>>>> All rights reserved.
>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>> 23 */
>>>>>>>>>>>>>> - Missing 1 space for line 2 ~ 23.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 39 julong phys_mem = FLAG_IS_DEFAULT(MaxRAM) ?
>>>>>>>>>>>>>> os::physical_memory() : (julong)MaxRAM;
>>>>>>>>>>>>>> - I saw your reasoning, but why would you like to allow
>>>>>>>>>>>>>> MaxRAM via command-line? Accidentally it can be larger
>>>>>>>>>>>>>> than physical
>>>>> RAM.
>>>>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>>>>
>>>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.hpp
>>>>>>>>>>>>>> - Missing 1 space for line 2 ~ 23.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Sangheon
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>> From: Stefan Johansson
>>>>>>>>>>>>>>>> [mailto:stefan.johansson at oracle.com]
>>>>>>>>>>>>>>>> Sent: Wednesday, December 12, 2018 1:38 AM
>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>>>>>>>>>>>>> sangheon.kim at oracle.com; 'hotspot-gc-
>>>>> dev at openjdk.java.net'
>>>>>>>>>>>>>>>> <hotspot-gc- dev at openjdk.java.net>; 'Hotspot dev
>> runtime'
>>>>>>>>>>>>>>>> <hotspot-runtime- dev at openjdk.java.net>
>>>>>>>>>>>>>>>> Cc: Viswanathan, Sandhya
>>> <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old
>>>>>>>>>>>>>>>> generation of java heap on alternate memory devices -
>>>>>>>>>>>>>>>> ParallelOld
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 2018-12-12 09:16, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>>>> Hi Sangheon, Stefan,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thank you for your reviews.
>>>>>>>>>>>>>>>>> I have an updated webrev (against g1 patch) at :
>>>>>>>>>>>>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.06/
>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/generationSizer.cpp
>>>>>>>>>>>>>>>> -------------------------------------------------
>>>>>>>>>>>>>>>> Instead of adding set_ergo_flags_for_hetero_heap() to
>>>>>>>>>>>>>>>> the generic GenerationSizer I think instead having a
>>>>>>>>>>>>>>>> HeterogeneousGenerationSizer which does this at the
>>>>>>>>>>>>>>>> correct point during initialization is a better approach.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Also I don't think you should use the whole physical ram
>>>>>>>>>>>>>>>> as limit, the JVM will use memory and you probably don't
>>>>>>>>>>>>>>>> want to try to expand the young gen all the way to the
>>>>>>>>>>>>>>>> max. The normal heap sizing code used when nothing is
>>>>>>>>>>>>>>>> set uses MaxRamPercentage to limit
>>>>>>>>>>> the heap.
>>>>>>>>>>>>>>>> There is also the flag MaxRam which can be used to limit
>>>>>>>>>>>>>>>> the amount of DRAM used by the heap so I think limiting
>>>>>>> MaxNewSize
>>>>>>>>>>>>>>>> to MIX(MaxRam, physical mem) * MaxRamPercentage /
>>>>>>>>>>>>>>>> 100 would be a good idea. Something similar to what is
>>>>>>>>>>>>>>>> done in Arguments::set_heap_size().
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [Kharbas, Kishor] I added HeterogeneousGenerationSizer
>>>>>>>>>>>>>>> which would limit NewSize and MaxNewSize. My reasoning
>> is
>>>>>>>>>>>>>>> as follows,
>>>>>>>>>>>>>>> - If MaxRAM is set on command line, use that as
>>>>>>>>>>>>>>> available dram memory
>>>>>>>>>>>>>>> - If MaxRAMPercentage or MaxRamFraction is set on
>>>>>>>>>>>>>>> cmd line, we will limit young size accordingly.
>>>>>>>>>>>>>>> - Otherwise, I use my own ' MaxRamFractionForYoung'
>>>>>>>>>>>>>>> to limit young size. Reason is - since MaxRAMPercentage
>>>>>>>>>>>>>>> is very low (20%) since its meant to size heap when
>>>>>>>>>>>>>>> MaxHeap is not
>>>>>>> specified.
>>>>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>>>>>>>>>>> ---------------------------------------
>>>>>>>>>>>>>>>> 2960 // When AllocateOldGenAt is set, we cannot use
>>>>>>>>>>>>>>>> largepages for entire heap memory.
>>>>>>>>>>>>>>>> 2961 // Only young gen which is allocated in dram can
>>>>>>>>>>>>>>>> use large pages, but we currently don't support that.
>>>>>>>>>>>>>>>> 2962 if (!FLAG_IS_DEFAULT(AllocateOldGenAt)) {
>>>>>>>>>>>>>>>> 2963 FLAG_SET_DEFAULT(UseLargePages, false);
>>>>>>>>>>>>>>>> 2964 }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> From what I can see you have not addressed this or
>>>>>>>>>>>>>>>> explained why this have to be done here instead of in
>>>>>>> GCArguments.
>>>>>>>>>>>>>>> [Kharbas, Kishor] I moved this to GCArguments. But since
>>>>>>>>>>>>>>> this is also part of G1 patch, so it's not seen here.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thank you
>>>>>>>>>>>>>>> Kishor
>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.05_to_
>>>>>>>>>> 0
>>>>>>>>>>>>>>>>> 6
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Jtreg tests (hotspot_runtime, compiler+hotspot,
>>>>>>>>>>>>>>>>> serviceability,
>>>>>>>>>>>>>>>> vmTestbase_vm_gc) on my system gave me following
>>> results.
>>>>>>>>>>>>>>>>> (1) OpenJDK tip : passed: 1,806; failed: 24; error: 189
>>>>>>>>>>>>>>>>> (2) Parallel patch, no flag : passed: 1,590; failed:
>>>>>>>>>>>>>>>>> 15;
>>>>>>>>>>>>>>>>> error:
>>>>>>>>>>>>>>>>> 242
>>>>>>>>>>>>>>>>> (3) Parallel patch with flag : passed 1,816; failed: 15; error:
>>>>>>>>>>>>>>>>> 188
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> New failure in (1) -> (2)
>>>>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>>> compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runt
>>>>>>>>>>>>>>>> im
>>>>>>>>>>>>>>>> e/
>>>>>>>>>>>>>>>> te
>>>>>>>>>>>>>>>> st
>>>>>>>>>>>>>>>> /T
>>>>>>>>>>>>>>>> es
>>>>>>>>>>>>>>>> tResol
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> vedJavaType.java
>>>>>>>>>>>>>>>>> I looked at the report, but could not figure
>>>>>>>>>>>>>>>>> out why it fails. Most likely it's
>>>>>>>>>>>>>>>> my environment not being setup correctly.
>>>>>>>>>>>>>>>> I think this was an issue in OpenJDK pushed during the
>>>>>>>>>>>>>>>> weekend and fixed (test excluded) Monday afternoon.
>>>>>>>>>>>>>>>>> New failures in (1) -> (3)
>>>>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>>>> compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/run
>>>>>>>>>>>>>>>>> ti
>>>>>>>>>>>>>>>>> me
>>>>>>>>>>>>>>>>> /t
>>>>>>>>>>>>>>>>> es
>>>>>>>>>>>>>>>>> t/
>>>>>>>>>>>>>>>>> Te
>>>>>>>>>>>>>>>>> stR
>>>>>>>>>>>>>>>>> esolvedJavaType.java
>>>>>>>>>>>>>>>>> - compiler/aot/verification/vmflags/TrackedFlagTest.java
>>>>>>>>>>>>>>>>> Fails because we disable UseCompressedOops.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> @Stefan, I will do performance regression test with
>>>>>>>>>>>>>>>>> SPECjvm2008
>>>>>>>>>>>>>>>> benchmarks.
>>>>>>>>>>>>>>>> Good, SPECjvm2008 should not show any problems, but
>>> good
>>>>> to
>>>>>>>>>>>>>>>> run anyways.
>>>>>>>>>>>>>>>> I've run SPECjbb2015 without seeing any regressions so
>>>>>>>>>>>>>>>> that is also good.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Stefan
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>>>>>>>>> Sent: Tuesday, December 11, 2018 2:17 PM
>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>>>>>>>>>>>>>>>>> Johansson <stefan.johansson at oracle.com>; 'hotspot-
>> gc-
>>>>>>>>>>> dev at openjdk.java.net'
>>>>>>>>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev
>>> runtime'
>>>>>>>>>>>>>>>>>> <hotspot- runtime-dev at openjdk.java.net>
>>>>>>>>>>>>>>>>>> Cc: Viswanathan, Sandhya
>>>>> <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old
>>>>>>>>>>>>>>>>>> generation of java heap on alternate memory devices -
>>>>>>>>>>>>>>>>>> ParallelOld
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 12/11/18 1:00 AM, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Here is an updated webrev which fixes the comments
>>>>> below.
>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.05
>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04_to_05/
>>>>>>>>>>>>>>>>>> Here are some general comments:
>>>>>>>>>>>>>>>>>> - As I told in review thread of 8211425-G1 part review
>>>>>>>>>>>>>>>>>> thread, I would like to see separate patch for
>>>>>>>>>>>>>>>>>> Parallel GC
>>>>> part.
>>>>>>>>>>>>>>>>>> - A way not to use more than DRAM, same as G1.
>>>>>>>>>>>>>>>>>> - Testing results with/without this feature.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> And I have some minor comments here.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -----------------------------------
>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
>>>>>>>>>>>>>>>>>> 287 AdjoiningGenerations*
>>>>>>>>>>>>>>>>>>
>>> AdjoiningGenerations::create_adjoining_generations(ReservedSpac
>>>>>>>>>>> e
>>>>>>>>>>>>>>>>>> old_young_rs,
>>>>>>>>>>>>>>>>>> 288 GenerationSizer* policy,
>>>>>>>>>>>>>>>>>> 289 size_t alignment) {
>>>>>>>>>>>>>>>>>> - Wrong alignment at line 288/289.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -----------------------------------
>>>>>>>>>>>>>>>>>>
>> src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.
>>>>>>>>>>>>>>>>>> cp
>>>>>>>>>>>>>>>>>> p
>>>>>>>>>>>>>>>>>> 47 _total_size_limit =
>>>>>>>>>>>>>>>>>> policy->max_heap_byte_size();
>>>>>>>>>>>>>>>>>> - Please move back to initialization list unless
>>>>>>>>>>>>>>>>>> there's any good reason.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -----------------------------------
>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/generationSizer.cpp
>>>>>>>>>>>>>>>>>> - Copyright update
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 80 if(is_hetero_heap() &&
>>>>>>>>>>>>>>>>>> UseAdaptiveGCBoundary) {
>>>>>>>>>>>>>>>>>> 81 // This is the size that young gen can
>>>>>>>>>>>>>>>>>> grow to, when AdaptiveGCBoundary is true.
>>>>>>>>>>>>>>>>>> 83 // This is the size that old gen can
>>>>>>>>>>>>>>>>>> grow to, when AdaptiveGCBoundary is true.
>>>>>>>>>>>>>>>>>> - Line 81 and 83, 'AdaptiveGCBoundary' ->
>>>>>>>>>>> 'UseAdaptiveGCBoundary'.
>>>>>>>>>>>>>>>>>> -----------------------------------
>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/generationSizer.hpp
>>>>>>>>>>>>>>>>>> - Copyright update
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -----------------------------------
>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>>>>>>>>>>>>>>>>>> 1998 // We also return when its a heterogenous heap
>>>>>>>>>>>>>>>>>> because old generation cannot absorb data from eden
>>>>>>>>>>>>>>>>>> - 'also return' -> 'also return false'.
>>>>>>>>>>>>>>>>>> - 'its' -> 'it's' or 'it is'
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2000 if (!(UseAdaptiveSizePolicy &&
>>>>>>>>>>>>>>>>>> UseAdaptiveGCBoundary)
>>>>>>>>>>>>>>>>>> ||
>>>>>>>>>>>>>>>>>> ParallelScavengeHeap::heap()->ps_collector_policy()->i
>>>>>>>>>>>>>>>>>> s_
>>>>>>>>>>>>>>>>>> he
>>>>>>>>>>>>>>>>>> te
>>>>>>>>>>>>>>>>>> ro
>>>>>>>>>>>>>>>>>> _h
>>>>>>>>>>>>>>>>>> ea
>>>>>>>>>>>>>>>>>> p()
>>>>>>>>>>>>>>>>>> ) {
>>>>>>>>>>>>>>>>>> - Add new line for the new condition?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>> Sangheon
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I have create separate webrev for tests since they
>>>>>>>>>>>>>>>>>>> are common this issue and 8211425 -
>>>>>>>>>>>>>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.test.00
>>>>>>>>>>>>>>>>>>> /
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I am running jtreg tests for all hotspot tests to
>>>>>>>>>>>>>>>>>>> check the stability.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>>>>>> From: Stefan Johansson
>>>>>>>>>>>>>>>>>>>> [mailto:stefan.johansson at oracle.com]
>>>>>>>>>>>>>>>>>>>> Sent: Friday, November 30, 2018 6:12 AM
>>>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>>>>>>>>>>>>>>>>> 'hotspot-gc- dev at openjdk.java.net'
>>>>>>>>>>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>;
>>>>>>>>>>>>>>>>>>>> 'Hotspot dev runtime'
>>>>>>>>>>>>>>>>>>>> <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>>>>>>>>>>>>>>> sangheon.kim at oracle.com
>>>>>>>>>>>>>>>>>>>> Cc: Viswanathan, Sandhya
>>>>>>> <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>>>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old
>>>>>>>>>>>>>>>>>>>> generation of java heap on alternate memory devices
>>>>>>>>>>>>>>>>>>>> - ParallelOld
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 2018-11-22 08:17, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Requesting review of the patch for allocating old
>>>>>>>>>>>>>>>>>>>>> generation of parallelold gc on alternate memory
>>>>>>>>>>>>>>>>>>>>> devices such
>>>>>>>>>> nv-dimm.
>>>>>>>>>>>>>>>>>>>>> The design of this implementation is explained on
>>>>>>>>>>>>>>>>>>>>> the bug page
>>>>>>>>>>>>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-
>> 8211424.
>>>>>>>>>>>>>>>>>>>>> This is subtask of
>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202286
>>>>>>>>>>>>>>>>>>>>> which is implementation for parallel old gc.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Please follow the parent issue here : 64 if
>>>>>>>>>>>>>>>>>>>>> (AllocateOldGenAt != NULL
>>>>>>>>>>>>>>>>>> &&
>>>>>>>>>>>>>>>>>>>> UseAdaptiveGCBoundary) {
>>>>>>>>>>>>>>>>>>>> 65 heap_size =
>>>>>>>>>>>>>>>>>>>>
>>> AdjoiningGenerationsForHeteroHeap::required_reserved_memory(_collec
>>>>>>>>>>> t
>>>>>>>>>>>>>>>> o
>>>>>>>>>>>>>>>>>>>> r
>>>>>>>>>>>>>>>>>>>> _policy);
>>>>>>>>>>>>>>>>>>>> 66 }
>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202286.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> (PS: this is continuation of old thread 'RFR(M):
>>> 8204908:
>>>>>>>>>>>>>>>>>>>>> NVDIMM for POGC and G1GC - ReserveSpace.cpp
>>>>> changes
>>>>>>> are
>>>>>>>>>>>>> mostly
>>>>>>>>>>>>>>>>>>>>> eliminated/no collector specific code.')
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04
>>>>>>>>>>>>>>>>>>>> First a general comment which goes a long the same
>>>>>>>>>>>>>>>>>>>> lines as many of my comments for the G1 part. I
>>>>>>>>>>>>>>>>>>>> would like to see some more abstraction around the
>>>>>>>>>>>>>>>>>>>> use of AllocateOldGenAt, I think we should consider
>>>>>>>>>>>>>>>>>>>> it during argument parsing to turn off unsupported
>>>>>>>>>>>>>>>>>>>> things like compressed oops and once during
>>>>>>>>>>>>>>>>>>>> initialization of the GC to know if we are using the
>>>>>>>>>>>>>>>>>>>> feature or not. After that there should only be
>>>>>>>>>>>>>>>>>>>> calls like
>>>>>>>>>>>>>>>>>>>> policy->is_hetero_heap() if we really need to decide
>>>>>>>>>>>>>>>>>>>> policy->on
>>>>> this.
>>> src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
>>>>>>>>>>>>>>>>>>>> ----------------------------------------------------
>>>>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>>>> 97
>>>>>>>>>>>>>>>>>>>> virtual_spaces()->reserved_space().first_part(max_lo
>>>>>>>>>>>>>>>>>>>> w_ by te _s iz e, AllocateOldGenAt != NULL /* split
>>>>>>>>>>>>>>>>>>>> */);
>>>>>>>>>>>>>>>>>>>> One example of what I mention above, I
>>>>>>>>>>>>>>>>>>>> think you should consider doing the same as you did
>>>>>>>>>>>>>>>>>>>> for G1 and create the ParallelScavengeHeap with at
>>>>>>>>>>>>>>>>>>>> CollectorPolicy that is nvdimm
>>>>>>>>>>> aware.
>>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/asPSOldGen.hpp
>>>>>>>>>>>>>>>>>>>> --------------------------------------------
>>>>>>>>>>>>>>>>>>>> 32 #include
>>>>>>>>>>>>>>>>>>>> "gc/parallel/psFileBackedVirtualspace.hpp"
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I see no use of this in this header.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>> src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp
>>>>>>>>>>>>>>>>>>>> ------------------------------------------------------
>>>>>>>>>>>>>>>>>>>> 65 // is the heap backed by two
>>>>>>>>>>>>>>>>>>>> different
>>> memories?
>>>>>>>>>>>>>>>>>>>> 66 bool _is_hetero_heap;
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> As for G1 I think this doesn't need to be stored in
>>>>>>>>>>>>>>>>>>>> the heap but the CollectorPolicy (GenerationSizer)
>>>>>>>>>>>>>>>>>>>> knows this and can be queried when needed. Keeping
>>> the
>>>>>>>>>>>>>>>>>>>> getter is fine, but just have it call the policy.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/parallel/psOldGen.cpp
>>>>>>>>>>>>>>>>>>>> ------------------------------------------
>>>>>>>>>>>>>>>>>>>> 75 if(AllocateOldGenAt != NULL) {
>>>>>>>>>>>>>>>>>>>> 76 _virtual_space = new
>>>>>>>>>>>>>>>>>>>> PSFileBackedVirtualSpace(rs, alignment,
>>>>>>>>>>>>>>>>>>>> AllocateOldGenAt);
>>>>>>>>>>>>>>>>>>>> 77 if (!(static_cast
>>>>>>>>>>>>>>>>>>>> <PSFileBackedVirtualSpace*>(_virtual_space))->initia
>>>>>>>>>>>>>>>>>>>> li
>>>>>>>>>>>>>>>>>>>> ze
>>>>>>>>>>>>>>>>>>>> ()
>>>>>>>>>>>>>>>>>>>> )
>>>>>>>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>>>>>>> 78 vm_exit_during_initialization("Could
>>>>>>>>>>>>>>>>>>>> not map space for PSOldGen at given
>> AllocateOldGenAt
>>> path");
>>>>>>>>>>>>>>>>>>>> 79 }
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Either pass in if this is a hetero heap or ask the
>>>>>>>>>>>>>>>>>>>> heap, it can be obtained by
>>> ParallelScavengeHeap::heap().
>>>>>>>>>>>>>>>>>>>> I also think it would be better to have the
>>>>>>>>>>>>>>>>>>>> PSFileBackedVirtualSpace constructor call initialize
>>>>>>>>>>>>>>>>>>>> directly and let it fail rather than returning
>>>>>>>>>>>>>>>>>>>> false, since we will abort the VM in this
>>>>>>>>>>>>>>>> case anyway.
>>>>>>>>>>>>>>>>>>>> src/hotspot/share/memory/universe.cpp
>>>>>>>>>>>>>>>>>>>> -------------------------------------
>>>>>>>>>>>>>>>>>>>> 829 // When AllocateOldGenAt is set, we
>>>>>>>>>>>>>>>>>>>> cannot use largepages for entire heap memory.
>>>>>>>>>>>>>>>>>>>> 830 // Only young gen which is allocated
>>>>>>>>>>>>>>>>>>>> in dram can use large pages, but we currently don't
>>>>>>>>>>>>>>>>>>>> support
>>>>> that.
>>>>>>>>>>>>>>>>>>>> 831 use_large_pages = (AllocateOldGenAt
>>>>>>>>>>>>>>>>>>>> !=
>>>>>>>>>>>>>>>>>>>> NULL) ? false :
>>>>>>>>>>>>>>>>>>>> use_large_pages;
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Move this to argument parsing and do
>>>>>>>>>>>>>>>>>> FLAG_SET_DEFAULT(UseLargePages,
>>>>>>>>>>>>>>>>>>>> false) when nvdimm is used.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>>>>>>>>>>>>>>> ---------------------------------------
>>>>>>>>>>>>>>>>>>>> 1627 if(AllocateOldGenAt != NULL) {
>>>>>>>>>>>>>>>>>>>> 1628 FLAG_SET_ERGO(bool, UseCompressedOops,
>>>>> false);
>>>>>>>>>>>>>>>>>>>> 1629 return;
>>>>>>>>>>>>>>>>>>>> 1630 }
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> As for G1 I can't see no reason why this can't be
>>>>>>>>>>>>>>>>>>>> moved to
>>>>>>>>>>>>>>>>>> GCArguments::init.
>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>> Stefan
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.03_to_04
>>>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>>>>>>>>
More information about the hotspot-gc-dev
mailing list