On Thu, 17 Aug 2023 00:20:44 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Change size of op_index back.
>
> src/hotspot/share/logging/logOutput.cpp line 69:
>
>> 67:
>> 68: static int tag_cmp(const LogTagType *a, const LogTagType *b) {
>> 69: return primitive_compare(a, b);
>
> This looks very odd given we are dealing with pointers not primitives.
Was it odd before or odd now? What we want to do is compare pointers for a
sort function. This primitive_compare has been used in other places as an
improvement.
> src/hotspot/share/memory/metaspace.cpp line 711:
>
>> 709: // it can get as low as 1:2. It is not a big deal though since ccs
>> is only
>> 710: // reserved and will be committed on demand only.
>> 711: size_t max_ccs_size = (size_t)((double) MaxMetaspaceSize * 0.8);
>
> Really no need for FP arithmetic here - we could just do:
>
> size_t max_ccs_size = 8 * (MaxMetaspaceSize / 10);
That's a good observation.
> src/hotspot/share/services/threadService.hpp line 107:
>
>> 105: static jlong get_peak_thread_count() { return
>> _peak_threads_count->get_value(); }
>> 106: static int get_live_thread_count() { return
>> _atomic_threads_count; }
>> 107: static int get_daemon_thread_count() { return
>> _atomic_daemon_threads_count; }
>
> Given all the other jlong usage in these functions, and that these are used
> for the return value of `jlong get_long_attribute(jmmLongAttribute att)` in
> management.cpp, I think these should stay as jlong returning functions.
If they stay jlong returns (note that these fields are in fact int), then we
need to add casting to all the callers. Casting is worse than returning the
correct types. If someone wants to make these fields jlong someday then they
can propagate the change to the callers. This change corrects the types.
> src/hotspot/share/services/threadStackTracker.cpp line 46:
>
>> 44:
>> 45: int ThreadStackTracker::compare_thread_stack_base(const
>> SimpleThreadStackSite& s1, const SimpleThreadStackSite& s2) {
>> 46: return primitive_compare(s1.base(), s2.base());
>
> Again these are not primitive values, but addresses, so this looks odd to me.
Maybe you don't like the name primitive. We can file an RFE to change the name
to something you'd prefer?
> src/hotspot/share/utilities/elfFile.cpp line 792:
>
>> 790: // We must align to twice the address size.
>> 791: uint8_t alignment = DwarfFile::ADDRESS_SIZE * 2;
>> 792: uint64_t padding = alignment - (_reader.get_position() -
>> _section_start_address) % alignment;
>
> 64-bit seems a waste here. I would just cast the result to uint8_t. The %
> operator ensures it is within range.
Wasting what?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297129576
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297147389
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297133657
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297134164
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1297134606
