RFR: JDK-8293313: NMT: Rework MallocLimit [v4]

[Thomas Stuefe]

On Wed, 25 Jan 2023 17:41:41 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>>> I would prefer to see `suppress_limit_handling()` checked inside `total_limit_reached()`, same for `category_limit_reached()`. This way we would force those APIs to always obey the suppression without having to bother to add `suppress_limit_handling()`, they could call `VMError::is_error_reported()` directly.
>> 
>> Note that `total_limit_reached()` and `category_limit_reached()` are private APIs. 
>> 
>> If we move `suppress_limit_handling()` into `total_limit_reached()`, `total_limit_reached()` would need to report it back to `MallocMemorySummary::check_exceeds_limit` since its processed there too, it affects the return code. So `total_limit_reached()` cannot be void anymore, and gets somewhat more complex. I prefer it this way, tbh.
>
> I'm not following.
> 
> If we get rid of `suppress_limit_handling()` SPI, and do:
> 
> 
> bool MallocMemorySummary::total_limit_reached(size_t s, size_t so_far, const malloclimit* limit) {
>   if (VMError::is_error_reported()) {
>     return false;
>   }
> 
> #define FORMATTED \
> "MallocLimit: reached global limit (triggering allocation size: " PROPERFMT ", allocated so far: " PROPERFMT ", limit: " PROPERFMT ") ", \
> PROPERFMTARGS(s), PROPERFMTARGS(so_far), PROPERFMTARGS(limit->sz)
> 
>   if (limit->mode == MallocLimitMode::trigger_fatal) {
>     fatal(FORMATTED);
>   } else {
>     log_warning(nmt)(FORMATTED);
>   }
> 
> #undef FORMATTED
> 
>   return true;
> }
> 
> bool MallocMemorySummary::category_limit_reached(MEMFLAGS f, size_t s, size_t so_far, const malloclimit* limit) {
>   if (VMError::is_error_reported()) {
>     return false;
>   }
> 
> #define FORMATTED \
>   "MallocLimit: reached category "%s" limit (triggering allocation size: " PROPERFMT ", allocated so far: " PROPERFMT ", limit: " PROPERFMT ") ", \
>   NMTUtil::flag_to_enum_name(f), PROPERFMTARGS(s), PROPERFMTARGS(so_far), PROPERFMTARGS(limit->sz)
> 
>   if (limit->mode == MallocLimitMode::trigger_fatal) {
>     fatal(FORMATTED);
>   } else {
>     log_warning(nmt)(FORMATTED);
>   }
> 
> #undef FORMATTED
> 
>   return true;
> }
> 
> 
> 
> then we can simply have:
> 
> 
> // Returns true if allocating s bytes on f would trigger either global or the category limit
> inline bool MallocMemorySummary::check_exceeds_limit(size_t s, MEMFLAGS f) {
> 
>   // Note: checks are ordered to have as little impact as possible on the standard code path,
>   // when MallocLimit is unset, resp. it is set but we have reached no limit yet.
>   // Somewhat expensive are:
>   // - as_snapshot()->total(), total malloc load (requires iteration over arena types)
>   // - VMError::is_error_reported() is a load from a volatile.
>   if (MallocLimitHandler::have_limit()) {
> 
>     // Global Limit ?
>     const malloclimit* l = MallocLimitHandler::global_limit();
>     if (l->sz > 0) {
>       size_t so_far = as_snapshot()->total();
>       if ((so_far + s) > l->sz) { // hit the limit
>         if (total_limit_reached(s, so_far, l)) {
>           return true;
>         }
>       }
>     } else {
>       // Category Limit ?
>       l = MallocLimitHandler::category_limit(f);
>       if (l->sz > 0) {
>         const MallocMemory* mm = as_snapshot()->by_type(f);
>         size_t so_far = mm->malloc_size() + mm->arena_size();
>         if ((so_far + s) > l->sz) {
>           if (category_limit_reached(f, s, so_far, l)) {
>             return true;
>           }
>         }
>       }
>     }
>   }
> 
>   return false;
> }
> 
> 
> 
> Doesn't that work?

Yes, you are right, its better. Can remove vmError.hpp dependency from the header too.

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

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