JDK-8244189 : Remove second argument in the two-arg ReservedSpace constructor
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: gc
  • Affected Version: 15
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2020-04-30
  • Updated: 2021-04-21
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
tbdUnresolved
Related Reports
Relates :  
JDK-8243393 - Improve ReservedSpace constructor resolution
Relates :  
JDK-8265066 - Split ReservedSpace constructor to avoid default parameter
Relates :  
JDK-8244107 - Incorrect parameters in ReservedSpace constructor change
Relates :  
JDK-8265268 - Unify ReservedSpace reservation code in initialize and try_reserve_heap
Description
This two-arg constructor can be accidentally used instead of the 4-arg constructor and the one caller in g1 and the 3 in shenandoah can use the 4 arg constructor instead to specify alignment and large pages.

I have a tested patch for this but not sure if it covers the cross product of windows where os::vm_allocation_granularity != os::vm_page_size, and UseLargePages correctly, or how to tell.  So assigning to category GC.
Comments
You also might have fixed this with another recent change so can close as WNF or NAI.
21-04-2021
[~sjohanss] This was a small change to remove a parameter in ReservedSpace but it wasn't right. Can you have a look at fixing this since your change for JDK-8265268 touches the same area and looks like a nice cleanup.
21-04-2021
Also looks like the Shenandoah patch turns off large pages, where it previously could use them.
30-04-2020
This doesn't look right: G1RegionToSpaceMapper* G1CollectedHeap::create_aux_memory_mapper(const char* description, size_t size, size_t translation_factor) { size_t preferred_page_size = os::page_size_for_region_unaligned(size, 1); + // Allocate a new reserved space, preferring to use large pages. - ReservedSpace rs(size, preferred_page_size); + ReservedSpace rs(size, preferred_page_size, UseLargePages); It forcefully sets it to use large pages even when preferred_page_size is small.
30-04-2020
My patch passes tier1-3, and makes it more difficult to call the wrong constructor by mistake. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8244189.01/webrev Currently, If you do a single argument ReservedSpace rs(size), you get an address aligned to allocation granularity: ReservedSpace::ReservedSpace(size_t size, size_t preferred_page_size) : _fd_for_heap(-1) { bool has_preferred_page_size = preferred_page_size != 0; // Want to use large pages where possible and pad with small pages. size_t page_size = has_preferred_page_size ? preferred_page_size : os::page_size_for_region_unaligned(size, 1); bool large_pages = page_size != (size_t)os::vm_page_size(); size_t alignment; if (large_pages && has_preferred_page_size) { alignment = MAX2(page_size, (size_t)os::vm_allocation_granularity()); // ReservedSpace initialization requires size to be aligned to the given // alignment. Align the size up. size = align_up(size, alignment); } else { // Don't force the alignment to be large page aligned, // since that will waste memory. alignment = os::vm_allocation_granularity(); <==== HERE } initialize(size, alignment, large_pages, NULL, false); } And then: void ReservedSpace::initialize(size_t size, size_t alignment, bool large, char* requested_address, bool executable) { const size_t granularity = os::vm_allocation_granularity(); assert((size & (granularity - 1)) == 0, "size not aligned to os::vm_allocation_granularity()"); assert((alignment & (granularity - 1)) == 0, "alignment not aligned to os::vm_allocation_granularity()"); then later in initialize, if you pass alignment 0 to the 4 argument ReservedSpace constructor (which cardTable.cpp does because _page_size always is os::page_size()), you get: alignment = MAX2(alignment, (size_t)os::vm_page_size()); But it appears that the size is asserted to always be a multiple of os::vm_allocation_granularity(), but the alignment can be os::page_size() or os::vm_allocation_granularity() depending on what you pass to which constructor. UseLargePages could be different. If UseLargePages is true, the call to ReservedSpace with one argument, and preferred_page_size = 0 (default parameter), but might pass large_pages = true to initialize if the size passed in is big enough. Not sure if that's what this code wants to do in the callers either in GC.
30-04-2020