RFR: 8329433: Reduce nmethod header size [v3]

[Vladimir Kozlov]

On Tue, 16 Apr 2024 06:13:59 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Union fields which usages do not overlap
>
> src/hotspot/share/code/nmethod.cpp line 1235:
> 
>> 1233:     int skipped_insts_size   = code_buffer->total_skipped_instructions_size();
>> 1234: #ifdef ASSERT
>> 1235:     assert(((skipped_insts_size >> 16) == 0), "size is bigger than 64Kb: %d", skipped_insts_size);
> 
> Suggestion:
> 
> 
> I think it's simpler just to use checked_cast below.

Agree

> src/hotspot/share/code/nmethod.cpp line 1240:
> 
>> 1238:     int consts_offset = code_buffer->total_offset_of(code_buffer->consts());
>> 1239:     assert(consts_offset == 0, "const_offset: %d", consts_offset);
>> 1240: #endif
> 
> Suggestion:

I assume you are suggesting to remove `#ifdef ASSERT`.
Done.

> src/hotspot/share/code/nmethod.cpp line 1241:
> 
>> 1239:     assert(consts_offset == 0, "const_offset: %d", consts_offset);
>> 1240: #endif
>> 1241:     _skipped_instructions_size = (uint16_t)skipped_insts_size;
> 
> Suggestion:
> 
>     _skipped_instructions_size = checked_cast<uint16_t>(code_buffer->total_skipped_instructions_size());

Done.

> src/hotspot/share/code/nmethod.cpp line 1441:
> 
>> 1439:     int deps_size     = align_up((int)dependencies->size_in_bytes(), oopSize);
>> 1440:     int sum_size      = oops_size + metadata_size + deps_size;
>> 1441:     assert((sum_size >> 16) == 0, "data size is bigger than 64Kb: %d", sum_size);
> 
> I suggest using checked_cast for the assignment below, rather than special-purpose checks here.

Okay. But I will put above code under `#ifdef ASSERT` then.

> src/hotspot/share/code/nmethod.cpp line 1445:
> 
>> 1443:     _metadata_offset      = (uint16_t)oops_size;
>> 1444:     _dependencies_offset  = _metadata_offset      + (uint16_t)metadata_size;
>> 1445:     _scopes_pcs_offset    = _dependencies_offset  + (uint16_t)deps_size;
> 
> Use checked_cast instead of raw casts.

okay

> src/hotspot/share/code/nmethod.cpp line 1459:
> 
>> 1457:     assert((data_offset() + data_end_offset) <= nmethod_size, "wrong nmethod's size: %d < %d", nmethod_size, (data_offset() + data_end_offset));
>> 1458: 
>> 1459:     _entry_offset          = (uint16_t)offsets->value(CodeOffsets::Entry);
> 
> Use checked_cast.

done

> src/hotspot/share/memory/heap.hpp line 58:
> 
>> 56:   void set_length(size_t length)                 {
>> 57:     LP64_ONLY( assert(((length >> 32) == 0), "sanity"); )
>> 58:     _header._length = (uint32_t)length;
> 
> Suggestion:
> 
>     _header._length = checked_cast<uint32_t>length;

Done.

> src/hotspot/share/memory/heap.hpp line 63:
> 
>> 61:   // Accessors
>> 62:   void* allocated_space() const                  { return (void*)(this + 1); }
>> 63:   size_t length() const                          { return (size_t)_header._length; }
> 
> This cast looks unnecessary.

Agree.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567619512
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567620520
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567620735
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567627565
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567636013
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567638682
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567644204
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567645140