RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area

[Thomas Stüfe]

Thank you Marcus!

Would you mind running the change through jtreg? I ran it on Linux x64
(hotspot/test branch) and I got a number of errors, but nothing suspicious.
I repeated a most of the tests with an unpatched VM and got the same errors.

There is one suspicious test failing:
runtime/modules/PatchModule/PatchModuleCDS.java, with "Failed. Execution
failed: `main' threw exception: java.lang.RuntimeException: '[class,load]
java.lang.Thread source: jrt:/java.base' missing from stdout/stderr".
However, the same error also happens without my change. Still I would feel
better if you would do an independent test.

Thank you!

@ all:
I'd like to have another reviewer (maybe someone from the gc team? a lot of
gc log sites changed), and as usual a sponsor.

Kind Regards, Thomas

On Thu, Jun 22, 2017 at 3:33 PM, Marcus Larsson <marcus.larsson at oracle.com>
wrote:

>
>
> On 2017-06-21 18:16, Thomas Stüfe wrote:
>
>> Hi Marcus,
>>
>> On Wed, Jun 21, 2017 at 12:18 PM, Marcus Larsson <
>> marcus.larsson at oracle.com <mailto:marcus.larsson at oracle.com>> wrote:
>>
>>     Hi,
>>
>>
>>     On 2017-06-21 09:29, Thomas Stüfe wrote:
>>
>>         Hi Marcus,
>>
>>         thank you for reviewing!
>>
>>         New webrev:
>>         http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-
>> ul-logstream/all.webrev.01/webrev/
>>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/all.webrev.01/webrev/>
>>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/all.webrev.01/webrev/
>>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/all.webrev.01/webrev/>>
>>
>>         Delta to last version:
>>         http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-
>> ul-logstream/delta-all-00-to-01/webrev/
>>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/delta-all-00-to-01/webrev/>
>>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/delta-all-00-to-01/webrev/
>>
>>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/delta-all-00-to-01/webrev/>>
>>
>>         Changes:
>>
>>         - classfile/loaderConstraints.cpp: as you suggested, I fixed
>>         up all cases of "superfluous LogStream usage" I found and
>>         converted them to direct LogTarget::print() calls.
>>
>>         - classfile/sharedPathsMiscInfo.cpp: here I opted for removing
>>         any notion of UL from this method, instead I just hand in an
>>         outputStream*. Both the "is_enabled" check and the LogStream
>>         creation is now handed in the caller frame. I also added a
>>         trailing cr().
>>
>>         - gc/cms/compactibleFreeListSpace.cpp: removed the superfluous
>>         ResourceMark
>>
>>         - logging/logStream.hpp: enabled the private operator new()
>>         declarations to disable heap allocations for class LogStream.
>>         I also gave it a try, works fine, if you do new(), now you get
>>         a linker error.
>>
>>         The rest of the changes is concerned with the removal of
>>         "LogStreamCHeap" which is not needed anymore. Note that I
>>         found some new instances of "unguarded printing" and I updated
>>         comments at JDK-8182466
>>         <https://bugs.openjdk.java.net/browse/JDK-8182466
>>         <https://bugs.openjdk.java.net/browse/JDK-8182466>> .
>>
>>         Thanks & Regards, Thomas
>>
>>
>>     This looks great.
>>
>>     One last thing just hit me: We should have a unit test to cover
>>     the allocation case for LogStream. Preferably one that forces it
>>     to grow twice (once from stackbuffer, and once from heap), to make
>>     sure we exercise that code.
>>
>>
>> Good idea, especially since I had a bug in the line buffer handling :P
>>
>
> Aha! :)
>
>
>> New Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-
>> ul-logstream/all.webrev.02/webrev/ <http://cr.openjdk.java.net/%7
>> Estuefe/webrevs/8181917-refactor-ul-logstream/all.webrev.02/webrev/>
>>
>> Delta to last: http://cr.openjdk.java.net/~st
>> uefe/webrevs/8181917-refactor-ul-logstream/delta-01-to-02/webrev/ <
>> http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refact
>> or-ul-logstream/delta-01-to-02/webrev/>
>>
>
> Looks good to me.
>
> Thanks,
> Marcus
>
>
>
>> Changes: fixed a bug where, on buffer resize in LogStream, the new buffer
>> size was calculated wrong. Added a test to test LogStream resizing.
>>
>> I ran the gtests on win-x64 and linux-x64, and the jtreg
>> servicability/logging tests on linux-x64. All good. I am currently running
>> the whole hotspot jtreg tests, but that will take a while.
>>
>> Kind Regards, Thomas
>>
>>     Thanks,
>>     Marcus
>>
>>
>>
>