On Wed, 16 Aug 2023 23:56:58 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Fix MaxElementPrintSize and casts. Also fixed miscellaneous -Wconversion
>> warnings in runtime code. This is the last one I'm going to do for runtime
>> for a while.
>> Tested with tier1-4.
>
> 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.
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);
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.
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.
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.
src/hotspot/share/utilities/elfFile.hpp line 486:
> 484: DwarfFile* _dwarf_file;
> 485: MarkedDwarfFileReader _reader;
> 486: uintptr_t _section_start_address;
This seems suspicious - is this a 32-bit value or 64-bit?
src/hotspot/share/utilities/elfFile.hpp line 851:
> 849: void reset_fields();
> 850: // Defined in section 6.2.5.1 of the DWARF spec 4.
> add_to_address_register() must always be executed before set_index_register.
> 851: void add_to_address_register(uint8_t operation_advance, const
> LineNumberProgramHeader& header);
Not clear how you decided/determined that `operation_advance` should always be
8-bit?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296554448
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296555979
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296558300
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296559830
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296569150
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296594924
PR Review Comment: https://git.openjdk.org/jdk/pull/15233#discussion_r1296595284
