RFR: JDK-8304421: Introduce malloc size feedback [v3]

[David Holmes]

On Fri, 17 Mar 2023 19:55:19 GMT, Justin King <jcking at openjdk.org> wrote:

>> Hi,
>> 
>> some initial thoughts.
>> 
>> I find using excess memory in this manner quite scary. It depends on the implementation correctly not using what it reports. Eg glibc malloc_usable_size comes with a warning label of "this is bad practice". Practically speaking we now treat new paths, which are UB, and may trigger paths in libcs that are not well tested. The gains are probably not large enough to justify this risk.
>> 
>> We could probably just resize arena chunks correctly to page sizes or power of two sizes; much simpler that way.
>> 
>> There could be some merit in collecting this number and reporting it. But probably not for the end user. Since the real cost of malloc comprises many parts, and overhead of outstanding allocations is just a part of that. Retention size and size of side structures are often more significant. So we would report just an arbitrary part of a larger, unknown whole. One more number to confuse folks, but ultimately not that helpful. Note that we already report libc overhead as a whole.
>> 
>> There could be a use case for hotspot devs to highlight particularly "unhappy" allocation sizes. I fear that the number of actionable call sites would be small, though, since the vast majority of allocations come from new and depend on object sizes. That leaves us with buffer allocations, which are usually already power two-sized.
>> 
>> Lunping refactoring together with functional changes does not help. Makes it harder to spot the functional changes. Refactoring can be done in separate RFEs if necessary. Since refactoring is often a matter of opinion, it's also fairer to discuss them separately from functional changes.
>> 
>> More thoughts:
>> 
>> - It is unspecified what the effect should be on NMT allocation guards. Would the guard be added to the user-requested size or to the actual size? The former would render this improvement pointless; the latter makes me personally unhappy since we lose our ability to catch off by one overwrites. This needs to be specified, and we need regression tests for this.
>> - Speaking of it, there are no tests.
>> - The duplication of code in NMTPreInit is just plain bad, as is the duplication of os::free in os.cpp. That said, I don't get the point of free_sized. Why is this needed?
>> - Platform-dependent changes should go into their respective platform folders
>> - Does muslc support malloc_usable_size? If not, pls make your code conditional for glibc (see e.g. the trim coding in the linux branch)
>> 
>> Cheers, Thomas
>
>> Hi,
>> 
>> some initial thoughts.
>> 
>> I find using excess memory in this manner quite scary. It depends on the implementation correctly not using what it reports. Eg glibc malloc_usable_size comes with a warning label of "this is bad practice". Practically speaking we now treat new paths, which are UB, and may trigger paths in libcs that are not well tested. The gains are probably not large enough to justify this risk.
> 
> Any implementation returning an invalid size from `malloc_usable_size` is a broken implementation and has no business existing or being supported.
> 
> glibc `malloc_usable_size` man pages have no such warning, they do have the following quote `Although the excess bytes can be overwritten by the application without ill effects, this is not good programming practice: the number of excess bytes in an allocation depends on the underlying implementation.` The warning is there so that callers do not rely on any specific size being returned other than it being at least the original request size. Two different allocations for the same size could have different excess bytes, so make no assumptions.
> 
> Size feedback is likely being added to operator new in the next version of C++ where a tuple will be returned containing the pointer and size, C has yet to follow suite but likely will in the future. At which point that could be used.
> 
>> 
>> We could probably just resize arena chunks correctly to page sizes or power of two sizes; much simpler that way.
> 
> That would not work and would still have waste. It is heavily dependent on the underlying `malloc()` implementation which is free to change. For example if you request exactly a page from glibc `malloc()` your're likely going to get more than that. Whether power-of-two will work also depends on the implementation.
> 
>> 
>> Lunping refactoring together with functional changes does not help. Makes it harder to spot the functional changes. Refactoring can be done in separate RFEs if necessary. Since refactoring is often a matter of opinion, it's also fairer to discuss them separately from functional changes.
>> 
>> More thoughts:
>> 
>> * It is unspecified what the effect should be on NMT allocation guards. Would the guard be added to the user-requested size or to the actual size? The former would render this improvement pointless; the latter makes me personally unhappy since we lose our ability to catch off by one overwrites. This needs to be specified, and we need regression tests for this.
> 
> Its the latter. The size is now the actual size. The point of size feedback is "allocate at least N bytes, then tell me how much you actually allocated".  I'll work on adding some tests.
> 
>> * Speaking of it, there are no tests.
>> * The duplication of code in NMTPreInit is just plain bad, as is the duplication of os::free in os.cpp. That said, I don't get the point of free_sized. Why is this needed?
> 
> free_sized is in C23 and is an optimization for many `malloc()` implementations that allows free to be faster. It's the same rationale as having sized delete in C++. If the size is known, the implementation can skip having to determine the size in order to do the free.
> 
> That said I could probably try to combine them into a single implementation under the hood.
> 
>> * Platform-dependent changes should go into their respective platform folders
>> * Does muslc support malloc_usable_size? If not, pls make your code conditional for glibc (see e.g. the trim coding in the linux branch)
> 
> Yes it supports it.

@jcking so is the basic idea that if our own internal allocation mechanism wants to allocate a chunk of size M, and what malloc actually gives back is a chunk of size N (N>M), then we just mark the size of our chunk as N instead of M, thus taking full advantage of what malloc returns?

How often do we get back a significant extra amount of memory? I can understand the returned about being rounded up based on page size, or alignment constraints, but I'd expect those to be minimal (else surely it is a very poor malloc implementation!). And I'd expect our own allocators to use similar rounding approaches themselves anyway.

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

PR: https://git.openjdk.org/jdk/pull/13081