RFR: JDK-8266503: [ul] Make Decorations safely copy-able and reduce their size

[Xin Liu]

On Tue, 4 May 2021 15:05:43 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> In Universal Logging, class LogDecorations keeps resolved decorations as well as a lookup table of pointers to the start of each resolved decoration, by decorator. This is dangerous, since it makes object copy non-trivial (the pointers would need to be relocated). It is also wasteful since we spend 8 bytes per pointer per index.
> 
> Better would be to use a numerical index array of offset values, which could be safely copied. 
> 
> And since the resolve buffer is 256 char, we can easily make this index an 8-bit value, which reduces the size of a LogDecorations object down to 280 bytes (from 368). Size matters especially in the context of JDK-8229517.
> 
> The patch also adds a gtest, which tests that decorations are safely copy-able and that decorator resolution works as expected.
> 
> Testing:
> - manually jtreg runtime/logging
> - manually gtests
> - Will run more tests tonight

I think the idea lookup table of offsets is great.  Also compilers can make use of SIMD instructions to generate more efficient copying code. 

But I think we still need a custom copy constructor. Here is why. 
1. the internal representation of decorations is compact. LogDecorations::create_decorations() puts all non-trivial decorations together. 
2. the average use of _decorations_buffer is just 1/10 of its capacity or DecorationsBufferSize.
I dump all decorations : java -Xlog:async -Xlog:all=trace:file=hotspot.log --version 
By default, its decorators is LogDecorators::DefaultDecoratorsMask =  (1 << uptime_decorator) | (1 << level_decorator) | (1 << tags_decorator); 

The internal representation of  _decorations_buffer most likely looks as follows. 

![Screen Shot 2021-05-04 at 1 11 12 PM](https://user-images.githubusercontent.com/2386768/117063851-424b2100-acda-11eb-8b72-b649a79b5152.png)

uptime | lvl(optimized out) | tags
2.582s '\0' '\0' codestrings '\0' [followed by garbage bytes]

I think it's a good idea to copy what we need.  My copy constructor actually just copys 20bytes instead of 256 bytes. 

LogDecorations::LogDecorations(const LogDecorations& o)
    : _level(o._level), _tagset(o._tagset) {
  size_t sz = 0;
  for (int i = 0; i < LogDecorators::Count; ++i) {
    if (o._decoration_offset[i] != NULL) {
      sz += strlen(o._decoration_offset[i]) + 1;
    }
  }

  // safe to call with sz = 0
  memcpy(_decorations_buffer, o._decorations_buffer, sz);
  for (int i = 0; i < LogDecorators::Count; ++i) {
    if (o._decoration_offset[i] != NULL) {
      _decoration_offset[i] = (o._decoration_offset[i] - o._decorations_buffer) + _decorations_buffer;
    } else {
      _decoration_offset[i] = NULL;
    }
  }
}


Can we that resolve copy ctor of logDecoration in this JBS?  I think we can split this from that PR, which has been quite big. 
I can try your approach and see how much performance hit  on average.

src/hotspot/share/logging/logDecorations.hpp line 43:

> 41:   typedef uint8_t offset_t;
> 42:   static const offset_t invalid_offset = DecorationsBufferSize - 1;
> 43:   offset_t _decoration_offset[LogDecorators::Count];

This is less flexible than before.  the type uint8_t limits the size of _decorations_buffer is lesser than 256.  it's impossible to stretch _decorations_buffer.   Further, we should avoid implicit assumption between DecorationsBufferSize and uint8_t.

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

PR: https://git.openjdk.java.net/jdk/pull/3855