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

[Thomas Stuefe]

On Thu, 6 May 2021 09:40:42 GMT, Volker Simonis <simonis at openjdk.org> wrote:

> Hi Thomas,
> 
> your change looks good and I like it :)

:-)

> 
> Am I right that you've changed the 256-char-for-all-decorators limit to a 255-char limit for a single decorator (which is obviously much better)?

yes - and even that can be improved by using a fileStream directly, not raw jio_fprintf.

> 
> Find my other comments inline.
> 
> Thank you and best regards,
> Volker

Thanks!

> You have no error handling here. `os::iso8601_time()` can return `NULL` and `print_raw()` will call `strlen(NULL)` which will crash. Do something like:
> 
> ```
> char *result = os::iso8601_time(_millis, buf, sizeof(buf), false);
> st->print_raw(result == NULL ? "-1" : result);
> ```

Good catch, I missed that. Note that I think os::iso8601_time() could be made nicer - in my opinion, any API returning the output buffer for convenience should not return NULL, because that defeats the purpose.

> 
> Notice that the previous implementation was buggy as well. If `os::iso8601_time()` returned `NULL` it would have called `ASSERT_AND_RETURN(-1, pos)` which would have overwritten the last character in `pos` for non-product builds. It could also lead in a buffer-underflow if `pos` pointed to the very first character of an array.

You are right. Nice to see this fixed.

> src/hotspot/share/logging/logDecorations.cpp line 97:
> 
>> 95: void LogDecorations::print_utctime_decoration(outputStream* st) const {
>> 96:   char buf[29];
>> 97:   st->print_raw(os::iso8601_time(_millis, buf, sizeof(buf), true));
> 
> Same as for `LogDecorations::print_time_decoration()`.

+1

> src/hotspot/share/logging/logDecorations.cpp line 134:
> 
>> 132: void LogDecorations::print_tags_decoration(outputStream* st) const {
>> 133:   char tmp[255];
>> 134:   _tagset.label(tmp, sizeof(tmp)); // Todo: add a LogTagSet::label(outputStream*) to avoid this copy step
> 
> Yes, that would be desirable and probably not too hard :)
> 
> Otherwise you'll might have an uninitialized `tmp` array if the first tag doesn't fir into the array and  `LogTagSet::label()` returns `-1`. I understand that that's more a theoretical problem because at least one tag should always fir within 255 characters :)

My fist version had this but Xin mentioned the patch was too large :) I'll reinstate my first solution.

> src/hotspot/share/logging/logDecorations.hpp line 42:
> 
>> 40:   const jlong _nanos;             // for "timenanos"
>> 41:   const double _elapsed_seconds;  // for "uptime", "uptimemillis", "uptimenanos"
>> 42:   const int _pid;                 // for "pid"
> 
> You can use the same trick for `_pid` (i.e. make it static and resolve it a single time) like for `_host_name`. That will save you another 4 bytes for the size of `LogDecorations`.

Good point. Might even be more because of padding.

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

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