8364248: Separate memory limit detection #26530
Conversation
|
👋 Welcome back jsikstro! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/cc hotspot-gc |
|
@jsikstro |
Webrevs
|
There was a problem hiding this comment.
Generally good. But why not simply return the limit. Unlimited = size_max.
Oh, I see ZGC does that already. I would adopt that for has_limited_virtual_address_space. Then you can remove the ZGC implementation completely.
Side note, to be unbearably nitpicky, it should theoretically be "address space limit - os::vm_min_address()" since the OS does not allow low-address mappings below that.
This would make the ZGC could read more nicely I think. I like it!
Do you think it would be prudent to add this now? To clarify, if there is a limit on POSIX, return |
Nah, this was a half joke tbh. I don't think this fidelity is needed, and it may confuse people if we get a weirdly unaligned size back. E.g. if on Linux, the min map is configured to be just one page, you could get something like "4PB - 4K". Also, you won't ever be able to use the full extend of that size anyway, since of course it is pre-populated with all kind of mappings, is fragmented, etc. Side note, it would be valuable to have a sister function to os::vm_min_address() that returns the maximum mappable user address. I did hard code values at various places, but I believe ZGC has some function like this that is more elegant and more truthful. Would be nice to have this in os space. |
|
I have some second thoughts about this. Sorry for the bikeshedding, but naming is important, especially since we never bother with comments on prototypes. "allocatable_memory_limit" implies much more than it delivers. How much memory you can allocate depends on many factors, only one of which being the user-addressable address space size. For example, how much you can commit depends on OS, swap space size, OS-specific overcommit behavior etc. How much you can really use depends on how much memory you really have and how the OS is willing to give you. Etc. I think it would be easier to clearly name this function as "address_space_limit" or similar, since that's what it is doing. |
|
Just want to start of by saying thank you for taking the time with this.
I agree that the naming is strong, but I don't see an issue with that. Like you're saying, on Linux (maybe all POSIX systems?), you can never commit more memory than you have virtual address space, regardless of OS, swap space size, and overcommit behavior. So a strong name for a strong upper-bound seems reasonable to me. On POSIX, we only really need an os::address_space_limit(), since it answers both how much memory we can commit and the size of the virtual address space. On Windows, we need an os::allocatable_memory_limit(), which would answer how much memory we can commit, which can be adjusted by the user, unlike the size of the virtual address space, which cannot be artificially limited. So I think we need both, so that both can be called from both OS's. |
| bool os::allocatable_memory_limit(size_t* limit) { | ||
| // On POSIX systems, the amount of allocatable memory is limited by the | ||
| // size of the virtual address space. | ||
| *limit = address_space_limit(); |
There was a problem hiding this comment.
In @toxaart's PR the out parameter is never set if the return value is false. I think the reason why that was done was so that code could be written like this:
size_t value = 0;
if (os::memory_function(value)) {
// Succeeded - do stuff
} else {
// Failed - do other stuff
}
// Use 0 if the function returned an error
use(value);
I think we should strive for unification around that in the various os:: memory functions. I also think that he used & in his patch and this function uses a pointer with the output parameter. Unification around would be good as well.
Maybe it depends on what you need and what question it should answer. Which I don't know. If the question is "what do you think I could allocate at most, were I to try now?"), then yes, allocatableMemory would be good name. It will always be a guess anyway. |
The function os::has_allocatable_memory_limit() is intended to determine whether there is a system-imposed limit on how much memory can be committed, and if so, what that limit is. On POSIX systems, limiting committable memory is typically enforced by restricting the available virtual address space, such as via RLIMIT_AS. As a result, os::has_allocatable_memory_limit() tells us both how much memory can be committed and how much virtual address space is available. On Windows however, os::has_allocatable_memory_limit() always returns true, along with the size of the available virtual address space. This is misleading because it is not possible to limit how much memory can be committed via virtual address space, and also the virtual address space cannot be limited.
ZGC currently uses os::has_allocatable_memory_limit() to check if the virtual address space is limited. To make it clear that the virtual address space cannot be limited on Windows, I propose that we create a new function called os::has_limited_virtual_address_space() which simply returns false on Windows, since the virtual address space cannot be limited there.
As a follow-up, I think it is reasonable to re-visit the implementation of os::has_allocatable_memory_limit() on Windows, since it doesn't follow any user-set limits, apart from how much virtual memory is available. Perhaps looking at limit(s) set by Job Objects could be more fruitful, and would improve the support for native Windows containers (Hyper-V).
Testing:
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26530/head:pull/26530$ git checkout pull/26530Update a local copy of the PR:
$ git checkout pull/26530$ git pull https://git.openjdk.org/jdk.git pull/26530/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26530View PR using the GUI difftool:
$ git pr show -t 26530Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26530.diff
Using Webrev
Link to Webrev Comment