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

[Thomas Stüfe]

Hi Marcus,

On Wed, Jun 21, 2017 at 12:18 PM, Marcus Larsson <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/%7
>> Estuefe/webrevs/8181917-refactor-ul-logstream/all.webrev.01/webrev/>
>>
>> Delta to last version: http://cr.openjdk.java.net/~st
>> uefe/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/>
>>
>> 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> .
>>
>> 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

New Webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/all.webrev.02/webrev/

Delta to last:
http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/delta-01-to-02/webrev/

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
>