RFR: 8292758: put support for UNSIGNED5 format into its own header file [v3]

[Coleen Phillimore]

On Sat, 3 Sep 2022 23:46:41 GMT, John R Rose <jrose at openjdk.org> wrote:

>> Refactor code from inside of CompressedStream into its own unit.
>> 
>> This code is likely to be used in future refactorings, such as JDK-8292818 (replace 96-bit representation for field metadata with variable-sized streams).
>> 
>> Add gtests.
>
> John R Rose has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add more support for SA including vmStructs; tweak some debug code

Thank you for adding the SA and debug.cpp for debugging this.

src/hotspot/share/code/compressedStream.cpp line 85:

> 83:   const int min_expansion = UNSIGNED5::MAX_LENGTH;
> 84:   if (nsize < min_expansion*2)
> 85:     nsize = min_expansion*2;

I think our coding style guide recommends having {} around even one line code inside if and else statements. I usually ignore this if it's on the same line, but on a separate line, it seems worth pointing out.

src/hotspot/share/utilities/unsigned5.cpp line 35:

> 33: template u4 UNSIGNED5::read_uint(u_char* array, int& offset_rw, int limit, AGS);
> 34: template void UNSIGNED5::write_uint(uint32_t value, u_char* array, int& offset_rw, int limit, AGS);
> 35: template int UNSIGNED5::check_length(u_char* array, int offset, int limit, AGS);

I don't know why you'd need these explicit instantiations.  The entire templates are in the hpp files so the caller will instantiate them.

src/hotspot/share/utilities/unsigned5.hpp line 174:

> 172: 
> 173:   // returns the encoded byte length of an unsigned 32-bit int
> 174:   static constexpr int encoded_length(uint32_t value) {

Should all of these functions be private?

src/hotspot/share/utilities/unsigned5.hpp line 312:

> 310:       : _array(const_cast<ARR&>(array)), _limit_ptr(NULL)
> 311:         // note:  if _limit_ptr is NULL, the ARR& is never reassigned
> 312:     { limit_init(); _position = 0; }

indent needed.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/Unsigned5.java line 59:

> 57:       hval = db.lookupIntConstant("UNSIGNED5::H").intValue();
> 58:       lval = db.lookupIntConstant("UNSIGNED5::L").intValue();
> 59:       xval = db.lookupIntConstant("UNSIGNED5::X").intValue();

These constants aren't likely to change ever, are they?  I don't see why we'd need the SA to get these constants from the VM.  It could just hard-code them like you have here.  Then UNSIGNED5 doesn't have to make VMStructs a friend class.
Also, there isn't a compatibility issue with older SA - it might work sometimes but the SA version should match the JVM that is being debugged.

test/hotspot/gtest/utilities/test_unsigned5.cpp line 31:

> 29: // T.I.L.
> 30: // $ sh ./configure ... --with-gtest=<...>/googletest ...
> 31: // $ make exploded-test TEST=gtest:unsigned5

What does T.I.L mean?  This isn't the only way to run this test, so don't include this.

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

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