RFR: 8293170: Improve encoding of the debuginfo nmethod section [v6]

[Boris Ulasevich]

On Sun, 30 Oct 2022 17:27:23 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>> Boris Ulasevich has refreshed the contents of this pull request, and previous commits have been removed. Incremental views are not available.
>
> src/hotspot/share/code/compressedStream.cpp line 117:
> 
>> 115: 
>> 116: 
>> 117: bool CompressedSparseDataReadStream::read_zero() {
> 
> If the last value written to a stream was 0, a reader would not know this is one 0 or eight 0s. Is there a guarantee that the number of reads will the same as the number of writes?

Reader knows what he reads. We have no marks in a raw data.

> src/hotspot/share/code/compressedStream.cpp line 145:
> 
>> 143:   //
>> 144:   //       value | byte0    | byte1    | byte2    | byte3    | byte4
>> 145:   //  -----------+----------+----------+----------+----------+----------
> 
> in each byte bit 6 indicates whether it is the last byte in the sequence

such comment would be misleading

> src/hotspot/share/code/compressedStream.cpp line 171:
> 
>> 169:   if (_bit_pos == 0) {
>> 170:     return _position;
>> 171:   }
> 
> We can rewrite the function not to use `_curr_byte`. We work directly on `_buffer` and rename `_bit_pos` into `_used_bits`:
> 
>   if (_used_bits == 8) {
>     _buffer[++_position] = 0;
>     _used_bits = 1;     
>   } else {
>     _buffer[_position] >>= 1;
>     ++_used_bits;
>   }

thanks for the idea not to use _curr_byte
_bit_pos is used in both Read and Write streams. I think proper name is _bit_position.

> src/hotspot/share/code/compressedStream.cpp line 175:
> 
>> 173:   write(_curr_byte << (8 - _bit_pos));
>> 174:   _curr_byte = 0;
>> 175:   _bit_pos = 0;
> 
> Let's extract this and call it `flush()`.

Ok. Now it is align() as _curr_byte is evicted

> src/hotspot/share/code/compressedStream.cpp line 189:
> 
>> 187: 
>> 188: void CompressedSparseDataWriteStream::write_byte_impl(uint8_t b) {
>> 189:   write((_curr_byte << (8 - _bit_pos)) | (b >> _bit_pos));
> 
> _buffer[_position] |= (b >> _used_bits);
>   _buffer[++_position] = (b << (8 - _used_bits));

OK

> I don't see what causes it to be written.
It was a flush side effect caused by get_position in the end of the buffer filling.
Anyway, it is rewritten now. Thanks!

> src/hotspot/share/code/compressedStream.hpp line 115:
> 
>> 113: };
>> 114: 
>> 115: class CompressedBitStream : public ResourceObj {
> 
> Maybe it is better `CompressedSparseData`?

OK

> src/hotspot/share/code/compressedStream.hpp line 185:
> 
>> 183:   int position(); // method have a side effect: the current byte becomes aligned
>> 184:   void set_position(int pos) {
>> 185:     position();
> 
> `position()` -> `flush()`

OK

> src/hotspot/share/code/compressedStream.hpp line 197:
> 
>> 195:       grow();
>> 196:     }
>> 197:     _buffer[_position++] = b;
> 
> I think `pos` must be `<= position()`. Should we check this?

In fact, it is used as rollback. Though I do not want to limit functionality to this usage only.
Let me add `assert(_position < _size, "set_position is only used for rollback");` check here

> src/hotspot/share/code/debugInfo.hpp line 298:
> 
>> 296: // debugging information. Used by ScopeDesc.
>> 297: 
>> 298: class DebugInfoReadStream : public CompressedSparseDataReadStream {
> 
> I don't think `DebugInfoReadStream`/`DebugInfoWriteStream` need public inheritance. The relation  is more like composition.
> I would have implemented them like:
> 
> class DebugInfoReadStream : private CompressedSparseDataReadStream {
> public:
>    // we are using only needed functions from CompressedSparseDataReadStream.
>     using CompressedSparseDataReadStream::buffer();
>     using CompressedSparseDataReadStream::read_int();
>     using ...
> };
> 
> Or
> 
> template <typename DataReadStream> class DebugInfoReadStream {
> public:
>     // define only needed functions which use a minimum number of functions from DataReadStream
> };
> 
> 
> I prefer the templates because we can easily switch between different implementations of `DataReadStream`/DataWriteStream` without doing this kind of modifications.

@No templates please! :)
For me, the following change is counterproductive as well.

- class DebugInfoReadStream : private CompressedSparseDataReadStream {
+ class DebugInfoReadStream : private CompressedSparseDataReadStream {
+  public:
+   using CompressedSparseDataReadStream::read_int;
+   using CompressedSparseDataReadStream::read_signed_int;
+   using CompressedSparseDataReadStream::read_double;
+   using CompressedSparseDataReadStream::read_long;
+   using CompressedSparseDataReadStream::read_bool;
+   using CompressedSparseDataReadStream::CompressedSparseDataReadStream;

> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CompressedSparseDataReadStream.java line 28:
> 
>> 26: import sun.jvm.hotspot.debugger.*;
>> 27: 
>> 28: public class CompressedSparseDataReadStream extends CompressedReadStream {
> 
> This needs to be aligned with C++ code.
> Can we test the code?

I have added a simple jtreg test. thanks!

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

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