RFR: 8330144: Revise os::free_memory() [v2]

[Robert Toyonaga]

On Tue, 9 Jul 2024 18:26:52 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Robert Toyonaga has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Minor cleanup and comments.
>>  - rename to disclaim_memory and update test
>
> Great, thanks @roberttoyonaga. The main work was the analysis work beforehand.
> 
> About naming, I would name the thing "os::disclaim_memory". free_without_uncommit is a mouthful. There is a precedence in the "disclaim" API on AIX, which in a future RFE may be used to implement os::disclaim_memory.

Thank you @tstuefe for the review feedback! I've renamed `free_memory_without_uncommit` to `disclaim_memory` and removed the `committed_in_range` check so the unit test can be more reliable.

> src/hotspot/os/windows/os_windows.cpp line 3896:
> 
>> 3894: 
>> 3895: void os::pd_realign_memory(char *addr, size_t bytes, size_t alignment_hint) { }
>> 3896: void os::pd_disclaim_memory(char *addr, size_t bytes) { }
> 
> Give us a little comment about what this API does?
> 
> "Hints to the OS that the memory is not needed anymore and can be reclaimed by the OS; will destroy memory content; it will be re-aquired on touch, no explicit committing needed"
> 
> Something like that

Ok I've added a comment with a description. 

Is it good practice to add these types of descriptions in the shared code header files (os.hpp), in the platform dependent code (os_linux.hpp), or both? I see some examples of all 3 cases, but I'm wondering if there's a best practice.

> test/hotspot/gtest/runtime/test_os.cpp line 1002:
> 
>> 1000:   size_t committed_size;
>> 1001:   address committed_start;
>> 1002:   ASSERT_FALSE(os::committed_in_range((address) base, size, committed_start, committed_size));
> 
> Is there a chance of this generating false positives? Do we know if the madvise effect immediate or delayed?

That's a good point. Based on the linux [docs](https://man7.org/linux/man-pages/man2/madvise.2.html) it might not happen immediately, causing the test to be flaky. I'll remove the `committed_in_range` check.  I suppose we could poll with a timeout, but there's still no guarantee the pages actually get freed in a timely manner.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20080#issuecomment-2220609342
PR Review Comment: https://git.openjdk.org/jdk/pull/20080#discussion_r1672735056
PR Review Comment: https://git.openjdk.org/jdk/pull/20080#discussion_r1672249380