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

[Marcus Larsson]

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-refactor-ul-logstream/all.webrev.01/webrev/>
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refactor-ul-logstream/all.webrev.01/webrev/
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refactor-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-refactor-ul-logstream/delta-all-00-to-01/webrev/>
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refactor-ul-logstream/delta-all-00-to-01/webrev/
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refactor-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/%7Estuefe/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/ 
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8181917-refactor-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
>
>