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

[Boris Ulasevich]

On Wed, 16 Nov 2022 22:52:51 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>>> // Roll the stream state back to the marked one.  
>>> void roll_back();
>> 
>> get_position() is not about roll back only. See the DebugInformationRecorder, it serializes offsets into the stream.
>> 
>> 
>> int DebugInformationRecorder::serialize_scope_values(...) {
>>   ...
>>   int result = stream()->position();
>>   ...
>>   return result;
>> }
>> 
>> DebugToken* DebugInformationRecorder::create_scope_values(...) {
>>   ...
>>   return (DebugToken*) (intptr_t) serialize_scope_values(values);
>> }
>> 
>> void PhaseOutput::Process_OopMap_Node(MachNode *mach, int current_offset) {
>>   ...
>>   DebugToken *locvals = C->debug_info()->create_scope_values(locarray);
>>   DebugToken *expvals = C->debug_info()->create_scope_values(exparray);
>>   DebugToken *monvals = C->debug_info()->create_monitor_values(monarray);
>> 
>>   C->debug_info()->describe_scope(
>>     ...
>>     locvals,
>>     expvals,
>>     monvals
>>   );
>>   
>> void DebugInformationRecorder::describe_scope(...
>>                                               DebugToken* locals,
>>                                               DebugToken* expressions,
>>                                               DebugToken* monitors) {
>>   ...
>>   // serialize the locals/expressions/monitors
>>   stream()->write_int((intptr_t) locals);
>>   stream()->write_int((intptr_t) expressions);
>>   stream()->write_int((intptr_t) monitors);
>
> Thank you for the information. It's very helpful.
> 
> I think we should not simulate `CompressedWriteStream`. 
> 
> `DebugInformationRecorder` needs certain operations:
> We write debug info into a stream writer: as grouped multiple data and single data. We need to know where bytes of grouped data begin and end. We need to keep offsets of grouped data in the stream. We need to be able to discard last written grouped data. We need to get the number of used bytes. We don't need to know how data stored in a stream.
> 
> Based on the specification, we need a stream writer to provide operations:
> 
> // Start grouped data.
> // Return a position (byte offset) in the stream where grouped data begins.
> int start_group();
> 
> // Finish grouped data.
> // Return a position (byte offset) in the stream where grouped data ends.
> int finish_group();
> 
> // Revert the stream to the specified position.
> void set_position(int pos);
> 
> // Return the number of bytes stored data uses.
> int data_size() const;
> 
> 
> With them we don't have a function which in one implementation is const but in another implementation is with side effects. IMHO, at some point later side effects will cause bugs. 
> 
> Possible implementations:
> 
> int start_group() {
>   complete_current_byte(); // this is renamed align()
>   return _position;
> }
> 
> int finish_group() {
>   complete_current_byte();
>   return _position;
> }
> 
> int data_size() const {
>   if (_position == 0 && _bit_position == 0)
>     return 0;
> 
>  int used_bytes = _position;
>  if (_bit_position != 0)
>    ++used_bytes;
>  return used_bytes;
> }
> 
> 
> In `DebugInformationRecorder` we will need to replace `position()` with `start_group()` and add `finish_group()`.
> We will need to change `int DebugInformationRecorder::find_sharable_decode_offset(int stream_offset)` to
> `int DebugInformationRecorder::find_sharable_decode_offset(int data_begin_offset, int data_end_offset)`.
> 
> If we want `DebugInformationRecorder` to use `CompressedWriteStream` we can use an adapter:
> 
> class CompressedWriteStreamAdapter: public CompressedWriteStream {
> public:
> ...
> int start_group() {
>   return position();
> }
> 
> int finish_group() {
>   return position();
> }
> 
> int data_size() const {
>   return position();
> }
>  
> };

Sorry for not being online for a while :)
As you proposed, I refactored the CompressedSparseDataWriteStream interface.
I removed position() and set_position(int) methods. I added start_scope() and roll_back(int) instead.
Plus I have added is_empty() method (without side effect) to avoid unintentional align from assert.
Plus I added data_size() method (without side effect) - it is used to check the amount of data to copy.
Please check if it is Ok now.
Thanks.

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

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