RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Jun 22 14:46:58 UTC 2017
On Thu, Jun 22, 2017 at 4:13 PM, Erik Helin <erik.helin at oracle.com> wrote:
> 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
>
Thanks, Erik! No rush, take your time.
..Thomas
>
> > 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
> >
> >
> >
> >
>
More information about the hotspot-dev
mailing list