RFR: 8343700: ceil_log2 should not loop endlessly [v5]

[Kim Barrett]

On Fri, 15 Nov 2024 18:06:06 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi all, 
>> 
>> This PR addresses [8343700](https://bugs.openjdk.org/browse/JDK-8343700) where ceil_log2 looped endlessly if the input value has the highest bit set. 
>> 
>> I also dealt with the case where we try to find ceil_log2(1) which would've returned 1 as opposed to 0. 
>> 
>> Cheers, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixing assert

Changes requested by kbarrett (Reviewer).

src/hotspot/share/classfile/dictionary.cpp line 59:

> 57:   : _number_of_entries(0), _loader_data(loader_data) {
> 58: 
> 59:   size_t start_size_log_2 = MAX2((size_t)ceil_log2(table_size), (size_t)2); // 2 is minimum size even though some dictionaries only have one entry

Rather than adding a cast, remove the other (probably changing literal to `2u`).  The implicit conversion
of uint => size_t at the end is not a problem.

src/hotspot/share/utilities/powerOfTwo.hpp line 124:

> 122: 
> 123: // Find log2 value greater than this input
> 124: template <typename T, typename U = typename std::make_unsigned<T>::type, ENABLE_IF(std::is_integral<T>::value)>

U doesn't need to be a template parameter.  Make it a function-local type alias.

src/hotspot/share/utilities/powerOfTwo.hpp line 125:

> 123: // Find log2 value greater than this input
> 124: template <typename T, typename U = typename std::make_unsigned<T>::type, ENABLE_IF(std::is_integral<T>::value)>
> 125: inline int ceil_log2(T value) {

Pre-existing: I think this should have been called `log2i_ceil`, with a description something like:

// Ceiling of log2 of a positive, integral value, i.e., smallest i such that value <= 2^i.

And move it near the other log2i variants.

This could be done as a followup, to keep this PR focused on the issue at hand.

src/hotspot/share/utilities/powerOfTwo.hpp line 128:

> 126:   assert(value > 0, "Invalid value");
> 127:   U unsigned_value = static_cast<U>(value);
> 128:   int max_bits = sizeof(U) * 8;

Use BitsPerByte instead of `8`.

src/hotspot/share/utilities/powerOfTwo.hpp line 134:

> 132:       break;
> 133:     }
> 134:   }

But I think this could all be much "simpler"?  I think this works:

template<typename T, ENABLE_IF(std::is_integral<T>::value)>
int ceil_log2(T value) {
  assert(value > T(0), "Invalid value");
  return log2i_graceful(value - 1) + 1;
}

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

PR Review: https://git.openjdk.org/jdk/pull/22074#pullrequestreview-2439858700
PR Review Comment: https://git.openjdk.org/jdk/pull/22074#discussion_r1844616579
PR Review Comment: https://git.openjdk.org/jdk/pull/22074#discussion_r1844651668
PR Review Comment: https://git.openjdk.org/jdk/pull/22074#discussion_r1844651693
PR Review Comment: https://git.openjdk.org/jdk/pull/22074#discussion_r1844650960
PR Review Comment: https://git.openjdk.org/jdk/pull/22074#discussion_r1844680497