RFR: 8286180: Enable construction of LogStreamImpl from LogMessageImpl [v5]
David Holmes
dholmes at openjdk.java.net
Mon May 23 07:03:56 UTC 2022
On Wed, 18 May 2022 15:10:07 GMT, Johan Sjölén <duke at openjdk.java.net> wrote:
>> May I please have a review of this PR which adds non-interleaving log streams to UL?
>>
>> In `LogMessageImpl` I remove templates and instead directly store a reference to `LogTagSet` and change `LogTagHandle` to be consistent (from pointer to reference) with `LogMessageImpl`.
>>
>> Adding a non-interleaving log stream enables UL to replace two patterns found in HotSpot:
>>
>> 1. `ttyLocker` + `(x)tty`
>> Instead of taking the global tty lock in order to ensure non-interleaving logs we can now make a `NonInterleavingLogstream` and pass that around to various `print_on(outputStream*)` functions.
>>
>> 2. `stringStream` + `outputStream*`
>> In order to avoid taking the lock and being able to take arbitrary output streams a `stringStream` is created and finally printed through `print_raw(ss.as_string())`. This however requires a `ResourceMark`, which `NonInterleavingLogStream` does not require.
>>
>> Passes tier 1.
>
> Johan Sjölén has updated the pull request incrementally with three additional commits since the last revision:
>
> - Use unary not instead of comparing to false
> - Sort includes
> - Style changes
The code looks reasonable just reading through the changes, but I'm missing something fundamental here. We have:
`class NonInterleavingLogStream : public LogStreamImpl<LogMessageHandle>`
and
`class LogStream : public LogStreamImpl<LogTargetHandle>`
but I can't tell what makes the former "non-inter-leaving" while the latter is "inter-leaving" , nor do I understand the test cases for these.
Thanks.
test/hotspot/gtest/logging/test_logStream.cpp line 101:
> 99: {
> 100: LogStream foo(Log(gc)::info());
> 101: if(foo.is_enabled()) {
Style nit: space after "if" please - here and below.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8748
More information about the hotspot-runtime-dev
mailing list