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

[Evgeny Astigeevich]

On Tue, 15 Nov 2022 06:57:09 GMT, Boris Ulasevich <bulasevich at openjdk.org> wrote:

>> 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

IMHO, the functionality of changing the position after the current position won't be correct. It breaks the invariant of a stream: all data written up to the current position must be correct.
Anyway, at least we have an assertion.

>> 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;

Ok. I accept your arguments.

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

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