Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Hi Magnus, >> I got reviews for shared-build, but an earlier version, so maybe makes >> sense to look over this again. Erik J, Magnus? > > Build changes look good. Thanks, Magnus! Roman
Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Hi Roman,
On 12/3/18 8:27 PM, Roman Kennke wrote:
Round 5 of Shenandoah review includes:
- A fix for the @requires tag in TestFullGCCountTest.java. It should be
correct now. We believe the CMS @requires was also not quite right and
fixed it the same.
It reads now: Don't run this test if:
- Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is
true, as set by harness
- Actual GC set by harness is Shenandoah *and*
ExplicitGCInvokesConcurrent is not set false by harness (it's true by
default in Shenandoah, so this needs to be double-inverteed).
The @requires for CMS was wrong before (we think), because it would also
filter defaultGC + ExplicitGCInvokesConcurrent.
- Sorting of macros was fixed, as was pointed out by Per
- Some stuff was added to SA, as suggested by Jini
- Rebased on most current jdk/jdk code
Webrevs:
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/
I also need reviews from GC reviewers for the CSR:
https://bugs.openjdk.java.net/browse/JDK-8214349
I already got reviews for:
[x] shared-runtime (coleenp)
[x] shared-compiler (kvn)
I got reviews for shared-build, but an earlier version, so maybe makes
sense to look over this again. Erik J, Magnus?
I still need approvals for:
[ ] shared-build (kvn, erikj, ihse, pliden)
[ ] shared-gc (pliden, kbarrett)
[ ] shared-serviceability (jgeorge, pliden)
[ ] shared-tests (lmesnik, pliden)
The above parts look good to me. Reviewed.
Just one tiny nit (and I don't need to see a new webrev for this):
In src/hotspot/share/gc/shared/gcCause.cpp you have this:
+case _shenandoah_allocation_failure_evac:
+ return "Allocation Failure During Evac";
+
+case _shenandoah_stop_vm:
+ return "Stopping VM";
+
+case _shenandoah_concurrent_gc:
+ return "Shenandoah Concurrent GC";
+
+case _shenandoah_traversal_gc:
+ return "Shenandoah Traversal GC";
+
+case _shenandoah_upgrade_to_full_gc:
+ return "Shenandoah Upgrade To Full GC";
+
And in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GCCause.java
you have this:
+ _shenandoah_stop_vm ("Stop VM"),
+ _shenandoah_allocation_failure_evac ("Allocation Failure During
Evacuation"),
+ _shenandoah_concurrent_gc ("Concurrent GC"),
+ _shenandoah_traversal_gc ("Traversal GC"),
+ _shenandoah_upgrade_to_full_gc ("Upgrade to Full GC"),
It would be good to have the exact same strings in both places. There
are currently small differences in all of them. "Evac" vs "Evacuation",
"Stop" vs "Stopping", "Shenandoah" vs "", etc.
May I also suggest that you skip "Shenandoah" in things like "Shenandoah
Concurrent GC" as I kind of think it's implied by the context. But I
also know that CMS/G1 isn't consistent on this point. An alternative
would be to add "Shenandoah" to all of the strings to keep things
consistent, but I'm not sure I like that better. You decide.
[ ] shenandoah-gc
[ ] shenandoah-tests
I haven't looked very much on these parts, and I didn't plan to do so in
detail right now. I think it's fine of the folks that have been working
on the Shenandoah code reviewed this.
cheers,
Per
Thanks for your patience and ongoing support!
Cheers,
Roman
Hi all,
here comes round 4 of Shenandoah upstreaming review:
This includes fixes for the issues that Per brought up:
- Verify and gracefully reject dangerous flags combinations that
disables required barriers
- Revisited @requires filters in tests
- Trim unused code from Shenandoah's SA impl
- Move ShenandoahGCTracer to gc/shenandoah
- Fix ordering of GC names in various files
- Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/
Thanks everybody for taking time to review this!
Roman
Hello all,
Thanks so far for all the reviews and support!
I forgot to update the 'round' yesterday. We are in round 3 now :-)
Also, I fixed the numbering of my webrevs to match the review-round. ;-)
Things we've changed today:
- We moved shenandoah-specific code out of .ad files into our own .ad
files under gc/shenandoah (in shenandoah-gc), how cool is that? This
requires an addition in build machinery though, see
make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build).
- Improved zero-disabling and build-code-simplification as suggested by
Magnus and Per
- Cleaned up some leftovers in C2
- Improved C2 loop opts code by introducing another APIs in
BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp.
- I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now.
- We would all very much prefer to keep ShenandoahXYZNode names, as
noted earlier. This stuff is Shenandoah-specific, so let's just call it
that.
- Rehashed Shenandoah tests (formatting, naming, directory layout, etc)
- Rebased on jdk-12+22
- Question: let us know if you need separate RFE for the new BSC2 APIs?
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/
Thanks,
Roman
Alright, we fixed:
- The
Re: RFR(XS): 8214063: [AIX] Disable symbol visibility flags
Hi Adam, I've just pushed the change: http://hg.openjdk.java.net/jdk/jdk/rev/fc54d27e58d8 Best regards, Volker On Thu, Nov 29, 2018 at 5:54 PM Adam Farley8 wrote: > > Hi All, > > The build passed on xlC 13.1 with the makefile patch I proposed (good catch > on the comments Volkar!). > > With Volkar, Erik, Matthias, and Magnus all approving the change, it sounds > like we're good to merge! > > Volkar, can you do the honours? > > Best Regards > > Adam Farley > IBM Runtimes > > P.S. I approve the change too. ;) > > > Volker Simonis wrote on 29/11/2018 11:54:33: > > > From: Volker Simonis > > To: Magnus Ihse Bursie > > Cc: build-dev , ppc-aix-port- > > d...@openjdk.java.net, adam.far...@uk.ibm.com > > Date: 29/11/2018 11:54 > > Subject: Re: RFR(XS): 8214063: [AIX] Disable symbol visibility flags > > > > On Thu, Nov 29, 2018 at 12:20 PM Magnus Ihse Bursie > > wrote: > > > > > > On 2018-11-27 16:33, Volker Simonis wrote: > > > > > > > Hi, > > > > > > > > can I please have a review for the following trivial change which > > > > simply disables the symbol visibility flags on AIX: > > > > > > > > https://urldefense.proofpoint.com/v2/url? > > u=http-3A__cr.openjdk.java.net_-7Esimonis_webrevs_2018_8214063_=DwIBaQ=jf_iaSHvJObTbx- > > siA1ZOg=P5m8KWUXJf- > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > YxF4UGVrVEIqu_wVvivFVUA=DptrWUUtJCcpUCbCWkkBOeFJCVk5im3hm9T_DcD0Jd8= > > > > https://urldefense.proofpoint.com/v2/url? > > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8214063=DwIBaQ=jf_iaSHvJObTbx- > > siA1ZOg=P5m8KWUXJf- > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > YxF4UGVrVEIqu_wVvivFVUA=jBFABkJb5E5W9K8pMX794-3gnpLfPyi3oASA1kizQ7A= > > > Looks good to me. I am sorry for the mess I caused by optimisically > > > trying to fix things on a platform I could not compile on... :( > > > > > > > Thanks for the review and don't worry! We really appreciate your > > continued help. It's really us who should have tested and spotted the > > problems earlier :) > > > > Regards, > > Volker > > > > > This also reminds me that the visibility flags *really* should move into > > > configure/spec, not be sprinkled like this in the make files. > > > > > > /Magnus > > > > > > > > Change "8202322: AIX: symbol visibility flags not support on xlc 12.1" > > > > [1] blindly introduced these flags for all xlC compiler versions > > > > > 12.1 without ever testing it (which should not have happened). Now > > > > that people are starting to really use xlC 13 it turns out that there > > > > is more to do than just enabling the flags. This future work is > > > > covered by "8204541: Correctly support AIX xlC 13.1 symbol visibility > > > > flags". > > > > > > > > Thank you and best regards, > > > > Volker > > > > > > > > [1] https://urldefense.proofpoint.com/v2/url? > > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8202322=DwIBaQ=jf_iaSHvJObTbx- > > siA1ZOg=P5m8KWUXJf- > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > YxF4UGVrVEIqu_wVvivFVUA=pd7-rH7OPxeaq2g6S0dQPmb_3-8PLi8JZFKcP_Abp6Q= > > > > [2] https://urldefense.proofpoint.com/v2/url? > > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8204541=DwIBaQ=jf_iaSHvJObTbx- > > siA1ZOg=P5m8KWUXJf- > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > YxF4UGVrVEIqu_wVvivFVUA=q7KHUASpF-opdcLXbTTUT1bPoKrkTeaHTtd7c2jN4rc= > > > > > > > Unless stated otherwise above: > IBM United Kingdom Limited - Registered in England and Wales with number > 741598. > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: RFR: 8212794 IBM-964 is required for AIX default charset
On 2018-12-04 04:30, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext
package
I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I'm not sure I'm fully following the template intricacies here, but
would this not be solved if IBM33722.java was made a template as well?
Then you could do
import $PACKAGE$. SimpleEUCEncoder
Or so I'd think.
/Magnus
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
I'll changed.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?
Yes, it's no need.
Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/
Thanks,
Ichiroh Takiguchi
On 2018-12-04 05:50, Roger Riggs wrote:
Hi Ichiroh,
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?
Regards, Roger
On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:
On 2018-11-30 10:49, Ichiroh Takiguchi wrote:
Hello.
Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review
it too.
/Magnus
I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)
On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.
Without fix
$ jshell
| Welcome to JShell -- Version 12-ea
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964
jshell> /exit
| Goodbye
$
==
With fix
==
$ jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
On AIX platform
IBM964 is moved to java.base module from jdk.charset module.
==
$ LANG=zh_TW jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-11-29 22:39, Ichiroh Takiguchi wrote:
Hello Alan & Magnus.
Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.
I think I should not put 2 kind of modification.
For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)
I'll submit code review again.
Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix
I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java
My build machine is not so fast, after test is done.
I'll post new code.
Thanks,
Ichiroh Takiguchi
On 2018-11-28 19:10, Magnus Ihse Bursie wrote:
On 2018-11-28 10:36, Alan Bateman wrote:
On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character
sets in the build in general. :-( I'd really like to modernize
it. I have a, slightly fuzzy, laundry list of things I want to
fix from a build perspective, but I'm not sure of what
"external"
Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Hi Jini, > Thank you for making the changes. The SA portion looks good to me. Thank you! > One > nit though: > > In > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/HeapSummary.java, > in printGCAlgorithm(), does displaying the nbr of Parallel GC threads > not make sense for Shenandoah (like it is for G1, ZGC, etc)? I suppose it does. I will add it. Thanks, Roman > Thank you, > Jini. > > > On 12/4/2018 12:57 AM, Roman Kennke wrote: >> Round 5 of Shenandoah review includes: >> - A fix for the @requires tag in TestFullGCCountTest.java. It should be >> correct now. We believe the CMS @requires was also not quite right and >> fixed it the same. >> >> It reads now: Don't run this test if: >> - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is >> true, as set by harness >> - Actual GC set by harness is Shenandoah *and* >> ExplicitGCInvokesConcurrent is not set false by harness (it's true by >> default in Shenandoah, so this needs to be double-inverteed). >> >> The @requires for CMS was wrong before (we think), because it would also >> filter defaultGC + ExplicitGCInvokesConcurrent. >> >> - Sorting of macros was fixed, as was pointed out by Per >> - Some stuff was added to SA, as suggested by Jini >> - Rebased on most current jdk/jdk code >> >> Webrevs: >> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/ >> >> I also need reviews from GC reviewers for the CSR: >> https://bugs.openjdk.java.net/browse/JDK-8214349 >> >> I already got reviews for: >> [x] shared-runtime (coleenp) >> [x] shared-compiler (kvn) >> >> I got reviews for shared-build, but an earlier version, so maybe makes >> sense to look over this again. Erik J, Magnus? >> >> I still need approvals for: >> [ ] shared-build (kvn, erikj, ihse, pliden) >> [ ] shared-gc (pliden, kbarrett) >> [ ] shared-serviceability (jgeorge, pliden) >> [ ] shared-tests (lmesnik, pliden) >> [ ] shenandoah-gc >> [ ] shenandoah-tests >> >> >> Thanks for your patience and ongoing support! >> >> Cheers, >> Roman >> >>> Hi all, >>> >>> here comes round 4 of Shenandoah upstreaming review: >>> >>> This includes fixes for the issues that Per brought up: >>> - Verify and gracefully reject dangerous flags combinations that >>> disables required barriers >>> - Revisited @requires filters in tests >>> - Trim unused code from Shenandoah's SA impl >>> - Move ShenandoahGCTracer to gc/shenandoah >>> - Fix ordering of GC names in various files >>> - Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W >>> >>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/ >>> >>> Thanks everybody for taking time to review this! >>> Roman >>> Hello all, Thanks so far for all the reviews and support! I forgot to update the 'round' yesterday. We are in round 3 now :-) Also, I fixed the numbering of my webrevs to match the review-round. ;-) Things we've changed today: - We moved shenandoah-specific code out of .ad files into our own .ad files under gc/shenandoah (in shenandoah-gc), how cool is that? This requires an addition in build machinery though, see make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build). - Improved zero-disabling and build-code-simplification as suggested by Magnus and Per - Cleaned up some leftovers in C2 - Improved C2 loop opts code by introducing another APIs in BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp. - I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now. - We would all very much prefer to keep ShenandoahXYZNode names, as noted earlier. This stuff is Shenandoah-specific, so let's just call it that. - Rehashed Shenandoah tests (formatting, naming, directory layout, etc) - Rebased on jdk-12+22 - Question: let us know if you need separate RFE for the new BSC2 APIs? http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/ Thanks, Roman > Alright, we fixed: > - The minor issues that Kim reported in shared-gc > - A lot of fixes in shared-tests according to Leonid's review > - Disabled SA heapdumping similar to ZGC as Per suggested > > Some notes: > Leonid: test/hotspot/jtreg/gc/TestFullGCCount.java was actually > correct. The @requires there means to exclude runs with both CMS and > ExplicitGCInvokesConcurrent at the same time, because that would be > (expectedly) failing. It can run CMS, default GC and any other GC just > fine. Adding the same clause for Shenandoah means the same, and > filters > the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I > made the condition a bit clearer by avoiding triple-negation. > > See: > http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html > > > Per: Disabling the SA part for heapdumping makes 2 tests fail: > -
Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Hi Per,
>> Round 5 of Shenandoah review includes:
>> - A fix for the @requires tag in TestFullGCCountTest.java. It should be
>> correct now. We believe the CMS @requires was also not quite right and
>> fixed it the same.
>>
>> It reads now: Don't run this test if:
>> - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is
>> true, as set by harness
>> - Actual GC set by harness is Shenandoah *and*
>> ExplicitGCInvokesConcurrent is not set false by harness (it's true by
>> default in Shenandoah, so this needs to be double-inverteed).
>>
>> The @requires for CMS was wrong before (we think), because it would also
>> filter defaultGC + ExplicitGCInvokesConcurrent.
>>
>> - Sorting of macros was fixed, as was pointed out by Per
>> - Some stuff was added to SA, as suggested by Jini
>> - Rebased on most current jdk/jdk code
>>
>> Webrevs:
>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/
>>
>> I also need reviews from GC reviewers for the CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8214349
>>
>> I already got reviews for:
>> [x] shared-runtime (coleenp)
>> [x] shared-compiler (kvn)
>>
>> I got reviews for shared-build, but an earlier version, so maybe makes
>> sense to look over this again. Erik J, Magnus?
>>
>> I still need approvals for:
>> [ ] shared-build (kvn, erikj, ihse, pliden)
>> [ ] shared-gc (pliden, kbarrett)
>> [ ] shared-serviceability (jgeorge, pliden)
>> [ ] shared-tests (lmesnik, pliden)
>
> The above parts look good to me. Reviewed.
Great! Thanks!
> Just one tiny nit (and I don't need to see a new webrev for this):
>
> In src/hotspot/share/gc/shared/gcCause.cpp you have this:
>
> + case _shenandoah_allocation_failure_evac:
> + return "Allocation Failure During Evac";
> +
> + case _shenandoah_stop_vm:
> + return "Stopping VM";
> +
> + case _shenandoah_concurrent_gc:
> + return "Shenandoah Concurrent GC";
> +
> + case _shenandoah_traversal_gc:
> + return "Shenandoah Traversal GC";
> +
> + case _shenandoah_upgrade_to_full_gc:
> + return "Shenandoah Upgrade To Full GC";
> +
>
> And in
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GCCause.java
> you have this:
>
> + _shenandoah_stop_vm ("Stop VM"),
> + _shenandoah_allocation_failure_evac ("Allocation Failure During
> Evacuation"),
> + _shenandoah_concurrent_gc ("Concurrent GC"),
> + _shenandoah_traversal_gc ("Traversal GC"),
> + _shenandoah_upgrade_to_full_gc ("Upgrade to Full GC"),
>
> It would be good to have the exact same strings in both places. There
> are currently small differences in all of them. "Evac" vs "Evacuation",
> "Stop" vs "Stopping", "Shenandoah" vs "", etc.
>
> May I also suggest that you skip "Shenandoah" in things like "Shenandoah
> Concurrent GC" as I kind of think it's implied by the context. But I
> also know that CMS/G1 isn't consistent on this point. An alternative
> would be to add "Shenandoah" to all of the strings to keep things
> consistent, but I'm not sure I like that better. You decide.
I'm addressing it here:
http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-December/008572.html
>> [ ] shenandoah-gc
>> [ ] shenandoah-tests
>
> I haven't looked very much on these parts, and I didn't plan to do so in
> detail right now. I think it's fine of the folks that have been working
> on the Shenandoah code reviewed this.
Yes, I think so too (except maybe stuff like shenandoah_globals.hpp, but
that was already reviewed I think).
Thanks for reviewing, Per!
Roman
> cheers,
> Per
>
>>
>>
>> Thanks for your patience and ongoing support!
>>
>> Cheers,
>> Roman
>>
>>> Hi all,
>>>
>>> here comes round 4 of Shenandoah upstreaming review:
>>>
>>> This includes fixes for the issues that Per brought up:
>>> - Verify and gracefully reject dangerous flags combinations that
>>> disables required barriers
>>> - Revisited @requires filters in tests
>>> - Trim unused code from Shenandoah's SA impl
>>> - Move ShenandoahGCTracer to gc/shenandoah
>>> - Fix ordering of GC names in various files
>>> - Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W
>>>
>>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/
>>>
>>> Thanks everybody for taking time to review this!
>>> Roman
>>>
Hello all,
Thanks so far for all the reviews and support!
I forgot to update the 'round' yesterday. We are in round 3 now :-)
Also, I fixed the numbering of my webrevs to match the review-round.
;-)
Things we've changed today:
- We moved shenandoah-specific code out of .ad files into our own .ad
files under gc/shenandoah (in shenandoah-gc), how cool is that? This
requires an addition in build machinery though, see
make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build).
- Improved zero-disabling and build-code-simplification as suggested by
Magnus and Per
- Cleaned up some leftovers in C2
- Improved C2 loop opts code by introducing
Re: RFR: 8212794 IBM-964 is required for AIX default charset
Hello Magnus.
IBM33722 should be in sun.nio.cs.ext package on jdk.charset module
Because it's not used for default encoding on AIX platform.
In case of template file, build tool checks
make/data/charsetmapping/stdcs-aix file for this case.
If class name is there, it will be migrated to sun.nio.cs package.
About IBM33722,
If IBM33722 is moved to sun.nio.cs package also,
"import sun.nio.cs.*;" is no need on IBM33722.java
If IBM33722 is not in stdcs-aix,
"import sun.nio.cs.*;" is still required on IBM33722.java
Thanks,
Ichiroh Takiguchi
On 2018-12-04 19:42, Magnus Ihse Bursie wrote:
On 2018-12-04 04:30, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers
it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext
package
I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I'm not sure I'm fully following the template intricacies here, but
would this not be solved if IBM33722.java was made a template as well?
Then you could do
import $PACKAGE$. SimpleEUCEncoder
Or so I'd think.
/Magnus
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
I'll changed.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Yes, it's no need.
Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/
Thanks,
Ichiroh Takiguchi
On 2018-12-04 05:50, Roger Riggs wrote:
Hi Ichiroh,
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Regards, Roger
On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:
On 2018-11-30 10:49, Ichiroh Takiguchi wrote:
Hello.
Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review
it too.
/Magnus
I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)
On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.
Without fix
$ jshell
| Welcome to JShell -- Version 12-ea
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964
jshell> /exit
| Goodbye
$
==
With fix
==
$ jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
On AIX platform
IBM964 is moved to java.base module from jdk.charset module.
==
$ LANG=zh_TW jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-11-29 22:39, Ichiroh Takiguchi wrote:
Hello Alan & Magnus.
Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.
I think I should not put 2 kind of modification.
For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)
I'll submit code review again.
Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix
I'll not touch
Re: RFR: 8212794 IBM-964 is required for AIX default charset
Hi Ichiroh,
On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext
package
I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I understand, it is because IBM33722 may or may not be in the same
package as SimpleEUCEncoder.
It is SimpleEUCEncoder that may be in a different package, not IBM33722.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
I'll changed.
226-227: add a space before "{" to have the same style as line 210.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?
Yes, it's no need.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/
Thanks, looks fine.
Regards, Roger
Thanks,
Ichiroh Takiguchi
On 2018-12-04 05:50, Roger Riggs wrote:
Hi Ichiroh,
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?
Regards, Roger
On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:
On 2018-11-30 10:49, Ichiroh Takiguchi wrote:
Hello.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review
it too.
/Magnus
I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)
On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.
Without fix
$ jshell
| Welcome to JShell -- Version 12-ea
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964
jshell> /exit
| Goodbye
$
==
With fix
==
$ jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
On AIX platform
IBM964 is moved to java.base module from jdk.charset module.
==
$ LANG=zh_TW jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-11-29 22:39, Ichiroh Takiguchi wrote:
Hello Alan & Magnus.
Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.
I think I should not put 2 kind of modification.
For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)
I'll submit code review again.
Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix
I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java
My build machine is not so fast, after test is done.
I'll post new code.
Thanks,
Ichiroh Takiguchi
On 2018-11-28 19:10, Magnus Ihse Bursie wrote:
On 2018-11-28 10:36, Alan Bateman wrote:
On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character
sets in the build in general. :-( I'd really like to modernize
it. I have a, slightly fuzzy, laundry list of things I
Re: RFR(XS): 8214063: [AIX] Disable symbol visibility flags
Thanks Volker. :) Best Regards Adam Farley IBM Runtimes Volker Simonis wrote on 04/12/2018 08:30:58: > From: Volker Simonis > To: adam.far...@uk.ibm.com > Cc: build-dev , Magnus Ihse Bursie > , ppc-aix-port-...@openjdk.java.net > Date: 04/12/2018 08:31 > Subject: Re: RFR(XS): 8214063: [AIX] Disable symbol visibility flags > > Hi Adam, > > I've just pushed the change: > > INVALID URI REMOVED > u=http-3A__hg.openjdk.java.net_jdk_jdk_rev_fc54d27e58d8=DwIBaQ=jf_iaSHvJObTbx- > siA1ZOg=P5m8KWUXJf- > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=NhALBBoEo6HsbPIjB8bJJj30UR8DRP- > PuJckMbmJvA0=gLabfGk2XJdLwimruwQdLAmjBXtCueO7qR01_xw5wuw= > > Best regards, > Volker > On Thu, Nov 29, 2018 at 5:54 PM Adam Farley8 wrote: > > > > Hi All, > > > > The build passed on xlC 13.1 with the makefile patch I proposed > (good catch on the comments Volkar!). > > > > With Volkar, Erik, Matthias, and Magnus all approving the change, > it sounds like we're good to merge! > > > > Volkar, can you do the honours? > > > > Best Regards > > > > Adam Farley > > IBM Runtimes > > > > P.S. I approve the change too. ;) > > > > > > Volker Simonis wrote on 29/11/2018 11:54:33: > > > > > From: Volker Simonis > > > To: Magnus Ihse Bursie > > > Cc: build-dev , ppc-aix-port- > > > d...@openjdk.java.net, adam.far...@uk.ibm.com > > > Date: 29/11/2018 11:54 > > > Subject: Re: RFR(XS): 8214063: [AIX] Disable symbol visibility flags > > > > > > On Thu, Nov 29, 2018 at 12:20 PM Magnus Ihse Bursie > > > wrote: > > > > > > > > On 2018-11-27 16:33, Volker Simonis wrote: > > > > > > > > > Hi, > > > > > > > > > > can I please have a review for the following trivial change which > > > > > simply disables the symbol visibility flags on AIX: > > > > > > > > > > INVALID URI REMOVED > > > > u=http-3A__cr.openjdk.java.net_-7Esimonis_webrevs_2018_8214063_=DwIBaQ=jf_iaSHvJObTbx- > > > siA1ZOg=P5m8KWUXJf- > > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > > YxF4UGVrVEIqu_wVvivFVUA=DptrWUUtJCcpUCbCWkkBOeFJCVk5im3hm9T_DcD0Jd8= > > > > > INVALID URI REMOVED > > > > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8214063=DwIBaQ=jf_iaSHvJObTbx- > > > siA1ZOg=P5m8KWUXJf- > > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > > YxF4UGVrVEIqu_wVvivFVUA=jBFABkJb5E5W9K8pMX794-3gnpLfPyi3oASA1kizQ7A= > > > > Looks good to me. I am sorry for the mess I caused by optimisically > > > > trying to fix things on a platform I could not compile on... :( > > > > > > > > > > Thanks for the review and don't worry! We really appreciate your > > > continued help. It's really us who should have tested and spotted the > > > problems earlier :) > > > > > > Regards, > > > Volker > > > > > > > This also reminds me that the visibility flags *really* shouldmove into > > > > configure/spec, not be sprinkled like this in the make files. > > > > > > > > /Magnus > > > > > > > > > > Change "8202322: AIX: symbol visibility flags not support onxlc 12.1" > > > > > [1] blindly introduced these flags for all xlC compiler versions > > > > > > 12.1 without ever testing it (which should not have happened). Now > > > > > that people are starting to really use xlC 13 it turns out that there > > > > > is more to do than just enabling the flags. This future work is > > > > > covered by "8204541: Correctly support AIX xlC 13.1 symbol visibility > > > > > flags". > > > > > > > > > > Thank you and best regards, > > > > > Volker > > > > > > > > > > [1] INVALID URI REMOVED > > > > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8202322=DwIBaQ=jf_iaSHvJObTbx- > > > siA1ZOg=P5m8KWUXJf- > > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > > YxF4UGVrVEIqu_wVvivFVUA=pd7-rH7OPxeaq2g6S0dQPmb_3-8PLi8JZFKcP_Abp6Q= > > > > > [2] INVALID URI REMOVED > > > > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8204541=DwIBaQ=jf_iaSHvJObTbx- > > > siA1ZOg=P5m8KWUXJf- > > > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=6y4Npxy6aG4q8E9Xca-- > > > YxF4UGVrVEIqu_wVvivFVUA=q7KHUASpF-opdcLXbTTUT1bPoKrkTeaHTtd7c2jN4rc= > > > > > > > > > > > Unless stated otherwise above: > > IBM United Kingdom Limited - Registered in England and Wales with > number 741598. > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
RFR: JDK-8214780 Create pandoc package for Windows
As requested in JDK-8179917, we should create a jib package for pandoc on Windows. Bug: https://bugs.openjdk.java.net/browse/JDK-8214780 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8214780-create-pandoc-package-for-windows/webrev.01 /Magnus
bootcycle builds x86_64-linux-gnu?
with jdk-12+22, bootcycle builds fail at least on x86_64-linux-gnu. Is there some place where I can check if this kind of build succeeds for others? thanks, Matthias
Re: bootcycle builds x86_64-linux-gnu?
On 12/4/18 2:24 PM, Matthias Klose wrote: > with jdk-12+22, bootcycle builds fail at least on x86_64-linux-gnu. Is there > some place where I can check if this kind of build succeeds for others? Just did this on jdk/jdk tip, and it passed: $ CONF=linux-x86_64-server-fastdebug make bootcycle-images ... Creating CDS archive for jdk image Stopping sjavac server Finished building target 'product-images' in configuration 'linux-x86_64-server-fastdebug' Finished building target 'bootcycle-images' in configuration 'linux-x86_64-server-fastdebug' -Aleksey
Re: RFR 8214794 : java.specification.version should be only the major version number
On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Hi Roger, Looks fine. Brian > On Dec 4, 2018, at 8:23 AM, Roger Riggs wrote: > > Including build-dev for the change to GensrcMisc.gmk. > > thx. > > On 12/04/2018 11:16 AM, Roger Riggs wrote: >> Please review correctly setting the java.specification.version property >> with only the major version number. A test is added to ensure the >> java spec version agrees with the major version. >> >> The symptoms are that jtreg would fail with a full version number. >> >> Webrev: >> http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2
Re: RFR: 8212794 IBM-964 is required for AIX default charset
Hello Roger.
Thank you for your suggestion.
Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.04/
Thanks,
Ichiroh Takiguchi
On 2018-12-04 23:36, Roger Riggs wrote:
Hi Ichiroh,
On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers
it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext
package
I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I understand, it is because IBM33722 may or may not be in the same
package as SimpleEUCEncoder.
It is SimpleEUCEncoder that may be in a different package, not
IBM33722.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
I'll changed.
226-227: add a space before "{" to have the same style as line 210.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Yes, it's no need.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/
Thanks, looks fine.
Regards, Roger
Thanks,
Ichiroh Takiguchi
On 2018-12-04 05:50, Roger Riggs wrote:
Hi Ichiroh,
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Regards, Roger
On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:
On 2018-11-30 10:49, Ichiroh Takiguchi wrote:
Hello.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review
it too.
/Magnus
I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)
On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.
Without fix
$ jshell
| Welcome to JShell -- Version 12-ea
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964
jshell> /exit
| Goodbye
$
==
With fix
==
$ jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
On AIX platform
IBM964 is moved to java.base module from jdk.charset module.
==
$ LANG=zh_TW jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-11-29 22:39, Ichiroh Takiguchi wrote:
Hello Alan & Magnus.
Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.
I think I should not put 2 kind of modification.
For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)
I'll submit code review again.
Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix
I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java
My build machine is not so fast, after test is done.
I'll post new code.
Thanks,
Ichiroh Takiguchi
On 2018-11-28 19:10, Magnus Ihse Bursie
Re: RFR: 8212794 IBM-964 is required for AIX default charset
Hi Ichiroh,
Looks fine.
I can sponsor that for you. Tomorrow, though, to allow time for any
other comments.
Thanks, Roger
On 12/04/2018 10:21 AM, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.04/
Thanks,
Ichiroh Takiguchi
On 2018-12-04 23:36, Roger Riggs wrote:
Hi Ichiroh,
On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:
Hello Roger.
Thank you for your suggestion.
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers
it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext
package
I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I understand, it is because IBM33722 may or may not be in the same
package as SimpleEUCEncoder.
It is SimpleEUCEncoder that may be in a different package, not IBM33722.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
I'll changed.
226-227: add a space before "{" to have the same style as line 210.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Yes, it's no need.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/
Thanks, looks fine.
Regards, Roger
Thanks,
Ichiroh Takiguchi
On 2018-12-04 05:50, Roger Riggs wrote:
Hi Ichiroh,
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:
I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
TestIBMBugs:
- Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.
- The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for
33722?
Regards, Roger
On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:
On 2018-11-30 10:49, Ichiroh Takiguchi wrote:
Hello.
Could you review the fix again ?
Bug: https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review
it too.
/Magnus
I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)
On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.
Without fix
$ jshell
| Welcome to JShell -- Version 12-ea
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964
jshell> /exit
| Goodbye
$
==
With fix
==
$ jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
On AIX platform
IBM964 is moved to java.base module from jdk.charset module.
==
$ LANG=zh_TW jshell
| Welcome to JShell -- Version 12-internal
| For an introduction type: /help intro
jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964
jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"
jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964
jshell> /exit
| Goodbye
$
==
I'd like to obtain a sponsor for this issue.
Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.
On 2018-11-29 22:39, Ichiroh Takiguchi wrote:
Hello Alan & Magnus.
Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.
I think I should not put 2 kind of modification.
For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)
I'll submit code review again.
Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix
I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
Re: RFR: JDK-8214780 Create pandoc package for Windows
Looks good. /Erik On 2018-12-04 04:34, Magnus Ihse Bursie wrote: As requested in JDK-8179917, we should create a jib package for pandoc on Windows. Bug: https://bugs.openjdk.java.net/browse/JDK-8214780 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8214780-create-pandoc-package-for-windows/webrev.01 /Magnus
Re: RFR 8214794 : java.specification.version should be only the major version number
Including build-dev for the change to GensrcMisc.gmk. thx. On 12/04/2018 11:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Thanks, Roger
Re: RFR 8214794 : java.specification.version should be only the major version number
The revised webrev looks okay. Mandy On 12/4/18 11:32 AM, Roger Riggs wrote: Hi Mandy, Martin, The new test is unnecessary, the case is covered by java/lang/System/Versions test and uses the stronger comparison for the version numbers. It would not detect the problem unless the version included more than the major version. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ Thanks, Roger On 12/04/2018 01:41 PM, Mandy Chung wrote: On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Hi Mandy, Martin, The new test is unnecessary, the case is covered by java/lang/System/Versions test and uses the stronger comparison for the version numbers. It would not detect the problem unless the version included more than the major version. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ Thanks, Roger On 12/04/2018 01:41 PM, Mandy Chung wrote: On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Looks good. /Erik On 2018-12-04 11:32, Roger Riggs wrote: Hi Mandy, Martin, The new test is unnecessary, the case is covered by java/lang/System/Versions test and uses the stronger comparison for the version numbers. It would not detect the problem unless the version included more than the major version. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ Thanks, Roger On 12/04/2018 01:41 PM, Mandy Chung wrote: On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ Looks okay. I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case. At some point it'd be good to consolidate these two tests. Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group. VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. Mandy
Re: RFR 8214794 : java.specification.version should be only the major version number
Looks good to me, too. /Magnus > 4 dec. 2018 kl. 20:34 skrev Mandy Chung : > > The revised webrev looks okay. > > Mandy > >> On 12/4/18 11:32 AM, Roger Riggs wrote: >> Hi Mandy, Martin, >> >> The new test is unnecessary, the case is covered by >> java/lang/System/Versions test >> and uses the stronger comparison for the version numbers. >> >> It would not detect the problem unless the version included more than the >> major version. >> >> Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/ >> >> Thanks, Roger >> >>> On 12/04/2018 01:41 PM, Mandy Chung wrote: >>> >>> On 12/4/18 8:16 AM, Roger Riggs wrote: Please review correctly setting the java.specification.version property with only the major version number. A test is added to ensure the java spec version agrees with the major version. The symptoms are that jtreg would fail with a full version number. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/ >>> >>> Looks okay. I agree with Martin to go with a stronger assertion without >>> converting into a number. test/jdk/java/lang/System/Versions.java looks >>> like also covering this test case. At some point it'd be good to >>> consolidate these two tests. >>> >>> Nit: in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a >>> relevant group. VERSION_SPECIFICATION can be moved to group with >>> VERSION_CLASSFILE_MAJOR and MINOR. Magnus may have an opinion. >>> >>> Mandy >> >
Re: bootcycle builds x86_64-linux-gnu?
On 4/12/2018 11:24 pm, Matthias Klose wrote: with jdk-12+22, bootcycle builds fail at least on x86_64-linux-gnu. Is there some place where I can check if this kind of build succeeds for others? They don't fail for us, we run them regularly. Cheers, David thanks, Matthias
Re: RFR (round 5), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Hi The shared tests changes looks good for me. Thank you for fixing and testing different combinations. Leonid > On Dec 3, 2018, at 11:10 PM, Roman Kennke wrote: > > Round 5 of Shenandoah review includes: > - A fix for the @requires tag in TestFullGCCountTest.java. It should be > correct now. We believe the CMS @requires was also not quite right and > fixed it the same. > > It reads now: Don't run this test if: > - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is > true, as set by harness > - Actual GC set by harness is Shenandoah *and* > ExplicitGCInvokesConcurrent is not set false by harness (it's true by > default in Shenandoah, so this needs to be double-inverteed). > > The @requires for CMS was wrong before (we think), because it would also > filter defaultGC + ExplicitGCInvokesConcurrent. > > - Sorting of macros was fixed, as was pointed out by Per > - Some stuff was added to SA, as suggested by Jini > - Rebased on most current jdk/jdk code > > Webrevs: > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/ > > I also need reviews from GC reviewers for the CSR: > https://bugs.openjdk.java.net/browse/JDK-8214349 > > I already got reviews for: > [x] shared-runtime (coleenp) > [x] shared-compiler (kvn) > > I got reviews for shared-build, but an earlier version, so maybe makes > sense to look over this again. Erik J, Magnus? > > I still need approvals for: > [ ] shared-build (kvn, erikj, ihse, pliden) > [ ] shared-gc (pliden, kbarrett) > [ ] shared-serviceability (jgeorge, pliden) > [ ] shared-tests (lmesnik, pliden) > [ ] shenandoah-gc > [ ] shenandoah-tests > > > Thanks for your patience and ongoing support! > > Cheers, > Roman > > >> Hi all, >> >> here comes round 4 of Shenandoah upstreaming review: >> >> This includes fixes for the issues that Per brought up: >> - Verify and gracefully reject dangerous flags combinations that >> disables required barriers >> - Revisited @requires filters in tests >> - Trim unused code from Shenandoah's SA impl >> - Move ShenandoahGCTracer to gc/shenandoah >> - Fix ordering of GC names in various files >> - Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W >> >> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/ >> >> Thanks everybody for taking time to review this! >> Roman >> >>> Hello all, >>> >>> Thanks so far for all the reviews and support! >>> >>> I forgot to update the 'round' yesterday. We are in round 3 now :-) >>> Also, I fixed the numbering of my webrevs to match the review-round. ;-) >>> >>> Things we've changed today: >>> - We moved shenandoah-specific code out of .ad files into our own .ad >>> files under gc/shenandoah (in shenandoah-gc), how cool is that? This >>> requires an addition in build machinery though, see >>> make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build). >>> - Improved zero-disabling and build-code-simplification as suggested by >>> Magnus and Per >>> - Cleaned up some leftovers in C2 >>> - Improved C2 loop opts code by introducing another APIs in >>> BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp. >>> - I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now. >>> - We would all very much prefer to keep ShenandoahXYZNode names, as >>> noted earlier. This stuff is Shenandoah-specific, so let's just call it >>> that. >>> - Rehashed Shenandoah tests (formatting, naming, directory layout, etc) >>> - Rebased on jdk-12+22 >>> >>> - Question: let us know if you need separate RFE for the new BSC2 APIs? >>> >>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/ >>> >>> Thanks, >>> Roman >>> Alright, we fixed: - The minor issues that Kim reported in shared-gc - A lot of fixes in shared-tests according to Leonid's review - Disabled SA heapdumping similar to ZGC as Per suggested Some notes: Leonid: test/hotspot/jtreg/gc/TestFullGCCount.java was actually correct. The @requires there means to exclude runs with both CMS and ExplicitGCInvokesConcurrent at the same time, because that would be (expectedly) failing. It can run CMS, default GC and any other GC just fine. Adding the same clause for Shenandoah means the same, and filters the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I made the condition a bit clearer by avoiding triple-negation. See: http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html Per: Disabling the SA part for heapdumping makes 2 tests fail: - test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java - test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java we filter them for Shenandoah now. I'm wondering: how do you get past those with ZGC? See: http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008466.html (Note to Leonid and tests reviewers: I'll add those related filters in
Re: RFR (round 5), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Thanks, Leonid, for reviewing! Roman > Hi > > The shared tests changes looks good for me. Thank you for fixing and testing > different combinations. > > Leonid > >> On Dec 3, 2018, at 11:10 PM, Roman Kennke wrote: >> >> Round 5 of Shenandoah review includes: >> - A fix for the @requires tag in TestFullGCCountTest.java. It should be >> correct now. We believe the CMS @requires was also not quite right and >> fixed it the same. >> >> It reads now: Don't run this test if: >> - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is >> true, as set by harness >> - Actual GC set by harness is Shenandoah *and* >> ExplicitGCInvokesConcurrent is not set false by harness (it's true by >> default in Shenandoah, so this needs to be double-inverteed). >> >> The @requires for CMS was wrong before (we think), because it would also >> filter defaultGC + ExplicitGCInvokesConcurrent. >> >> - Sorting of macros was fixed, as was pointed out by Per >> - Some stuff was added to SA, as suggested by Jini >> - Rebased on most current jdk/jdk code >> >> Webrevs: >> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/ >> >> I also need reviews from GC reviewers for the CSR: >> https://bugs.openjdk.java.net/browse/JDK-8214349 >> >> I already got reviews for: >> [x] shared-runtime (coleenp) >> [x] shared-compiler (kvn) >> >> I got reviews for shared-build, but an earlier version, so maybe makes >> sense to look over this again. Erik J, Magnus? >> >> I still need approvals for: >> [ ] shared-build (kvn, erikj, ihse, pliden) >> [ ] shared-gc (pliden, kbarrett) >> [ ] shared-serviceability (jgeorge, pliden) >> [ ] shared-tests (lmesnik, pliden) >> [ ] shenandoah-gc >> [ ] shenandoah-tests >> >> >> Thanks for your patience and ongoing support! >> >> Cheers, >> Roman >> >> >>> Hi all, >>> >>> here comes round 4 of Shenandoah upstreaming review: >>> >>> This includes fixes for the issues that Per brought up: >>> - Verify and gracefully reject dangerous flags combinations that >>> disables required barriers >>> - Revisited @requires filters in tests >>> - Trim unused code from Shenandoah's SA impl >>> - Move ShenandoahGCTracer to gc/shenandoah >>> - Fix ordering of GC names in various files >>> - Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W >>> >>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/ >>> >>> Thanks everybody for taking time to review this! >>> Roman >>> Hello all, Thanks so far for all the reviews and support! I forgot to update the 'round' yesterday. We are in round 3 now :-) Also, I fixed the numbering of my webrevs to match the review-round. ;-) Things we've changed today: - We moved shenandoah-specific code out of .ad files into our own .ad files under gc/shenandoah (in shenandoah-gc), how cool is that? This requires an addition in build machinery though, see make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build). - Improved zero-disabling and build-code-simplification as suggested by Magnus and Per - Cleaned up some leftovers in C2 - Improved C2 loop opts code by introducing another APIs in BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp. - I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now. - We would all very much prefer to keep ShenandoahXYZNode names, as noted earlier. This stuff is Shenandoah-specific, so let's just call it that. - Rehashed Shenandoah tests (formatting, naming, directory layout, etc) - Rebased on jdk-12+22 - Question: let us know if you need separate RFE for the new BSC2 APIs? http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/ Thanks, Roman > Alright, we fixed: > - The minor issues that Kim reported in shared-gc > - A lot of fixes in shared-tests according to Leonid's review > - Disabled SA heapdumping similar to ZGC as Per suggested > > Some notes: > Leonid: test/hotspot/jtreg/gc/TestFullGCCount.java was actually > correct. The @requires there means to exclude runs with both CMS and > ExplicitGCInvokesConcurrent at the same time, because that would be > (expectedly) failing. It can run CMS, default GC and any other GC just > fine. Adding the same clause for Shenandoah means the same, and filters > the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I > made the condition a bit clearer by avoiding triple-negation. > > See: > http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html > > Per: Disabling the SA part for heapdumping makes 2 tests fail: > - test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java > - test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java > > we filter them for Shenandoah now. I'm wondering: how do you get past > those with ZGC? > > See: >
Re: RFR 8214794 : java.specification.version should be only the major version number
> > LGTM
