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

Thu Jun 22 14:13:57 UTC 2017

On 06/22/2017 03:54 PM, Thomas Stüfe wrote:
> 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. 

I am on it :) I just had a lot of other to stuff to catch up on, but
hopefully I can return to this patch very soon. Given what I've seen so
far, I don't foresee any problems at all with this patch.

Thanks,
Erik

> Kind Regards, Thomas
> 
> On Thu, Jun 22, 2017 at 3:33 PM, Marcus Larsson
> <marcus.larsson at oracle.com <mailto: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>
>         <mailto: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/~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/
>         <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/~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/
>         <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>
>                 <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/~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/
>         <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/~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/
>         <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
> 
> 
> 
> 