Re: RFR: 8234532: Remove ThreadLocalAllocBuffer::_fast_refill_waste since it is never set [v3]
On Thu, 29 Apr 2021 08:11:12 GMT, Albert Mingkun Yang wrote: >> It has some cascading effect; two performance variables, `fastWaste` and >> `maxFastWaste`, can be removed also. > > Albert Mingkun Yang has updated the pull request incrementally with one > additional commit since the last revision: > > review Looks good. - Marked as reviewed by sjohanss (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3756
Re: RFR: 8253081: G1 fails on stale objects in archived module graph in Open Archive regions [v4]
On Tue, 17 Nov 2020 10:55:19 GMT, Thomas Schatzl wrote: >> Hi all, >> >> can I have reviews for this change that changes the way how archive >> regions are managed in general and specifically by the G1 collector, fixing >> the crashes caused by adding the module graph into the archive in >> [JDK-8244778](https://bugs.openjdk.java.net/browse/JDK-8244778)? >> >> Previously before the JDK-8244778 change, archived objects could always be >> assumed as live, and so the G1 collector did so, not caring about the >> archive region's contents at all. With JDK-8244778 however, archived objects >> could die, and keep stale references to objects outside of the archive >> regions, which obviously causes crashes when walking these objects. >> >> With this change, open archive region contents are basically handled as any >> other objects; to support that, all open archive regions are now reachable >> via a single object array root. This hopefully also facilitates >> implementation in other collectors. >> >> This allows us to remove quite a bit of special handling in G1 too; the only >> difference is that open archive regions will generally not be collected >> unless they are completely empty: we do want to profit from the sharing >> across VMs as much as possible. >> >> Testing: tier1-5, one or two 6-8 runs >> >> The appcds changes were done by @iklam. These changes are described in this >> document: >> https://wiki.openjdk.java.net/display/HotSpot/CDS+Archived+Heap+Improvements >> >> Thanks, >> Thomas > > Thomas Schatzl has updated the pull request incrementally with two additional > commits since the last revision: > > - sjohanss review > - Remove code that "activates" dormant objects as now all active objects are > reachable via the root object array Marked as reviewed by sjohanss (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1163
Re: RFR: 8253081: G1 fails on stale objects in archived module graph in Open Archive regions [v3]
On Mon, 16 Nov 2020 12:33:10 GMT, Thomas Schatzl wrote: >> Hi all, >> >> can I have reviews for this change that changes the way how archive >> regions are managed in general and specifically by the G1 collector, fixing >> the crashes caused by adding the module graph into the archive in >> [JDK-8244778](https://bugs.openjdk.java.net/browse/JDK-8244778)? >> >> Previously before the JDK-8244778 change, archived objects could always be >> assumed as live, and so the G1 collector did so, not caring about the >> archive region's contents at all. With JDK-8244778 however, archived objects >> could die, and keep stale references to objects outside of the archive >> regions, which obviously causes crashes when walking these objects. >> >> With this change, open archive region contents are basically handled as any >> other objects; to support that, all open archive regions are now reachable >> via a single object array root. This hopefully also facilitates >> implementation in other collectors. >> >> This allows us to remove quite a bit of special handling in G1 too; the only >> difference is that open archive regions will generally not be collected >> unless they are completely empty: we do want to profit from the sharing >> across VMs as much as possible. >> >> Testing: tier1-5, one or two 6-8 runs >> >> The appcds changes were done by @iklam. These changes are described in this >> document: >> https://wiki.openjdk.java.net/display/HotSpot/CDS+Archived+Heap+Improvements >> >> Thanks, >> Thomas > > Thomas Schatzl has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Merge branch 'master' of https://git.openjdk.java.net/jdk into > 8253081-null-narrow-klass-changes2 > - kbarrett review > - Initial import I've been focusing on the GC-parts and it looks good in general just a few comments. src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 4458: > 4456: heap_region_iterate(); > 4457: > 4458: remove_from_old_gen_sets(0, 0, cl.humongous_regions_reclaimed()); Looking at this call and now having three parameters that are "optional" for `remove_from_old_gen_sets()` I wonder if it would be cleaner to have three functions, one for each set. It would increase the number of times we take the look, but we could restructure the code in `G1ReclaimEmptyRegionsTask` to not do the updates in the worker threads and that way only take the lock when there will be no contention. If you fell like this is outside the scope of this change, please file an issue instead. src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1266: > 1264: _g1h->remove_from_old_gen_sets(cl.old_regions_removed(), > 1265:cl.archive_regions_removed(), > 1266:cl.humongous_regions_removed()); If we want to go with the one call per set approach, here we could just atomically add these to a task-counter for each type and then do the calls to update the sets after the task has finished. src/hotspot/share/gc/g1/g1FullGCPrepareTask.hpp line 60: > 58: G1FullGCCompactionPoint* _cp; > 59: uint _humongous_regions_removed; > 60: uint _open_archive_regions_freed; Since we no longer use these counters to update the sets, it is enough to just have one counter to track if any region has been freed. Or use a boolean if you prefer that. - Changes requested by sjohanss (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1163
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v6]
On Wed, 28 Oct 2020 11:31:01 GMT, Lin Zang wrote:
>> - Parallel heap iteration support for PSS
>> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103
>
> Lin Zang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix constant coding style and do code refine
The build testing caught an error. If you enable GitHub actions for openjdk,
you will get basic testing like this automatically (if your branch is based on
a recent master or merged recently).
src/hotspot/share/gc/parallel/psOldGen.hpp line 170:
> 168: uint iterable_blocks() {
> 169: return (object_space()->used_in_bytes() + IterateBlockSize - 1) /
> IterateBlockSize;
> 170: }
This causes a build failure on windows:
hotspot\share\gc/parallel/psOldGen.hpp(169): error C2220: the following warning
is treated as an error
hotspot\share\gc/parallel/psOldGen.hpp(169): warning C4267: 'return':
conversion from 'size_t' to 'uint', possible loss of data
I suggest either changing the return here to `int` and make an explicit cast or
`size_t` and then add the cast when used in `claim_and_get_block()`
-
Changes requested by sjohanss (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v5]
On Mon, 26 Oct 2020 07:40:50 GMT, Lin Zang wrote:
>> - Parallel heap iteration support for PSS
>> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103
>
> Lin Zang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refine HeapBlockClaimer implementation
Thanks for the update, some additional comments, but just minor things.
I did some basic local testing and I've also started some tests to build on
multiple platforms, once the review is complete I will do a more thorough test
run to make sure the code is exercised on all platforms as well.
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 588:
> 586: return new PSScavengeParallelObjectIterator(thread_num);
> 587: }
> 588:
No need to pass down thread_num to `PSScavengeParallelObjectIterator` because
it is not needed anymore. This way we can also remove the `_thread_num` member.
src/hotspot/share/gc/parallel/psOldGen.hpp line 56:
> 54:
> 55: // Block size for parallel iteration
> 56: static const size_t _iterate_block_size = 1024 * 1024;
Constants are named without _ and using camel case. I think you can also add to
the comment that the size is in bytes:
Suggestion:
// Block size in bytes for parallel iteration
static const size_t IterateBlockSize = 1024 * 1024;
Also remember to update the comments referring to the constant :)
src/hotspot/share/gc/parallel/psOldGen.hpp line 169:
> 167: void object_iterate(ObjectClosure* cl) {
> object_space()->object_iterate(cl); }
> 168: uint iterable_blocks() {
> 169: return (object_space()->used_in_bytes() + _iterate_block_size -1) /
> _iterate_block_size;
Space before 1 in `-1`.
-
Changes requested by sjohanss (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]
On Mon, 19 Oct 2020 13:09:34 GMT, Lin Zang wrote: >> - Parallel heap iteration support for PSS >> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103 > > Lin Zang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Took a second look today and found two additional problems. I would also be interested in what kind of testing you've done with this patch. src/hotspot/share/gc/parallel/psOldGen.cpp line 199: > 197: > 198: // iterate objects in block. > 199: HeapWord* end = MIN2(top, begin + _iterate_block_size); `_iterate_block_size` is a size in bytes (or at least you use it like a size in bytes when calculating the number of blocks), so before you can add it you need to convert it to a size in words like this: Suggestion: size_t block_word_size = _iterate_block_size / HeapWordSize; HeapWord* begin = bottom + block_index * block_word_size; assert((block_word_size % (ObjectStartArray::block_size)) == 0, "BLOCK SIZE not a multiple of start_array block"); // iterate objects in block. HeapWord* end = MIN2(top, begin + block_word_size); - Changes requested by sjohanss (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]
On Wed, 21 Oct 2020 19:49:03 GMT, Stefan Johansson wrote: >> Lin Zang has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 565: > >> 563: size_t old_gen_used = >> ParallelScavengeHeap::heap()->old_gen()->used_in_bytes(); >> 564: size_t block_size = >> ParallelScavengeHeap::heap()->old_gen()->iterate_block_size(); >> 565: uint n_blocks_in_old = old_gen_used / block_size + 1; > > Instead of doing this calculation here, what do you think about making > `iterate_block_size()` a constant in `PSOldGen` and instead adding a function > that returns the number of blocks available, something like: > `iterable_blocks()` One additional thing here, the `n_blocks_in_old` calculation will return one extra block when old_gen_used is a multiple of block_size. : Here's an alternative: uint n_blocks_in_old = (old_gen_used + block_size - 1) / block_size; - PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]
On Mon, 19 Oct 2020 13:09:34 GMT, Lin Zang wrote: >> - Parallel heap iteration support for PSS >> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103 > > Lin Zang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp line 318: > 316: static const uint eden_index = 0; > 317: static const uint survivor_index = 1; > 318: static const uint num_inseparable_spaces = 2; One more comment. These should be CamelCase, and I have an idea to get rid of `num_inseparable_spaces`. If we make `_claimed_index` an integral type we could define `EdenIndex = -2` and `SurvivorIndex = -1`. If we then initialize `_claimed_index = EdenIndex` the old gen indices will be correct without needing a decrease. - PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap [v4]
On Mon, 19 Oct 2020 13:09:34 GMT, Lin Zang wrote:
>> - Parallel heap iteration support for PSS
>> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103
>
> Lin Zang has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR.
Hi Lin,
A nice improvement over the last version. I still have some comments though.
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 543:
> 541:
> 542: void ParallelScavengeHeap::object_iterate_parallel(ObjectClosure* cl,
> 543:uint worker_id,
`worker_id` is never used, please remove.
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 562:
> 560: HeapBlockClaimer::HeapBlockClaimer(uint n_workers) :
> 561: _n_workers(n_workers), _n_blocks(0), _claims(NULL) {
> 562: assert(n_workers > 0, "Need at least one worker.");
Unless I'm missing something the only use of `_n_workers` is the assert here
and I think it's better to just skip it.
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 565:
> 563: size_t old_gen_used =
> ParallelScavengeHeap::heap()->old_gen()->used_in_bytes();
> 564: size_t block_size =
> ParallelScavengeHeap::heap()->old_gen()->iterate_block_size();
> 565: uint n_blocks_in_old = old_gen_used / block_size + 1;
Instead of doing this calculation here, what do you think about making
`iterate_block_size()` a constant in `PSOldGen` and instead adding a function
that returns the number of blocks available, something like:
`iterable_blocks()`
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 591:
> 589: }
> 590: next_index = Atomic::load(&_unclaimed_index);
> 591: }
I think this can be simplified a bit. If you use Atomic::fetch_and_add() for
the claiming, you only need `_unclaimed_index` and can get rid of `_claims`. I
think you might want to rename it to `_claimed_index` or something. It could
look something like this:
Suggestion:
assert(block_index != NULL, "Invalid index pointer");
*block_index = Atomic::fetch_and_add(&_claimed_index, 1);
if (*block_index >= _n_blocks) {
return false;
}
return true;
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 604:
> 602: _thread_num(thread_num),
> 603: _heap(ParallelScavengeHeap::heap()),
> 604: _claimer(thread_num == 0 ?
> ParallelScavengeHeap::heap()->workers().active_workers() : thread_num) {}
As mentioned, the `_claimer` don't need the number of threads so it can be
removed from here as well.
src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp line 297:
> 295: // The HeapBlockClaimer is used during parallel iteration over heap,
> 296: // allowing workers to claim heap blocks, gaining exclusive rights to
> these blocks.
> 297: // The eden, survivor spaces are treated as single blocks as it is hard
> to divide
Suggestion:
// The eden and survivor spaces are treated as single blocks as it is hard to
divide
src/hotspot/share/gc/parallel/psOldGen.cpp line 200:
> 198: // iterate objects in block.
> 199: HeapWord* end = MIN2(top, begin + _iterate_block_size);
> 200: // There can be no object between begin and end.
I would change this to something like:
"Only iterate if there are objects between begin and end.
src/hotspot/share/gc/parallel/psOldGen.hpp line 56:
> 54:
> 55: // Block size for parallel iteration
> 56: const size_t _iterate_block_size;
Turn into constant.
-
PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap
On Tue, 20 Oct 2020 07:44:54 GMT, Lin Zang wrote: >>> Dear @stefank, >>> I have update this PR that use a claimer to help worker thread do parallel >>> iteration. would you like to help review >>> again? Thanks, >>> Lin >> >> Wrong Stefan, I think you mean @kstefanj > > Hi @stefank, > So sorry to bother you! > Hi @kstefanj > Would you like to help review this? Thanks! > > BRs, > Lin Hi Lin, I will take a look when I get time. In the middle of some other things right now. For future reference, you should avoid force pushing changes to open PRs. Instead of rebasing you can merge with master to make your branch up to date. This time it don't really matter since we didn't have any comments in the code, but otherwise those might be lost. - PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap
On Fri, 11 Sep 2020 07:25:37 GMT, Lin Zang wrote: >> - Parallel heap iteration support for PSS >> - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103 > > Dear All, > May I ask your help to review this PR? Thanks! > -Lin Hi Lin, Sorry for not getting to this sooner. One of the reasons is that I haven't had time to explore a better solution, but the above notification triggered me to at least share my thoughts. I would like us to avoid having the logic that check how many workers are doing the work and instead have some mechanism that claim different chunks of work. You can compare it a bit to G1 where we have the `HeapRegionClaimer` that make sure only one thread handles a given region. This claimer needs to be a bit different and allow claiming of eden, to-space, from-space and then multiple chunks of old. But I believe such solution would both be more efficient (since all threads can help out on old in the end) and easier to follow (no special cases). So basically the `PSScavengeParallelObjectIterator` need to set up this claimer and pass it down to the workers and all workers than try to do all work, but only the one getting the claim will do the actual work. What do you think about this approach? Do you understand what I'm after? - PR: https://git.openjdk.java.net/jdk/pull/25
Integrated: 8253815: Remove unused HeapRegionManager::_num_committed from SA
On Tue, 29 Sep 2020 18:29:57 GMT, Stefan Johansson wrote: > While doing some refactoring I wanted to move > HeapRegionManager::_num_committed and realized that I needed to update > the SA. After some looking around it turns out that it is unused and I can > remove the numCommittedField from the > HeapRegionManager class in the SA. Local build and test looks good and > running tier1 and some svc-testing to ensure I > haven't missed anything. This pull request has now been integrated. Changeset: 709cfe5f Author:Stefan Johansson URL: https://git.openjdk.java.net/jdk/commit/709cfe5f Stats: 8 lines in 2 files changed: 0 ins; 8 del; 0 mod 8253815: Remove unused HeapRegionManager::_num_committed from SA Reviewed-by: tschatzl, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/408
Re: RFR: 8253815: Remove unused HeapRegionManager::_num_committed from SA
On Tue, 29 Sep 2020 21:32:07 GMT, Chris Plummer wrote: >> The field will remain for now, but the plan is to refactor it into a new >> class. The new class is not needed by the SA >> and I would really like to avoid having to add unused and untested code to >> the SA. To simplify this coming change I >> want to remove this from the SA straight away. Even without this coming >> change I would vote for removing this type of >> unused code from the SA. I don't think being able to dump the value is worth >> enough compared to the maintenance cost of >> keeping it in. > > Ok. If the field is going away eventually then you can remove it now. Awesome, thanks for reviewing. - PR: https://git.openjdk.java.net/jdk/pull/408
Re: RFR: 8253815: Remove unused HeapRegionManager::_num_committed from SA
On Tue, 29 Sep 2020 19:43:45 GMT, Chris Plummer wrote: >> While doing some refactoring I wanted to move >> HeapRegionManager::_num_committed and realized that I needed to update >> the SA. After some looking around it turns out that it is unused and I can >> remove the numCommittedField from the >> HeapRegionManager class in the SA. Local build and test looks good and >> running tier1 and some svc-testing to ensure I >> haven't missed anything. > > src/hotspot/share/gc/g1/vmStructs_g1.hpp line 56: > >> 54: >> \ >> 55: nonstatic_field(HeapRegionManager, _regions, >> G1HeapRegionTable)\ >> 56: nonstatic_field(HeapRegionManager, _num_committed,uint) >> \ > > If this field is remaining in hotspot, it should also remain in vmStructs. > Although SA does not need to explicitly know > about this field to function properly, SA can be used to dump hotspot > objects, and can only dump objects whose types > are in vmStructs, and for those types can only display the fields that are > also in vmStructs. The field will remain for now, but the plan is to refactor it into a new class. The new class is not needed by the SA and I would really like to avoid having to add unused and untested code to the SA. To simplify this coming change I want to remove this from the SA straight away. Even without this coming change I would vote for removing this type of unused code from the SA. I don't think being able to dump the value is worth enough compared to the maintenance cost of keeping it in. - PR: https://git.openjdk.java.net/jdk/pull/408
RFR: 8253815: Remove unused HeapRegionManager::_num_committed from SA
While doing some refactoring I wanted to move HeapRegionManager::_num_committed and realized that I needed to update the SA. After some looking around it turns out that it is unused and I can remove the numCommittedField from the HeapRegionManager class in the SA. Local build and test looks good and running tier1 and some svc-testing to ensure I haven't missed anything. - Commit messages: - 8253815: Remove unused HeapRegionManager::_num_committed from SA Changes: https://git.openjdk.java.net/jdk/pull/408/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=408=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253815 Stats: 8 lines in 2 files changed: 0 ins; 8 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/408.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/408/head:pull/408 PR: https://git.openjdk.java.net/jdk/pull/408
Re: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
Mach5 testing looks good. Can someone in the serviceability team do the second review? Cheers, Stefan On 2018-03-28 13:32, Yasumasa Suenaga wrote: Thanks Stefan, I'm waiting for second reviewer. Yasumasa 2018年3月28日(水) 18:36 Stefan Johansson <stefan.johans...@oracle.com <mailto:stefan.johans...@oracle.com>>: Hi Yasumasa, Local testing looks good and I've kicked of some additional Mach5 testing that will include these tests on all platforms. Cheers, Stefan On 2018-03-28 06:04, Yasumasa Suenaga wrote: > Hi Stefan, > > Thank you for sharing your report! > I could reproduce them on my VM. > > I've fixed them in new webrev, and it works fine on my environment. > Could you check again? > > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/ <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.03/> > > > Thanks, > > Yasumasa > > > > 2018-03-28 0:29 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com <mailto:stefan.johans...@oracle.com>>: >> >> On 2018-03-27 16:44, Yasumasa Suenaga wrote: >>> Hi Stefan, >>> >>> On 2018/03/27 22:45, Stefan Johansson wrote: >>>> Hi Yasumasa, >>>> >>>> On 2018-03-27 10:56, Yasumasa Suenaga wrote: >>>>> Hi Stefan, >>>>> >>>>> Thank you for your comment. >>>>> I updated webrev: >>>>> >>>>> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.01/ <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.01/> >>>> I think the usage of Optional in Expression.setRequired(bool) is a bit >>>> unnecessary. It will create temporary objects and there is no benefit from >>>> just doing two simple if-statements. >>> >>> I fixed it in new webrev: >>> http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.02/ <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.02/> >>> >>> >>>> I also ran this patch (and the one using forcibly) on my single core VM >>>> and realized that this fix will have to include some awk-file updates to >>>> make the test in test/jdk/sun/tools/jstat pass when Serial in chosen as the >>>> default collector. The tests in test/jdk/sun/tools/jstatd/ are fine. >>> >>> Can you share the failure report? >> It relates to all tests that display the the CGC and the CGCT columns, for >> example in jstatGCOutput1.sh: >> S0C S1C S0U S1U EC EU OC OU MC MU >> CCSC CCSU YGC YGCT FGC FGCT CGC CGCT GCT >> 256.0 256.0 254.0 0.0 2176.0 1025.0 5504.0 920.5 7168.0 >> 6839.7 768.0 602.8 2 0.007 0 0.000 - - 0.007 >> >> The awk regex needs to be updated to handle '-' for these tests: >> test: sun/tools/jstat/jstatGcCapacityOutput1.sh >> Failed. Execution failed: exit code 1 >> >> test: sun/tools/jstat/jstatGcMetaCapacityOutput1.sh >> Failed. Execution failed: exit code 1 >> >> test: sun/tools/jstat/jstatGcNewCapacityOutput1.sh >> Failed. Execution failed: exit code 1 >> >> test: sun/tools/jstat/jstatGcOldCapacityOutput1.sh >> Failed. Execution failed: exit code 1 >> >> test: sun/tools/jstat/jstatGcOldOutput1.sh >> Failed. Execution failed: exit code 1 >> >> test: sun/tools/jstat/jstatGcOutput1.sh >> Failed. Execution failed: exit code 1 >> >> >>> If it occurs in jstatClassloadOutput1.sh, it relates to JDK-8173942. >>> >>> >>> Thanks, >>> >>> Yasumasa >>> >>> >>>> Thanks, >>>> Stefan >>>>> submit-hs: mach5-one-ysuenaga-JDK-8199519-20180327-0652-16322 >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>> >>>>> 2018-03-27 0:03 GMT+09:00 Stefan Johansson >>>>> <stefan.johans...@oracle.com <mailto:stefan.johans...@oracle.com>>: >>>>>> Hi Yasumasa, >>>>>> >>>>>> On 2018-03-22 11:35, Yasumasa Suenaga wro
Re: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
Hi Yasumasa, Local testing looks good and I've kicked of some additional Mach5 testing that will include these tests on all platforms. Cheers, Stefan On 2018-03-28 06:04, Yasumasa Suenaga wrote: Hi Stefan, Thank you for sharing your report! I could reproduce them on my VM. I've fixed them in new webrev, and it works fine on my environment. Could you check again? http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/ Thanks, Yasumasa 2018-03-28 0:29 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com>: On 2018-03-27 16:44, Yasumasa Suenaga wrote: Hi Stefan, On 2018/03/27 22:45, Stefan Johansson wrote: Hi Yasumasa, On 2018-03-27 10:56, Yasumasa Suenaga wrote: Hi Stefan, Thank you for your comment. I updated webrev: webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.01/ I think the usage of Optional in Expression.setRequired(bool) is a bit unnecessary. It will create temporary objects and there is no benefit from just doing two simple if-statements. I fixed it in new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.02/ I also ran this patch (and the one using forcibly) on my single core VM and realized that this fix will have to include some awk-file updates to make the test in test/jdk/sun/tools/jstat pass when Serial in chosen as the default collector. The tests in test/jdk/sun/tools/jstatd/ are fine. Can you share the failure report? It relates to all tests that display the the CGC and the CGCT columns, for example in jstatGCOutput1.sh: S0CS1CS0US1U EC EU OC OU MC MU CCSC CCSU YGC YGCT FGCFGCTCGCCGCT GCT 256.0 256.0 254.0 0.02176.0 1025.05504.0 920.57168.0 6839.7 768.0 602.8 20.007 0 0.000 - -0.007 The awk regex needs to be updated to handle '-' for these tests: test: sun/tools/jstat/jstatGcCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcMetaCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcNewCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcOldCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcOldOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcOutput1.sh Failed. Execution failed: exit code 1 If it occurs in jstatClassloadOutput1.sh, it relates to JDK-8173942. Thanks, Yasumasa Thanks, Stefan submit-hs: mach5-one-ysuenaga-JDK-8199519-20180327-0652-16322 Thanks, Yasumasa 2018-03-27 0:03 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com>: Hi Yasumasa, On 2018-03-22 11:35, Yasumasa Suenaga wrote: Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8199519 webrev: cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.00/ The fix seems to make things to work as expected. Manually tested it and Mach5 also looks good. I have some comments regarding the patch. I think 'forcibly' should be rename to something more descriptive. Naming is never easy but I think 'required' would be better, as in, this column is required and not allowed to print '-'. That would also render the code in ExpressionResolver.java to be: return new Literal(isRequired ? 0.0d : Double.NaN); I think that also better explains why we return 0 instead of NaN. I would also like to see the forcibly/required state moved into the Expression it self, that way we don't have to pass it around but can instead do: return new Literal(e.isRequired() ? 0.0d : Double.NaN); Thanks, Stefan After JDK-815, some jstat tests are failed because GCT in jstat output is dash (-) if garbage collector is not concurrent collector e.g. Serial GC. I fixed that GCT can be calculated correctly. This change has been tested on Mach5 by Stefan. Thanks, Yasumasa
Re: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
On 2018-03-27 16:44, Yasumasa Suenaga wrote: Hi Stefan, On 2018/03/27 22:45, Stefan Johansson wrote: Hi Yasumasa, On 2018-03-27 10:56, Yasumasa Suenaga wrote: Hi Stefan, Thank you for your comment. I updated webrev: webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.01/ I think the usage of Optional in Expression.setRequired(bool) is a bit unnecessary. It will create temporary objects and there is no benefit from just doing two simple if-statements. I fixed it in new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.02/ I also ran this patch (and the one using forcibly) on my single core VM and realized that this fix will have to include some awk-file updates to make the test in test/jdk/sun/tools/jstat pass when Serial in chosen as the default collector. The tests in test/jdk/sun/tools/jstatd/ are fine. Can you share the failure report? It relates to all tests that display the the CGC and the CGCT columns, for example in jstatGCOutput1.sh: S0C S1C S0U S1U EC EU OC OU MC MU CCSC CCSU YGC YGCT FGC FGCT CGC CGCT GCT 256.0 256.0 254.0 0.0 2176.0 1025.0 5504.0 920.5 7168.0 6839.7 768.0 602.8 2 0.007 0 0.000 - - 0.007 The awk regex needs to be updated to handle '-' for these tests: test: sun/tools/jstat/jstatGcCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcMetaCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcNewCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcOldCapacityOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcOldOutput1.sh Failed. Execution failed: exit code 1 test: sun/tools/jstat/jstatGcOutput1.sh Failed. Execution failed: exit code 1 If it occurs in jstatClassloadOutput1.sh, it relates to JDK-8173942. Thanks, Yasumasa Thanks, Stefan submit-hs: mach5-one-ysuenaga-JDK-8199519-20180327-0652-16322 Thanks, Yasumasa 2018-03-27 0:03 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com>: Hi Yasumasa, On 2018-03-22 11:35, Yasumasa Suenaga wrote: Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8199519 webrev: cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.00/ The fix seems to make things to work as expected. Manually tested it and Mach5 also looks good. I have some comments regarding the patch. I think 'forcibly' should be rename to something more descriptive. Naming is never easy but I think 'required' would be better, as in, this column is required and not allowed to print '-'. That would also render the code in ExpressionResolver.java to be: return new Literal(isRequired ? 0.0d : Double.NaN); I think that also better explains why we return 0 instead of NaN. I would also like to see the forcibly/required state moved into the Expression it self, that way we don't have to pass it around but can instead do: return new Literal(e.isRequired() ? 0.0d : Double.NaN); Thanks, Stefan After JDK-815, some jstat tests are failed because GCT in jstat output is dash (-) if garbage collector is not concurrent collector e.g. Serial GC. I fixed that GCT can be calculated correctly. This change has been tested on Mach5 by Stefan. Thanks, Yasumasa
Re: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
Hi Yasumasa, On 2018-03-27 10:56, Yasumasa Suenaga wrote: Hi Stefan, Thank you for your comment. I updated webrev: webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.01/ I think the usage of Optional in Expression.setRequired(bool) is a bit unnecessary. It will create temporary objects and there is no benefit from just doing two simple if-statements. I also ran this patch (and the one using forcibly) on my single core VM and realized that this fix will have to include some awk-file updates to make the test in test/jdk/sun/tools/jstat pass when Serial in chosen as the default collector. The tests in test/jdk/sun/tools/jstatd/ are fine. Thanks, Stefan submit-hs: mach5-one-ysuenaga-JDK-8199519-20180327-0652-16322 Thanks, Yasumasa 2018-03-27 0:03 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com>: Hi Yasumasa, On 2018-03-22 11:35, Yasumasa Suenaga wrote: Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8199519 webrev: cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.00/ The fix seems to make things to work as expected. Manually tested it and Mach5 also looks good. I have some comments regarding the patch. I think 'forcibly' should be rename to something more descriptive. Naming is never easy but I think 'required' would be better, as in, this column is required and not allowed to print '-'. That would also render the code in ExpressionResolver.java to be: return new Literal(isRequired ? 0.0d : Double.NaN); I think that also better explains why we return 0 instead of NaN. I would also like to see the forcibly/required state moved into the Expression it self, that way we don't have to pass it around but can instead do: return new Literal(e.isRequired() ? 0.0d : Double.NaN); Thanks, Stefan After JDK-815, some jstat tests are failed because GCT in jstat output is dash (-) if garbage collector is not concurrent collector e.g. Serial GC. I fixed that GCT can be calculated correctly. This change has been tested on Mach5 by Stefan. Thanks, Yasumasa
Re: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
Hi Yasumasa, On 2018-03-22 11:35, Yasumasa Suenaga wrote: Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8199519 webrev: cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.00/ The fix seems to make things to work as expected. Manually tested it and Mach5 also looks good. I have some comments regarding the patch. I think 'forcibly' should be rename to something more descriptive. Naming is never easy but I think 'required' would be better, as in, this column is required and not allowed to print '-'. That would also render the code in ExpressionResolver.java to be: return new Literal(isRequired ? 0.0d : Double.NaN); I think that also better explains why we return 0 instead of NaN. I would also like to see the forcibly/required state moved into the Expression it self, that way we don't have to pass it around but can instead do: return new Literal(e.isRequired() ? 0.0d : Double.NaN); Thanks, Stefan After JDK-815, some jstat tests are failed because GCT in jstat output is dash (-) if garbage collector is not concurrent collector e.g. Serial GC. I fixed that GCT can be calculated correctly. This change has been tested on Mach5 by Stefan. Thanks, Yasumasa
Re: RFR: JDK-8175312: SA: clhsdb: Provide an improved heap summary for 'universe' for G1GC
Hi Jini, This looks good. I'm totally fine with skipping metaspace if that isn't displayed for the other GCs. Cheers, Stefan On 2018-03-09 10:29, Jini George wrote: Here is the revised webrev: http://cr.openjdk.java.net/~jgeorge/8175312/webrev.02/ I have made modifications to have the 'universe' command display details like: hsdb> universe Heap Parameters: garbage-first heap [0x00072520, 0x0007c000] region size 1024K G1 Heap: regions = 2478 capacity = 2598371328 (2478.0MB) used = 5242880 (5.0MB) free = 2593128448 (2473.0MB) 0.20177562550443906% used G1 Young Generation: Eden Space: regions = 5 capacity = 8388608 (8.0MB) used = 5242880 (5.0MB) free = 3145728 (3.0MB) 62.5% used Survivor Space: regions = 0 capacity = 0 (0.0MB) used = 0 (0.0MB) free = 0 (0.0MB) 0.0% used G1 Old Generation: regions = 0 capacity = 155189248 (148.0MB) used = 0 (0.0MB) free = 155189248 (148.0MB) 0.0% used I did not add the metaspace details since that did not seem to be in line with the 'universe' output for other GCs. I have added a new command "g1regiondetails" to display the region details, and have modified the tests accordingly. hsdb> g1regiondetails Region Details: Region: 0x00072520,0x00072520,0x00072530:Free Region: 0x00072530,0x00072530,0x00072540:Free Region: 0x00072540,0x00072540,0x00072550:Free Region: 0x00072550,0x00072550,0x00072560:Free Region: 0x00072560,0x00072560,0x00072570:Free Region: 0x00072570,0x00072570,0x00072580:Free ... Thanks, Jini. On 2/28/2018 12:56 PM, Jini George wrote: Thank you very much, Stefan. My answers inline. On 2/27/2018 3:30 PM, Stefan Johansson wrote: Hi Jini, JIRA ID:https://bugs.openjdk.java.net/browse/JDK-8175312 Webrev: http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.html It looks like a file is missing, did you forget to add it to the changeset? Indeed, I had missed that! I added the missing file in the following webrev: http://cr.openjdk.java.net/~jgeorge/8175312/webrev.01/ --- open/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1CollectedHeap.java:36: error: cannot find symbol import sun.jvm.hotspot.gc.shared.PrintRegionClosure; --- Otherwise the change looks good, but I would like to see the output live. For a big heap this will print a lot of data, just wondering if the universe command is the correct choice for this kind of output. I like having the possibility to print all regions, so I want the change but maybe it should be a different command and 'universe' just prints a little more than before. Something like our logging heap-summary at shutdown: garbage-first heap total 16384K, used 3072K [0xff00, 0x0001) region size 1024K, 4 young (4096K), 0 survivors (0K) Metaspace used 6731K, capacity 6825K, committed 7040K, reserved 1056768K class space used 559K, capacity 594K, committed 640K, reserved 1048576K Ok, will add this, and could probably have the region details displayed under a new command called "g1regiondetails", or some such, and send out a new webrev. Thanks, Jini. Thanks, Stefan Modifications have been made to display the regions like: ... Region: 0x0005c540,0x0005c560,0x0005c560:Old Region: 0x0005c560,0x0005c580,0x0005c580:Old Region: 0x0005c580,0x0005c5a0,0x0005c5a0:Old Region: 0x0005c5a0,0x0005c5c0,0x0005c5c0:Old Region: 0x0005c5c0,0x0005c5c0,0x0005c5e0:Free Region: 0x0005c5e0,0x0005c5e0,0x0005c600:Free Region: 0x0005c600,0x0005c620,0x0005c620:Old ... The jtreg test at this point does not include any testing for the display of archived or pinned regions. The testing for this will be added once JDK-8174994 is resolved. The SA tests pass with jprt and Mach5. Thanks, Jini.
Re: RFR: JDK-8175312: SA: clhsdb: Provide an improved heap summary for 'universe' for G1GC
Hi Jini, On 2018-02-27 08:37, Jini George wrote: Gentle reminder! Thanks, Jini. On 2/14/2018 2:37 PM, Jini George wrote: Hello, (Including the SVC and GC groups) Requesting reviews for enabling the clhsdb 'universe' command to print out the various G1 regions (along with the type). JIRA ID:https://bugs.openjdk.java.net/browse/JDK-8175312 Webrev: http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.html It looks like a file is missing, did you forget to add it to the changeset? --- open/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1CollectedHeap.java:36: error: cannot find symbol import sun.jvm.hotspot.gc.shared.PrintRegionClosure; --- Otherwise the change looks good, but I would like to see the output live. For a big heap this will print a lot of data, just wondering if the universe command is the correct choice for this kind of output. I like having the possibility to print all regions, so I want the change but maybe it should be a different command and 'universe' just prints a little more than before. Something like our logging heap-summary at shutdown: garbage-first heap total 16384K, used 3072K [0xff00, 0x0001) region size 1024K, 4 young (4096K), 0 survivors (0K) Metaspace used 6731K, capacity 6825K, committed 7040K, reserved 1056768K class space used 559K, capacity 594K, committed 640K, reserved 1048576K Thanks, Stefan Modifications have been made to display the regions like: ... Region: 0x0005c540,0x0005c560,0x0005c560:Old Region: 0x0005c560,0x0005c580,0x0005c580:Old Region: 0x0005c580,0x0005c5a0,0x0005c5a0:Old Region: 0x0005c5a0,0x0005c5c0,0x0005c5c0:Old Region: 0x0005c5c0,0x0005c5c0,0x0005c5e0:Free Region: 0x0005c5e0,0x0005c5e0,0x0005c600:Free Region: 0x0005c600,0x0005c620,0x0005c620:Old ... The jtreg test at this point does not include any testing for the display of archived or pinned regions. The testing for this will be added once JDK-8174994 is resolved. The SA tests pass with jprt and Mach5. Thanks, Jini.
Re: PING: RFR: JDK-8153333: [REDO] STW phases at Concurrent GC should count in PerfCounter
On 2018-02-06 06:10, Yasumasa Suenaga wrote: Hi Stefan, I agree, for G1 this should not be controlled. Maybe I was a bit unclear, I was wondering why we want to control it for CMS. I said to remove -XX:EnableConcGCPerfCounter in two years ago. I've missed it :-) http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017125.html So I uploaded new webrev. This change includes copyright year updates. http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.06/ Thanks Yasumasa, This looks good to me, will do some more testing while waiting for a second reviewer and I can sponsor the change once it's ready to go. This change passes all tests on submit repo, and :hotspot_serviceability :jdk_tools tests on my laptop. http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180206-0222-10428 If we do the change for CMS, we should probably also do a CSR, but that should be fairly straight forward. What should I do to get CSR approve? In the bug system under "More" you can choose "Create CSR" which is the first step. More information can be found on the wiki: https://wiki.openjdk.java.net/display/csr/CSR+FAQs Cheers, Stefan Thanks, Yasumasa 2018-02-06 0:33 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com>: On 2018-02-03 06:40, Yasumasa Suenaga wrote: On 2018/02/02 23:38, Stefan Johansson wrote: Hi Yasumasa, The changes doesn't apply clean on the latest jdk/hs, can you provide an updated webrev? I uploaded webrev for jdk-hs: cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.05/ Thanks, I've kicked off a testing job now to verify nothing unexpected fails. The testing done by the submit repo doesn't cover the tests you have update so I plan to take the change for a spin and make sure the correct tests are run and verified in Mach 5. I've also tested hotspot/jtreg/:hotspot_serviceability and jdk/:jdk_tools on my laptop. I did not see any errors / failures which are related to this change. I also ran some local tests on this and it looks good. Also a question about the change. Why do we need a special flag for CMS? I see that the original bug report refers to the flag as being a way to turn on and off the feature but the current implementation only consider the flag for CMS. http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/016774.html Originally, STW phases (Remark and Cleanup) at G1 are not counted in jstat FGC column. So I think we need not to control the behavior of PerfCounter for G1. I agree, for G1 this should not be controlled. Maybe I was a bit unclear, I was wondering why we want to control it for CMS. I think either we should change the behavior without guarding it by a flag or just skip updating CMS (and leave the pauses in FGC). If we do the change for CMS, we should probably also do a CSR, but that should be fairly straight forward. I also found the old review thread where Jon M had the same comment (removing the flag) and it looks like all agreed on that: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017118.html Thanks, Stefan Thanks, Yasumasa Thanks, Stefan On 2018-02-01 14:58, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ This change has been passed Mach 5 via submit repo: http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180201-0805-10101 Thanks, Yasumasa On 2017/11/01 22:02, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ Also I need JPRT results of this change. Could you cooperate? Thanks, Yasumasa On 2017/09/27 0:08, Yasumasa Suenaga wrote: Hi all, I uploaded new webrev to be adapted to jdk10/hs: http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ I want to check this patch via JPRT, but I cannot access it. Could you cooperate? Thanks, yasumasa On 2017/09/21 7:46, Yasumasa Suenaga wrote: PING: Have you checked this issue? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ Yasumasa On 2017/07/01 23:44, Yasumasa Suenaga wrote: PING: Have you checked this issue? Yasumasa On 2017/06/14 13:22, Yasumasa Suenaga wrote: Hi all, I changed PerfCounter to show CGC STW phase in jstat in JDK-8151674. However, it occurred several jtreg test failure, so it was back-outed. I want to resume to work for this issue. http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ These changes are work fine on jtreg test as below: hotspot/test/serviceability/tmtools/jstat jdk/test/sun/tools Since JDK 9, default GC algorithm is set to G1. So I think this change is useful to watch GC behavior through jstat. I cannot access JPRT. Could you help? Thanks, Yasumasa
Re: PING: RFR: JDK-8153333: [REDO] STW phases at Concurrent GC should count in PerfCounter
Hi, On 2018-02-06 06:30, Yasumasa Suenaga wrote: Hi Stefan, I've got another report from submit repo, and it includes one error on OS X. Can you share it? http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180206-0355-10435 This is a known issue, Robin W is working on a fix as we speak: https://bugs.openjdk.java.net/browse/JDK-8193308 Stefan I've tested this change (webrev.06) on Linux x64. Thanks, Yasumasa 2018-02-06 14:10 GMT+09:00 Yasumasa Suenaga <yasue...@gmail.com>: Hi Stefan, I agree, for G1 this should not be controlled. Maybe I was a bit unclear, I was wondering why we want to control it for CMS. I said to remove -XX:EnableConcGCPerfCounter in two years ago. I've missed it :-) http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017125.html So I uploaded new webrev. This change includes copyright year updates. http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.06/ This change passes all tests on submit repo, and :hotspot_serviceability :jdk_tools tests on my laptop. http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180206-0222-10428 If we do the change for CMS, we should probably also do a CSR, but that should be fairly straight forward. What should I do to get CSR approve? Thanks, Yasumasa 2018-02-06 0:33 GMT+09:00 Stefan Johansson <stefan.johans...@oracle.com>: On 2018-02-03 06:40, Yasumasa Suenaga wrote: On 2018/02/02 23:38, Stefan Johansson wrote: Hi Yasumasa, The changes doesn't apply clean on the latest jdk/hs, can you provide an updated webrev? I uploaded webrev for jdk-hs: cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.05/ Thanks, I've kicked off a testing job now to verify nothing unexpected fails. The testing done by the submit repo doesn't cover the tests you have update so I plan to take the change for a spin and make sure the correct tests are run and verified in Mach 5. I've also tested hotspot/jtreg/:hotspot_serviceability and jdk/:jdk_tools on my laptop. I did not see any errors / failures which are related to this change. I also ran some local tests on this and it looks good. Also a question about the change. Why do we need a special flag for CMS? I see that the original bug report refers to the flag as being a way to turn on and off the feature but the current implementation only consider the flag for CMS. http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/016774.html Originally, STW phases (Remark and Cleanup) at G1 are not counted in jstat FGC column. So I think we need not to control the behavior of PerfCounter for G1. I agree, for G1 this should not be controlled. Maybe I was a bit unclear, I was wondering why we want to control it for CMS. I think either we should change the behavior without guarding it by a flag or just skip updating CMS (and leave the pauses in FGC). If we do the change for CMS, we should probably also do a CSR, but that should be fairly straight forward. I also found the old review thread where Jon M had the same comment (removing the flag) and it looks like all agreed on that: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017118.html Thanks, Stefan Thanks, Yasumasa Thanks, Stefan On 2018-02-01 14:58, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ This change has been passed Mach 5 via submit repo: http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180201-0805-10101 Thanks, Yasumasa On 2017/11/01 22:02, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ Also I need JPRT results of this change. Could you cooperate? Thanks, Yasumasa On 2017/09/27 0:08, Yasumasa Suenaga wrote: Hi all, I uploaded new webrev to be adapted to jdk10/hs: http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ I want to check this patch via JPRT, but I cannot access it. Could you cooperate? Thanks, yasumasa On 2017/09/21 7:46, Yasumasa Suenaga wrote: PING: Have you checked this issue? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ Yasumasa On 2017/07/01 23:44, Yasumasa Suenaga wrote: PING: Have you checked this issue? Yasumasa On 2017/06/14 13:22, Yasumasa Suenaga wrote: Hi all, I changed PerfCounter to show CGC STW phase in jstat in JDK-8151674. However, it occurred several jtreg test failure, so it was back-outed. I want to resume to work for this issue. http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ These changes are work fine on jtreg test as below: hotspot/test/serviceability/tmtools/jstat jdk/test/sun/tools Since JDK 9, default GC algorithm is set to G1. So I think this chan
Re: PING: RFR: JDK-8153333: [REDO] STW phases at Concurrent GC should count in PerfCounter
On 2018-02-03 06:40, Yasumasa Suenaga wrote: On 2018/02/02 23:38, Stefan Johansson wrote: Hi Yasumasa, The changes doesn't apply clean on the latest jdk/hs, can you provide an updated webrev? I uploaded webrev for jdk-hs: cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.05/ Thanks, I've kicked off a testing job now to verify nothing unexpected fails. The testing done by the submit repo doesn't cover the tests you have update so I plan to take the change for a spin and make sure the correct tests are run and verified in Mach 5. I've also tested hotspot/jtreg/:hotspot_serviceability and jdk/:jdk_tools on my laptop. I did not see any errors / failures which are related to this change. I also ran some local tests on this and it looks good. Also a question about the change. Why do we need a special flag for CMS? I see that the original bug report refers to the flag as being a way to turn on and off the feature but the current implementation only consider the flag for CMS. http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/016774.html Originally, STW phases (Remark and Cleanup) at G1 are not counted in jstat FGC column. So I think we need not to control the behavior of PerfCounter for G1. I agree, for G1 this should not be controlled. Maybe I was a bit unclear, I was wondering why we want to control it for CMS. I think either we should change the behavior without guarding it by a flag or just skip updating CMS (and leave the pauses in FGC). If we do the change for CMS, we should probably also do a CSR, but that should be fairly straight forward. I also found the old review thread where Jon M had the same comment (removing the flag) and it looks like all agreed on that: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017118.html Thanks, Stefan Thanks, Yasumasa Thanks, Stefan On 2018-02-01 14:58, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ This change has been passed Mach 5 via submit repo: http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180201-0805-10101 Thanks, Yasumasa On 2017/11/01 22:02, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ Also I need JPRT results of this change. Could you cooperate? Thanks, Yasumasa On 2017/09/27 0:08, Yasumasa Suenaga wrote: Hi all, I uploaded new webrev to be adapted to jdk10/hs: http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ I want to check this patch via JPRT, but I cannot access it. Could you cooperate? Thanks, yasumasa On 2017/09/21 7:46, Yasumasa Suenaga wrote: PING: Have you checked this issue? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ Yasumasa On 2017/07/01 23:44, Yasumasa Suenaga wrote: PING: Have you checked this issue? Yasumasa On 2017/06/14 13:22, Yasumasa Suenaga wrote: Hi all, I changed PerfCounter to show CGC STW phase in jstat in JDK-8151674. However, it occurred several jtreg test failure, so it was back-outed. I want to resume to work for this issue. http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ These changes are work fine on jtreg test as below: hotspot/test/serviceability/tmtools/jstat jdk/test/sun/tools Since JDK 9, default GC algorithm is set to G1. So I think this change is useful to watch GC behavior through jstat. I cannot access JPRT. Could you help? Thanks, Yasumasa
Re: PING: RFR: JDK-8153333: [REDO] STW phases at Concurrent GC should count in PerfCounter
Hi Yasumasa, The changes doesn't apply clean on the latest jdk/hs, can you provide an updated webrev? The testing done by the submit repo doesn't cover the tests you have update so I plan to take the change for a spin and make sure the correct tests are run and verified in Mach 5. Also a question about the change. Why do we need a special flag for CMS? I see that the original bug report refers to the flag as being a way to turn on and off the feature but the current implementation only consider the flag for CMS. Thanks, Stefan On 2018-02-01 14:58, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ This change has been passed Mach 5 via submit repo: http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-815-20180201-0805-10101 Thanks, Yasumasa On 2017/11/01 22:02, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ Also I need JPRT results of this change. Could you cooperate? Thanks, Yasumasa On 2017/09/27 0:08, Yasumasa Suenaga wrote: Hi all, I uploaded new webrev to be adapted to jdk10/hs: http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.04/ I want to check this patch via JPRT, but I cannot access it. Could you cooperate? Thanks, yasumasa On 2017/09/21 7:46, Yasumasa Suenaga wrote: PING: Have you checked this issue? http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ Yasumasa On 2017/07/01 23:44, Yasumasa Suenaga wrote: PING: Have you checked this issue? Yasumasa On 2017/06/14 13:22, Yasumasa Suenaga wrote: Hi all, I changed PerfCounter to show CGC STW phase in jstat in JDK-8151674. However, it occurred several jtreg test failure, so it was back-outed. I want to resume to work for this issue. http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-815/webrev.03/jdk/ These changes are work fine on jtreg test as below: hotspot/test/serviceability/tmtools/jstat jdk/test/sun/tools Since JDK 9, default GC algorithm is set to G1. So I think this change is useful to watch GC behavior through jstat. I cannot access JPRT. Could you help? Thanks, Yasumasa
Re: PING: RFR: JDK-8163581: Heap Parameters in HSDB cannot handle G1CollectedHeap
Hi Yasumasa, Your change looks good. There might be more information that we want to add here in the future, but this is a good start. I would personally prefer the region info on its own line, but I don't have a very strong opinion. I can sponsor the change. Thanks, Stefan On 2016-08-23 15:17, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? Yasumasa On 2016/08/16 21:31, Yasumasa Suenaga wrote: Hi all, Have you checked changes for this issue? http://cr.openjdk.java.net/~ysuenaga/JDK-8163581/webrev.01/hotspot/ http://cr.openjdk.java.net/~ysuenaga/JDK-8163581/webrev.01/jdk/ For example, ParallelScavengeHeap.java overrides printOn() method. This method shows addresses of each memory regions. My proposal shows memory region of G1CollectedHeap and region size. Please review and sponsor for it. If it is not enough, please tell me. Thanks, Yasumasa On 2016/08/11 23:15, Yasumasa Suenaga wrote: Hi Jini, I added this check in new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8163581/webrev.01/jdk/ Thanks, Yasumasa On 2016/08/11 15:33, Jini Susan George wrote: Thank you for this fix, Yasumasa. This issue was manifested through the 'universe' command in clhsdb also, and your change fixes that. It would be great if you could modify BasicLauncherTest.java to have some testing done for this also. Thank you, Jini (Not a Reviewer). -Original Message- From: David Holmes Sent: Thursday, August 11, 2016 10:03 AM To: Yasumasa Suenaga; serviceability-dev@openjdk.java.net; hotspot-gc- d...@openjdk.java.net Subject: Re: RFR: JDK-8163581: Heap Parameters in HSDB cannot handle G1CollectedHeap Hi Yasumasa, Adding in GC folk. I agree with the need to override the printOn method, but can't comment on the details of what you are actually printing. Thanks, David On 10/08/2016 10:20 PM, Yasumasa Suenaga wrote: Hi all, When I chose "Heap Parameters" menu in HSDB, I got following value: Heap Parameters: unknown subtype of CollectedHeap @ 0x7f4d4c030510 (0x0006c6e0,0x0007c000) I think it should not be "unknown subtype". This value is shown by sun.jvm.hotspot.gc.shared.CollectedHeap#printOn(). This method is overrided in ParallelScavengeHeap and GenCollectedHeap. However, G1CollectedHeap does not override. I think G1CollectedHeap should override this method. I uploaded a webrev for this issue. Could you review it? http://cr.openjdk.java.net/~ysuenaga/JDK-8163581/webrev.00/ I'm jdk 9 committer, but I cannot access JPRT. So I need a sponsor. Thanks, Yasumasa
Re: RFR(S): 8152989: serviceability/tmtools/jstat/GcCauseTest02.java fails with OOME
Looks good, StefanJ On 2016-04-06 11:32, Per Liden wrote: Summary: This patch updates the tests in serviceability/tmtools/jstat/ to use a fixed (and relatively small) heap size. Without this these tests tend to run out of memory on machines with VM overcommit disabled. This has happened in nightly testing. The patch also moves a misplaced @ignore tag and cleans out an unused method. Testing: Without this patch I can easily get these tests to OOM on my local workstation by just disabling VM overcommit. They pass with this patch applied. Bug: https://bugs.openjdk.java.net/browse/JDK-8152989 Webrev: http://cr.openjdk.java.net/~pliden/8152989/webrev.0/ cheers, Per
