On Mon, 10 Jul 2023 10:37:18 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Thomas Stuefe has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Add test with 1ms trim interval >> - No need for atomics > > src/hotspot/share/runtime/globals.hpp line 1990: > >> 1988: "If TrimNativeHeap is enabled: interval, in ms, at which " >> \ >> 1989: "the to trim the native heap.") >> \ >> 1990: range(1, UINT_MAX) >> \ > > I have a suggestion that simplifies UX, I think. In other cases where we do > these kinds of intervals, we just use `0` as the "off" value. See for example > `AsyncDeflationInterval`, `GuaranteedSafepointInterval`. This would allow > users to supply one option only. > > We would then need to decide if we turn this thing on by default (probably > with large interval), or we default to `0` for "off". I would prefer to go > for `0`. I understand that would force users to decide on proper trim > interval when enabling, but I think that's a feature, not a bug. We would not > have to go into discussions if the 60 second default is good enough or not. > > Something like this: > > > product(uintx, TrimNativeHeapInterval, 0, EXPERIMENTAL, \ > “Attempt to trim the native heap every so many milliseconds, “ \ > “if platform supports it. Lower values provide better footprint “ \ > “under native allocation spikes, while higher values come with “ \ > “less overhead. Use 0 to disable trimming.” \ Makes sense. I was afraid of people shooting themselves in the foot if not presented with a sensible default, but you are right we have precedence with similar switches. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1258185263
