8299915: Remove ArrayAllocatorMallocLimit and associated code #11931

jcking · 2023-01-10T20:55:12Z

Remove abstraction that is a holdover from Solaris. Direct usages of MmapArrayAllocator have been switched to normal malloc. The justification is that none of the code paths are called from signal handlers, so using mmap directly does not make sense and is potentially slower than going through malloc which can potentially re-use memory without making any system calls. The remaining usages of ArrayAllocator and MallocArrayAllocator are equivalent.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8299915: Remove ArrayAllocatorMallocLimit and associated code

Contributors

Stefan Karlsson <stefank@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/11931/head:pull/11931
$ git checkout pull/11931

Update a local copy of the PR:
$ git checkout pull/11931
$ git pull https://git.openjdk.org/jdk.git pull/11931/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11931

View PR using the GUI difftool:
$ git pr show -t 11931

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11931.diff

Webrev

Link to Webrev Comment

Signed-off-by: Justin King <jcking@google.com>

bridgekeeper · 2023-01-10T20:56:45Z

👋 Welcome back jcking! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-10T21:28:47Z

@jcking The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-10T21:33:48Z

Webrevs

Signed-off-by: Justin King <jcking@google.com>

stefank · 2023-01-11T10:05:33Z

I'm happy to see this flag getting removed. I'm less happy about seeing usages of the array allocators being replaced by macros. I'd rather see an effort towards getting rid of these macros. Could we limit this patch to only remove the ArrayAllocatorMallocLimit flag and ArrayAllocator class, and defer the discussion around what API to use for array allocations?

jcking · 2023-01-11T14:27:44Z

I'm happy to see this flag getting removed. I'm less happy about seeing usages of the array allocators being replaced by macros. I'd rather see an effort towards getting rid of these macros. Could we limit this patch to only remove the ArrayAllocatorMallocLimit flag and ArrayAllocator class, and defer the discussion around what API to use for array allocations?

ArrayAllocator with ArrayAllocatorMallocLimit removed is effectively MallocArrayAllocator. Are you suggesting leaving MallocArrayAllocator and MmapArrayAllocator thus update references to ArrayAllocator to be MallocArrayAllocator?

As far as APIs, the majority of the codebase uses the macros. IMO consistency would be better and having two ways of doing things doesn't help. But if you feel strongly about it, we can punt and just surgically remove the bare minimum, assuming you clarify your expectation (see above).

tstuefe · 2023-01-11T14:49:59Z

Curious, I always thought we do ArrayAllocator - using mmap for larger allocations - to prevent memory retention for libc variants whose allocators are "grabby", i.e. which don't promptly return memory to the OS on free(). E.g. because they only use sbrk (Solaris, AIX), or are just cautious about returning memory (glibc).

Glibc's retention problem is only relevant for fine-grained allocations, so for glibc this is probably fine. This leaves at least AIX as a potential problem. @backwaterred, does the AIX libc malloc() still exclusively use the data segment ? Does free'd memory still stick to the process?

(While writing this, I remember that we at SAP even rewrote Arena allocation to use mmap for AIX, because large compile arenas caused lasting RSS increase, so it has definitely been a problem in the past)

stefank · 2023-01-11T15:08:23Z

ArrayAllocator with ArrayAllocatorMallocLimit removed is effectively MallocArrayAllocator. Are you suggesting leaving MallocArrayAllocator and MmapArrayAllocator thus update references to ArrayAllocator to be MallocArrayAllocator?

Yes, that was what I wanted.

As far as APIs, the majority of the codebase uses the macros. IMO consistency would be better and having two ways of doing things doesn't help. But if you feel strongly about it, we can punt and just surgically remove the bare minimum, assuming you clarify your expectation (see above).

I agree about the argument about consistency, so I retract my objection. We can deal with these macros as a separate RFE (if we ever get to it).

I would like to retain the usage of mmapped memory for ZGC to minimize the risk of any surprises for us. ZGC code also tend to initialize as much as possible in the initialization list, so the extra memset that comes after initialization lists sticks out a bit. Could you create a private ZGranuleMap::allocate_array function that uses os::reserve_memory/commmit_memory and change the code to be:

inline ZGranuleMap<T>::ZGranuleMap(size_t max_offset) :
    _size(max_offset >> ZGranuleSizeShift),
    _map(allocate_array(_size)) {

Or if you like, I can provide a patch on top of your branch to do that.

Signed-off-by: Justin King <jcking@google.com>

jcking · 2023-01-11T17:58:33Z

ArrayAllocator with ArrayAllocatorMallocLimit removed is effectively MallocArrayAllocator. Are you suggesting leaving MallocArrayAllocator and MmapArrayAllocator thus update references to ArrayAllocator to be MallocArrayAllocator?

Yes, that was what I wanted.

As far as APIs, the majority of the codebase uses the macros. IMO consistency would be better and having two ways of doing things doesn't help. But if you feel strongly about it, we can punt and just surgically remove the bare minimum, assuming you clarify your expectation (see above).

I agree about the argument about consistency, so I retract my objection. We can deal with these macros as a separate RFE (if we ever get to it).

I would like to retain the usage of mmapped memory for ZGC to minimize the risk of any surprises for us. ZGC code also tend to initialize as much as possible in the initialization list, so the extra memset that comes after initialization lists sticks out a bit. Could you create a private ZGranuleMap::allocate_array function that uses os::reserve_memory/commmit_memory and change the code to be:
inline ZGranuleMap<T>::ZGranuleMap(size_t max_offset) :
    _size(max_offset >> ZGranuleSizeShift),
    _map(allocate_array(_size)) {
Or if you like, I can provide a patch on top of your branch to do that.

The extra memset is due to malloc not handing out zero initialized memory, it looks like zGranuleMap is sensitive to that.

Done. Restored mmap as suggested for zGranuleMap.

jcking · 2023-01-11T18:10:47Z

Curious, I always thought we do ArrayAllocator - using mmap for larger allocations - to prevent memory retention for libc variants whose allocators are "grabby", i.e. which don't promptly return memory to the OS on free(). E.g. because they only use sbrk (Solaris, AIX), or are just cautious about returning memory (glibc).

Glibc's retention problem is only relevant for fine-grained allocations, so for glibc this is probably fine. This leaves at least AIX as a potential problem. @backwaterred, does the AIX libc malloc() still exclusively use the data segment ? Does free'd memory still stick to the process?

(While writing this, I remember that we at SAP even rewrote Arena allocation to use mmap for AIX, because large compile arenas caused lasting RSS increase, so it has definitely been a problem in the past)

Correct, glibc always mmap's allocations above a certain limit and always unmaps them on free. I believe most implementations do that, either immediately unmapping or using madvise. AIX still uses sbrk, based on their documentation that I could find. Though AIX does have something called malloc disclaim.

Signed-off-by: Justin King <jcking@google.com>

coleenp · 2023-01-11T18:52:23Z

To follow up on @tstuefe comment - and the one that I tried to say in the bug was that we added this MmapArrayAllocate feature for some G1 marking bits that used so much memory that hit the Solaris _sbrk issue. Maybe @stefank and @tschatzl remember this issue. Maybe it's ok for AIX, then removing this code is a good change. Maybe the G1 usages need a mmap implementation though.

Signed-off-by: Justin King <jcking@google.com>

jcking · 2023-01-11T20:56:27Z

To follow up on @tstuefe comment - and the one that I tried to say in the bug was that we added this MmapArrayAllocate feature for some G1 marking bits that used so much memory that hit the Solaris _sbrk issue. Maybe @stefank and @tschatzl remember this issue. Maybe it's ok for AIX, then removing this code is a good change. Maybe the G1 usages need a mmap implementation though.

The padding.inline.hpp usage seems to have one caller which is called once. The other mmap usage in G1 we can convert to mmap using a similar approach to zGranuleMap if that is preferred. That would then be equivalent behavior, it looks like the G1 code uses the page allocation granularity anyway so maybe keeping it mmap is the better way to go here anyway?

stefank

Thanks for the ZGC update. I have a request to slightly modify the style a bit. Would you mind taking this patch:
https://github.com/stefank/jdk/tree/pr_11931

There's a FIXME about that allocation.inline.hpp file is now empty. Will you remove it in this patch? I'd prefer if you did. Alternatively, remove the FIXME from this patch and then immediately clean this up as a separate patch.

tstuefe · 2023-01-12T16:52:50Z

To follow up on @tstuefe comment - and the one that I tried to say in the bug was that we added this MmapArrayAllocate feature for some G1 marking bits that used so much memory that hit the Solaris _sbrk issue. Maybe @stefank and @tschatzl remember this issue. Maybe it's ok for AIX, then removing this code is a good change. Maybe the G1 usages need a mmap implementation though.

The padding.inline.hpp usage seems to have one caller which is called once. The other mmap usage in G1 we can convert to mmap using a similar approach to zGranuleMap if that is preferred. That would then be equivalent behavior, it looks like the G1 code uses the page allocation granularity anyway so maybe keeping it mmap is the better way to go here anyway?

My uninformed opinion (I'm not the G1 code owner) is that it would be fine to use explicit mmap. I'd love the complexity reduction this patch brings.

Signed-off-by: Justin King <jcking@google.com>

jcking · 2023-01-13T17:32:21Z

/contributor add @stefank

jcking · 2023-01-13T17:33:32Z

Applied your patch @stefank

openjdk · 2023-01-13T17:34:52Z

@jcking
Contributor Stefan Karlsson <stefank@openjdk.org> successfully added.

Signed-off-by: Justin King <jcking@google.com>

jcking · 2023-01-13T17:46:43Z

Restored mmap for G1, though in the future it might make sense to switch to just using malloc.

Signed-off-by: Justin King <jcking@google.com>

jcking · 2023-01-19T17:41:51Z

There's a FIXME about that allocation.inline.hpp file is now empty. Will you remove it in this patch? I'd prefer if you did. Alternatively, remove the FIXME from this patch and then immediately clean this up as a separate patch.

Yeah, separate patch immediately after is probably best. Trying to avoid having to mass update includes and enter merge conflict hell for this patch. After is easier.

backwaterred · 2023-01-20T23:13:41Z

This leaves at least AIX as a potential problem. @backwaterred, does the AIX libc malloc() still exclusively use the data segment? Does free'd memory still stick to the process?

I agree with @coleenp that AIX seems to still use the data segment and sbrk for dynamic memory allocation according to the documentation. [Edit: ~~I am not sure about memory sticking to the process.~~ The section on load balancing seems to indicate that memory will indeed stick to the process unless the MALLOCOPTIONS variable is set to enable balancing.]

With these changes I see several of the gtests failing on AIX.

[==========] 58 tests from 6 test cases ran. (3718 ms total)
[  PASSED  ] 53 tests.
[  FAILED  ] 5 tests, listed below:
[  FAILED  ] os.safefetchN_negative_vm
[  FAILED  ] os.safefetch32_negative_vm
[  FAILED  ] os.safefetchN_negative_current_null_vm
[  FAILED  ] os.safefetch32_negative_current_null_vm
[  FAILED  ] os.safefetch_negative_at_safepoint_vm

I can experiment with the disclaim option as well.

[1] https://www.ibm.com/docs/en/aix/7.2?topic=concepts-system-memory-allocation-using-malloc-subsystem

openjdk · 2023-02-17T19:09:11Z

@jcking this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout remove-array-allocator
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

bridgekeeper · 2023-03-17T23:43:35Z

@jcking This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-04-15T01:45:00Z

@jcking This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

iklam · 2023-05-22T16:45:23Z

@tstuefe @backwaterred I'd like to see this RFE revived. Do we know if anyone is using the ArrayAllocatorMallocLimit flag in any production environment today?

It seems unlikely to me, as you'd need to explicitly specify -XX:+UnlockExperimentalVMOptions in the command-line.

And, if this option had been useful (for the AIX port, for example), it would have been changed to a non-experimental (with proper _pd support) option over the past 10 years.

tstuefe · 2023-05-22T17:48:33Z

src/hotspot/share/gc/z/zGranuleMap.inline.hpp

+
+template <typename T>
+size_t ZGranuleMap<T>::size_for_array(size_t count) {
+  return align_up(count * sizeof(T), os::vm_allocation_granularity());


assert ag. overflow?

tstuefe · 2023-05-22T17:51:54Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

+
+  void* const addr = os::reserve_memory(size, !ExecMem, mtGC);
+  if (addr == nullptr) {
+    return nullptr;


reserve fails, we return null, commit fails, we exit? Why the inconsistency?

tstuefe · 2023-05-22T17:53:19Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

@@ -126,6 +123,33 @@ bool G1CMMarkStack::resize(size_t new_capacity) {
  return true;
 }



Here, and in ZGC: we are cool with small allocations being rounded up to page size now?

tstuefe · 2023-05-22T17:54:26Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

+}
+
+size_t G1CMMarkStack::size_for_array(size_t count) {
+  return align_up(count * sizeof(TaskQueueEntryChunk), os::vm_allocation_granularity());


assert against overflow

tstuefe · 2023-05-22T17:58:08Z

src/hotspot/share/memory/padded.inline.hpp

  // Clear the allocated memory.
+  memset(chunk, '\0', total_size);
+


Old code, when doing the malloc path, did not initialize. Why here?

tstuefe · 2023-05-22T17:59:01Z

test/lib-test/jdk/test/whitebox/vm_flags/SizeTTest.java

@@ -35,7 +35,7 @@
 import jdk.test.lib.Platform;

 public class SizeTTest {
-    private static final String FLAG_NAME = "ArrayAllocatorMallocLimit";
+    private static final String FLAG_NAME = "StringDeduplicationCleanupDeadMinimum";


Small comment here that the flag itself, does not matter, just the fact that it is of type size_t?

tstuefe

Cursory review

tstuefe · 2023-05-22T18:01:28Z

@tstuefe @backwaterred I'd like to see this RFE revived. Do we know if anyone is using the ArrayAllocatorMallocLimit flag in any production environment today?

It seems unlikely to me, as you'd need to explicitly specify -XX:+UnlockExperimentalVMOptions in the command-line.

And, if this option had been useful (for the AIX port, for example), it would have been changed to a non-experimental (with proper _pd support) option over the past 10 years.

@iklam I'm fine with removing the ArrayAllocator.

stefank

Could we create a simpler version of this PR? One that only removes ArrayAllocator and ArrayAllocatorMallocLimit, but keeps MallocArrayAllocator and MmapArrayAllocator? That way we can figure out later, in a separate RFE, if we want to remove MmapArrayAllocator.

jcking added 2 commits January 10, 2023 12:36

Remove ArrayAllocatorMallocLimit

0ba86a5

Signed-off-by: Justin King <jcking@google.com>

Use REALLOC_C_HEAP_ARRAY instead of NEW_C_HEAP_ARRAY/FREE_C_HEAP_ARRAY

efb1a8b

Signed-off-by: Justin King <jcking@google.com>

jcking marked this pull request as ready for review January 10, 2023 20:57

openjdk bot changed the title ~~JDK-8299915: Remove ArrayAllocatorMallocLimit~~ 8299915: Remove ArrayAllocatorMallocLimit and associated code Jan 10, 2023

openjdk bot added the rfr Pull request is ready for review label Jan 10, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jan 10, 2023

jcking added 3 commits January 10, 2023 13:35

Use unconstrained flag for SizeTTest.java

9e96ddb

Signed-off-by: Justin King <jcking@google.com>

Remove ArrayAllocatorMallocLimit test

235abed

Signed-off-by: Justin King <jcking@google.com>

Initialize memory to zero in zGranuleMap

3216fb1

Signed-off-by: Justin King <jcking@google.com>

Restore mmap allocator for zGranuleMap

d30748e

Signed-off-by: Justin King <jcking@google.com>

Ensure Padded2DArray clears memory to preserve compatibility with mmap

cf3097e

Signed-off-by: Justin King <jcking@google.com>

Fix incompatible pointer types

5d30170

Signed-off-by: Justin King <jcking@google.com>

stefank suggested changes Jan 12, 2023

View reviewed changes

Apply patch from stefank@

4aba784

Signed-off-by: Justin King <jcking@google.com>

jcking added 2 commits January 13, 2023 09:44

Preserve mmap usage in gc/g1/g1ConcurrentMark

b9e71a2

Signed-off-by: Justin King <jcking@google.com>

Fix compilation error

4b5ce84

Signed-off-by: Justin King <jcking@google.com>

Do not pass nullptr to os::release_memory

de7d3ab

Signed-off-by: Justin King <jcking@google.com>

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 17, 2023

openjdk bot removed the rfr Pull request is ready for review label Apr 3, 2023

bridgekeeper bot closed this Apr 15, 2023

iklam mentioned this pull request May 22, 2023

8302218: CHeapBitMap::free frees with incorrect size #14079

Closed

3 tasks

tstuefe reviewed May 22, 2023

View reviewed changes

stefank reviewed May 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8299915: Remove ArrayAllocatorMallocLimit and associated code #11931

8299915: Remove ArrayAllocatorMallocLimit and associated code #11931

jcking commented Jan 10, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

mlbridge bot commented Jan 10, 2023 •

edited

Loading

stefank commented Jan 11, 2023

jcking commented Jan 11, 2023

tstuefe commented Jan 11, 2023 •

edited

Loading

stefank commented Jan 11, 2023

jcking commented Jan 11, 2023

jcking commented Jan 11, 2023

coleenp commented Jan 11, 2023

jcking commented Jan 11, 2023

stefank left a comment

tstuefe commented Jan 12, 2023

jcking commented Jan 13, 2023

jcking commented Jan 13, 2023

openjdk bot commented Jan 13, 2023

jcking commented Jan 13, 2023

jcking commented Jan 19, 2023

backwaterred commented Jan 20, 2023 •

edited

Loading

openjdk bot commented Feb 17, 2023

bridgekeeper bot commented Mar 17, 2023

bridgekeeper bot commented Apr 15, 2023

iklam commented May 22, 2023

tstuefe May 22, 2023

tstuefe May 22, 2023

tstuefe May 22, 2023

tstuefe May 22, 2023

tstuefe May 22, 2023

tstuefe May 22, 2023

tstuefe left a comment

tstuefe commented May 22, 2023

stefank left a comment

		@@ -126,6 +123,33 @@ bool G1CMMarkStack::resize(size_t new_capacity) {
		return true;
		}

		// Clear the allocated memory.
		memset(chunk, '\0', total_size);

8299915: Remove ArrayAllocatorMallocLimit and associated code #11931

8299915: Remove ArrayAllocatorMallocLimit and associated code #11931

Conversation

jcking commented Jan 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Contributors

Reviewing

Webrev

bridgekeeper bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

mlbridge bot commented Jan 10, 2023 • edited Loading

Webrevs

stefank commented Jan 11, 2023

jcking commented Jan 11, 2023

tstuefe commented Jan 11, 2023 • edited Loading

stefank commented Jan 11, 2023

jcking commented Jan 11, 2023

jcking commented Jan 11, 2023

coleenp commented Jan 11, 2023

jcking commented Jan 11, 2023

stefank left a comment

Choose a reason for hiding this comment

tstuefe commented Jan 12, 2023

jcking commented Jan 13, 2023

jcking commented Jan 13, 2023

openjdk bot commented Jan 13, 2023

jcking commented Jan 13, 2023

jcking commented Jan 19, 2023

backwaterred commented Jan 20, 2023 • edited Loading

openjdk bot commented Feb 17, 2023

bridgekeeper bot commented Mar 17, 2023

bridgekeeper bot commented Apr 15, 2023

iklam commented May 22, 2023

tstuefe May 22, 2023

Choose a reason for hiding this comment

tstuefe May 22, 2023

Choose a reason for hiding this comment

tstuefe May 22, 2023

Choose a reason for hiding this comment

tstuefe May 22, 2023

Choose a reason for hiding this comment

tstuefe May 22, 2023

Choose a reason for hiding this comment

tstuefe May 22, 2023

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

tstuefe commented May 22, 2023

stefank left a comment

Choose a reason for hiding this comment

jcking commented Jan 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 10, 2023 •

edited

Loading

tstuefe commented Jan 11, 2023 •

edited

Loading

backwaterred commented Jan 20, 2023 •

edited

Loading