RFR (xs): 8211735: Wrong heap mapper can be selected with UseLargePages on G1 (original) (raw)

sangheon.kim at oracle.com sangheon.kim at oracle.com
Wed Nov 21 04:54:25 UTC 2018

Hi all,

On 11/19/18 11:48 AM, sangheon.kim at oracle.com wrote:

Hi Thomas,

On 11/15/18 4:07 AM, Thomas Schatzl wrote: Hi Sangheon,

On Fri, 2018-11-09 at 13:17 -0800, sangheon.kim at oracle.com wrote: Hi Thomas,

Thanks for looking at this. On 11/7/18 3:26 PM, Thomas Schatzl wrote: Hi,

On Wed, 2018-11-07 at 14:38 -0800, sangheon.kim at oracle.com wrote: Hi Kim,

Thanks for your review. On 11/7/18 1:47 PM, Kim Barrett wrote: On Nov 7, 2018, at 4:22 PM, sangheon.kim at oracle.com wrote:

Hi all, Can I have reviews for this tiny patch fixes wrong heap mapper selection with UseLargePages on G1? It is unfortunate that we can't ask the ReservedSpace directly whether it supports large pages. Okay, applied this at the new webrev. My initial version was something like your suggestion, but dropped as I  understood only 1 place needs it. But it was wrong as you pointed below. Also, isn't there the same problem when determining the heap mapper for the auxiliary data structures in createauxmemorymapper, and shouldn't there be similar changes there? Yes, same treatment is needed there too. Before filing this bug, I thought we don't need this for the auxiliary data structures, but I was wrong. webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.2/ Testing: hs-tier1~3. PS. I'm also fine with going with webrev.1 style with auxiliary structure treated. After some thinking, and also looking at JDK-8213927 I would prefer the webrev.1 style change. Just put the code into a static helper function in g1CollectedHeap.cpp. Okay. The reason is that I believe that the large page code, and determining for which purpose we can assume large pages and the exceptions when not, is a bit messed up or at least underdocumented. Maybe partially because of the Linux large page mess :) Right!!!  From what I understand it seems that there is quite a bit of confusion in the code what "cancommitlargepagememory()" actually means; I believe at least some uses are incomplete (i.e. not considering the "special" flag) or not giving any reason why they do not consider it. I do not completely understand why UseTransparentHugePages implicitly enables UseLargePages either. I think at this point we should not add to the ReservedSpace confusion any further by adding more ReservedSpace methods. Sorry for initially suggesting that. I agree especially not adding more confusion at ReservedSpace. :) I would also prefer to use name for that function like "preferred/actualcommitpagesize(ReservedSpace rs)" to make it clear it is a page size for committing and not for any other purpose (which is the trap the bug exposed by JDK-8213927 fell into). I would prefer to use 'actualreservedpagesize()' as it is reserved and not committed yet. Or as you say it is for committing so if the name has such meaning, I am okay too. 'xxxcommitpagesize()' feels like 'committed' to me. Other than that the actual algorithm to determine the memory commit page size is good. Thanks. (We had a chat about this but adding here for the record) Same treatment at the auxiliary data structure is not needed. In the previous email I said we need the same one for the aux. structures because I understood the code: if the aux data structure size is smaller than current large page size, we should align up and then use large page. But current code is intentionally not use larger page, if the aux data structure size is less. e.g. aux. data structure size=3mb, large page size=4mb, currently we use default page in this case. So this new webrev 3 has a new static helper function added but only used for heap. webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.3 (full) http://cr.openjdk.java.net/~sangheki/8211735/webrev.3to2 (incremental) Testing: hs-tier1 ~ 3 Sorry, I have to post a new version for addressing aux. data structure case as well.

webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.4 (full) http://cr.openjdk.java.net/~sangheki/8211735/webrev.4_to_3 (incremental) Testing: hs-tier1 ~ 3

Thanks, Sangheon

Thanks, Sangheon

Thanks, Thomas

More information about the hotspot-gc-dev mailing list