RFR: 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale [v4]

[Thomas Stuefe]

On Tue, 10 Jan 2023 11:27:43 GMT, Afshin Zafari <duke at openjdk.org> wrote:

>> ### Description
>> MemReporterBase::diff_in_current_scale is defined as follows:
>> 
>>   inline long diff_in_current_scale(size_t s1, size_t s2) const {
>>     long amount = (long)(s1 - s2);
>>     long scale = (long)_scale;
>>     amount = (amount > 0) ? (amount + scale / 2) : (amount - scale / 2);
>>     return amount / scale;
>>   }
>> 
>> Long and size_t can have different sizes: 4 bytes and 8 bytes (LLP64). The result of 's1 - s2' might not fit into long. It might not fit into int64_t. For example: s1 is SIZE_MAX and s2 is SIZE_MAX-MAX_INT64-1.
>> 
>> Size_t should be used instead of long. Assertions must be added to check:
>> s1 >= s2 and (amount - scale/2) >= 0 and (amount + scale/2) <= SIZE_MAX.
>> 
>> ### Patch
>> `long` is replaced by `size_t`. Comparison to 0 is implemented accordingly since size_t is always >= 0.
>> Since s1 can be less than s2 in some invocations of this method, no assert is written for `(s1 >= s2)` case.
>> 
>> ### Test
>> local: runtime/NMT/Jcmd*
>> mach5: tier1
>
> Afshin Zafari has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
>  - Merge branch 'master' into _8281213
>  - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
>  - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
>  - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
>  - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
>  - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale

@afshin-zafari, thanks a lot for taking my suggestion about using in64_t. Some minor nits remain, mainly the diff function can be simplified.

Thanks, Thomas

src/hotspot/share/services/memReporter.hpp line 74:

> 72: 
> 73:   // Convert diff amount in bytes to current reporting scale
> 74:   int64_t diff_in_current_scale(size_t s1, size_t s2) const {

- Can you add a little comment please about why you use int64_t? E.g. something like:

// We use int64_t instead of ssize_t because on 32-bit it allows us to express deltas larger than 2 gb. On 64-bit we
// never expect memory sizes larger than INT64_MAX".

- I would assert the input values before doing the subtraction, then don't need the is_negative part. You could shrink the function to something like this:
 ```
  int64_t diff_in_current_scale(size_t s1, size_t s2) const {
    LP64_ONLY(assert(s1 < INT64_MAX && s2 < INT64_MAX, "size overflow");)
    return ((int64_t)s1 - (int64_t)s2)/scale;
 }

The LP64_ONLY could be needed because on 32-bit the compiler may complain about the compare being always true.

Am I overlooking something?

src/hotspot/share/services/memReporter.hpp line 88:

> 86:     size_t scaled = (amount / _scale);
> 87:     if ((amount % _scale) > (_scale - 1)/2) {
> 88:       scaled += 1;

This correction is not needed I think. 

The old code did not do this, so it may cause NMT numbers to seemingly go up after this patch, suggesting we use more memory. And it's wrong either way.

A better way to deal with this (as a future improvement) would be to print 2-3 significant digits, e.g. "2.33GB". Like we do in the `proper_unit_for_byte_size()` and friends, but with a fixed scale.

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

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