On Thu, 6 Jul 2023 22:32:41 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Boris Ulasevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressing review comments: super call, clarifying, and adding tests
>
> src/hotspot/share/code/compressedStream.hpp line 118:
>
>> 116: bool handle_zero(juint value) {
>> 117: if (value == 0) {
>> 118: _zero_count = (_zero_count == 0xFF) ? 0 : _zero_count;
>
> The case of `_zero_count` overflow is not clear. Apparently, I'm missing
> something here.
> Current code is just clearing the previously counted `_zero_count`.
> I'd expect some action like storing the current number of zeros or advancing
> the `_position`.
> Do you have a test for this?
Good question. It works like this. On each write(), values are stored in the
array, either normally or as a chain optimization (two bytes: zero byte and a
number of zeros). When we get another zero value, the chain is updated. When
the chain is full, we exit this function and the value is written by
UNSIGNED5::write_uint_grow().
To make things clear, I have updated this code:
- _zero_count = (_zero_count == 0xFF) ? 0 : _zero_count;
+ if (_zero_count == 0xFF) { // biggest zero chain length is 255
+ _zero_count = 1;
+ // for now, write it as an ordinary value (UNSINGED5 encodes zero int
as a single byte)
+ // the new zero sequence is started if there are more than two zero
values in a raw
+ return false;
+ }
Plus, I have added gtests to check this.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12387#discussion_r1270543845
