Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Jini George]

Hi Roman,

Thank you for making the changes. The SA portion looks good to me. 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)?


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:
- 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
next round).

Vladimir: Roland integrated a bunch of changes to make loop* code look
better. I can tell that we're not done with C2 yet. Can you look over
the code and see what is ok, and especially wh

Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Magnus Ihse Bursie]

> 3 dec. 2018 kl. 20:27 skrev Roman Kennke :
> 
> 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?

Build changes look good. 

/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
 next round).
 
 Vladimir: Roland integrated a bunch of changes to make loo

RFR (round 5), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Roman Kennke]

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
>>> next round).
>>>
>>> Vladimir: Roland integrated a bunch of changes to make loop* code look
>>> better. I can tell that we're not done with C2 yet. Can you look over
>>> the code and see what is ok, and especially what is not ok, so that we
>>> can focus our efforts on the relevant parts?
>>>
>>> Updated set of webrevs:
>>> http://cr.openjdk.java.

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Ichiroh Takiguchi]

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.


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" 
requirements are coming from AIX and the general core-libs agenda 
regarding character sets in general.


I think there is a good opportunity to solve many problems at the 
same time here, as long as everyone agrees on what is the 
preferred outcome.
The support in the

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Roger Riggs]

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" 
requirements are coming from AIX and the general core-libs agenda 
regarding character sets in general.


I think there is a good opportunity to solve many problems at the 
same time here, as long as everyone agrees on what is the 
preferred outcome.
The support in the build to configure the charsets to include in 
java.base on each platform has been working well. Charsets that 
aren't in java.base go into the jdk.charsets service provider 
module and that has been working well too. From the result point 
of view, perhaps, but definitely not from the build perspective. 
;-) But yes, I understand this is functionality that should be kept.
One thing that we lack is some way to add charsets for specific 
platforms and this comes up with the IBM patches where they are 
looking to adding several additional IBM charsets. One starting 
point that we've touched on in several threads here is dropping 
the EBCDIC charsets from the main stream builds. Going there will 
need build support.

So build support for trivially adding specific charsets to specific
platforms? Both to java.base (for AIX) and jdk.charsets, I presume,
then?

Can you expand on the issue of dropping ebcdic? What's the problem
that needs build support?

/Magnus



-Alan

Re: [12] Review Request: 8212680 (JDK12b14/Solaris-sparc) SplashScreen::getSplashScreen call fails with ULE: "libsplashscreen.so: ld.so.1: java: fatal: libz.so.1: open failed: No such file o

[Magnus Ihse Bursie]

Fair enough, that warning is enough. You don't need to add anything else. I'm 
ok with the patch. 

/Magnus

> 3 dec. 2018 kl. 20:37 skrev Sergey Bylokhov :
> 
> Hi, Magnus.
> 
>> This looks good to me too, but as I said before, I still think you should 
>> write a note about this change in UPDATING.txt, so this fix is not lost if 
>> pnglibconf.h were to be re-generated.
> All lines which started by "/*#undef " in the "pnglibconf.h" are commented 
> manually
> and this is an existed note in the UPDATING.txt:
> =
> 4) Special and careful handling of pnglibconf.h
> OpenJDK has a heavily modified copy of pnglibconf.h.
> This is the trickiest part of the whole exercise.
> This file is generated by png at build time.
> Except for the dates and version, you should generally not need to update
> OpenJDK's copy unless the new version of PNG has added rquired new #defines
> that cause problems building.
> You can run configure && make on the downloaded source and compare but we
> do not want to enable any of the WRITE support, and there are many more
> modifications as well.
> So do NOT just copy in a file from the new libpng.
> So lots of reasons to not copy over the new version,
> and instead just tweak the existing one.
> =
> 
> -- 
> Best regards, Sergey.

Re: [12] Review Request: 8212680 (JDK12b14/Solaris-sparc) SplashScreen::getSplashScreen call fails with ULE: "libsplashscreen.so: ld.so.1: java: fatal: libz.so.1: open failed: No such file o

[Sergey Bylokhov]

Hi, Magnus.


This looks good to me too, but as I said before, I still think you should write 
a note about this change in UPDATING.txt, so this fix is not lost if 
pnglibconf.h were to be re-generated.

All lines which started by "/*#undef " in the "pnglibconf.h" are commented 
manually
and this is an existed note in the UPDATING.txt:
=
4) Special and careful handling of pnglibconf.h
OpenJDK has a heavily modified copy of pnglibconf.h.
This is the trickiest part of the whole exercise.
This file is generated by png at build time.
Except for the dates and version, you should generally not need to update
OpenJDK's copy unless the new version of PNG has added rquired new #defines
that cause problems building.
You can run configure && make on the downloaded source and compare but we
do not want to enable any of the WRITE support, and there are many more
modifications as well.
So do NOT just copy in a file from the new libpng.
So lots of reasons to not copy over the new version,
and instead just tweak the existing one.
=

--
Best regards, Sergey.

Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Roman Kennke]

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
>>> next round).
>>>
>>> Vladimir: Roland integrated a bunch of changes to make loop* code look
>>> better. I can tell that we're not done with C2 yet. Can you look over
>>> the code and see what is ok, and especially what is not ok, so that we
>>> can focus our efforts on the relevant parts?
>>>
>>> Updated set of webrevs:
>>> http://cr.openjdk.java.n

RFR: JDK-8214720 Add pandoc filter to improve html man page output

[Magnus Ihse Bursie]

The html output of man pages looks weird due to the metadata header 
required by pandoc to generate proper man pages. We should add a pandoc 
filter for html output as well.


The actual javascript filter implementation was graciously provided to 
me by Jon.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214720
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214720-pandoc-filter-for-html-manpages/webrev.01


/Magnus

Re: RFR: JDK-8213187 Handle libwindowsaccessbridge need to access MSVCRT functions

[Erik Joelsson]

Looks good.

/Erik

On 2018-12-03 09:38, Magnus Ihse Bursie wrote:

From the bug report:

As a follow-up to JDK-8210944, find the best way to handle the needs 
of libwindowsaccessbridge to access the MSVCRT functions. Options 
include reverting JDK-8210944, and copying the MSVCRT*.DLL when 
needed, and/or possible other solutions.


I chose to revert JDK-8210944. I originally did JDK-8210944 to be able 
to finish a larger patch that moved all "CFLAGS := $(CFLAGS_JDKLIB)" 
constructs into SetupJdkLibrary, and then the filter-out logic 
completely screwed that up. But this patch has been lingering and 
bit-rotting while I've spent my time elsewhere, and it will not make 
it into JDK 12. I'll have to reconsider how I do that patch (possibly 
forcing that patch to include not only moving CFLAGS_JDKLIB into 
SetupJdkLibrary, but splitting up CFLAGS_JDKLIB in parts as well.


However, without this revert, we'll see a regression in JDK 12 on the 
loading of accessability libraries on Windows.


Bug: https://bugs.openjdk.java.net/browse/JDK-8213187
Patch inline:
diff --git a/make/lib/Lib-jdk.accessibility.gmk 
b/make/lib/Lib-jdk.accessibility.gmk

--- a/make/lib/Lib-jdk.accessibility.gmk
+++ b/make/lib/Lib-jdk.accessibility.gmk
@@ -41,7 +41,7 @@
 EXTRA_SRC := common, \
 OPTIMIZATION := LOW, \
 DISABLED_WARNINGS_microsoft := 4311 4302 4312, \
-    CFLAGS := $(CFLAGS_JDKLIB) \
+    CFLAGS :=  $(filter-out -MD, $(CFLAGS_JDKLIB)) -MT \
 -DACCESSBRIDGE_ARCH_$2, \
 EXTRA_HEADER_DIRS := \
 include/bridge \

/Magnus

RFR: JDK-8213187 Handle libwindowsaccessbridge need to access MSVCRT functions

[Magnus Ihse Bursie]

From the bug report:

As a follow-up to JDK-8210944, find the best way to handle the needs of 
libwindowsaccessbridge to access the MSVCRT functions. Options include 
reverting JDK-8210944, and copying the MSVCRT*.DLL when needed, and/or 
possible other solutions.


I chose to revert JDK-8210944. I originally did JDK-8210944 to be able 
to finish a larger patch that moved all "CFLAGS := $(CFLAGS_JDKLIB)" 
constructs into SetupJdkLibrary, and then the filter-out logic 
completely screwed that up. But this patch has been lingering and 
bit-rotting while I've spent my time elsewhere, and it will not make it 
into JDK 12. I'll have to reconsider how I do that patch (possibly 
forcing that patch to include not only moving CFLAGS_JDKLIB into 
SetupJdkLibrary, but splitting up CFLAGS_JDKLIB in parts as well.


However, without this revert, we'll see a regression in JDK 12 on the 
loading of accessability libraries on Windows.


Bug: https://bugs.openjdk.java.net/browse/JDK-8213187
Patch inline:
diff --git a/make/lib/Lib-jdk.accessibility.gmk 
b/make/lib/Lib-jdk.accessibility.gmk

--- a/make/lib/Lib-jdk.accessibility.gmk
+++ b/make/lib/Lib-jdk.accessibility.gmk
@@ -41,7 +41,7 @@
 EXTRA_SRC := common, \
 OPTIMIZATION := LOW, \
 DISABLED_WARNINGS_microsoft := 4311 4302 4312, \
-    CFLAGS := $(CFLAGS_JDKLIB) \
+    CFLAGS :=  $(filter-out -MD, $(CFLAGS_JDKLIB)) -MT \
 -DACCESSBRIDGE_ARCH_$2, \
 EXTRA_HEADER_DIRS := \
 include/bridge \

/Magnus

Re: RFR: JDK-8214718 Update missing copyright year in build system

[Tim Bell]

Magnus:

Looks good to me as well.

Tim

On 12/03/18 09:35, Erik Joelsson wrote:

Looks good.

/Erik

On 2018-12-03 08:30, Magnus Ihse Bursie wrote:

Not all changes in 2018 in the build system were accompanied by
updates of the copyright year. Add 2018 to files that were actually
modified in 2018, but did not get the copyright line updated.

I have found this list by running the update_copyright_year script,
and then manually verified that the files were indeed modified in
2018, and was not a "dummy" change (such as modifying the copyright
header...). Files with "dummy" changes were reverted. I have also
manually verified that the script did indeed rewrite the header
correctly. (I do not trust scripts... The only good script is a dead
scr... nah. But I do not trust them.)

As usual, the easiest way to review a change like this is to look at
the patch file in webrev.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214718
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8214718-update-missing-copyright-year-2018/webrev.01

Re: RFR: JDK-8214311 dtrace gensrc has missing dependencies

[Erik Joelsson]

Looks good.

/Erik

On 2018-12-03 08:45, Tim Bell wrote:

Magnus:


Building with JOBS=1 on solaris discovered that the dtrace gensrc has
not all dependencies properly declared. This has not been discovered
until now, since Solaris machines is typically very parallel, and the
needed prerequisites has just happened to have been generated before.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214311
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8214311-fix-dtrace-gensrc-dependencies/webrev.01 



Looks good.

Tim

Re: RFR: JDK-8214718 Update missing copyright year in build system

[Erik Joelsson]

Looks good.

/Erik

On 2018-12-03 08:30, Magnus Ihse Bursie wrote:
Not all changes in 2018 in the build system were accompanied by 
updates of the copyright year. Add 2018 to files that were actually 
modified in 2018, but did not get the copyright line updated.


I have found this list by running the update_copyright_year script, 
and then manually verified that the files were indeed modified in 
2018, and was not a "dummy" change (such as modifying the copyright 
header...). Files with "dummy" changes were reverted. I have also 
manually verified that the script did indeed rewrite the header 
correctly. (I do not trust scripts... The only good script is a dead 
scr... nah. But I do not trust them.)


As usual, the easiest way to review a change like this is to look at 
the patch file in webrev.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214718
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214718-update-missing-copyright-year-2018/webrev.01


And, for reference, here is the list of the latest changes for every 
file I've updated:


Latest entry for bin/idea.sh
changeset:   52517:59065e5d56ec
user:    stuefe
date:    Wed Nov 14 09:19:31 2018 +0100
summary: 8213591: running bin/idea.sh in Cygwin: generated project 
cannot be imported


Latest entry for make/BuildStatic.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/Bundles.gmk
changeset:   52774:56ca125c973b
user:    shurailine
date:    Thu Nov 29 06:34:46 2018 -0800
summary: 8214309: Enhance makefiles to allow generating JCov 
instrumented build


Latest entry for make/CompileDemos.gmk
changeset:   50831:59c6972e39fa
user:    prr
date:    Fri Jun 22 13:21:23 2018 -0700
summary: 8205494: Convert or remove all AWT applet demos

Latest entry for make/CompileToolsHotspot.gmk
changeset:   50330:2cbc42a5764b
user:    dlong
date:    Thu May 31 10:38:05 2018 -0700
summary: 8202670: Update Graal

Latest entry for make/CopyInterimCLDRConverter.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/CreateBuildJdkCopy.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/CreateJmods.gmk
changeset:   50142:f348e5d4769b
user:    erikj
date:    Fri May 11 08:39:21 2018 -0700
summary: 8202914: Let custom makefile override jmod intput dir 
locations


Latest entry for make/GenerateModuleSummary.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/GensrcModuleInfo.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/InterimImage.gmk
changeset:   49207:2a25589b5971
user:    redestad
date:    Mon Mar 12 19:36:59 2018 +0100
summary: 8199469: Disable generate-jli-classes when building 
interim-image


Latest entry for make/ZipSource.gmk
changeset:   50590:5fa19bad622d
user:    erikj
date:    Fri Jun 15 09:53:28 2018 -0700
summary: 8204973: Add build support for filtering translations

Latest entry for make/autoconf/boot-jdk.m4
changeset:   51822:f3c1945fa8aa
user:    ihse
date:    Thu Sep 20 18:39:53 2018 +0200
summary: 8210960: Allow --with-boot-jdk-jvmargs to work during 
configure


Latest entry for make/autoconf/build-aux/config.guess
changeset:   50311:04c8eba70a59
user:    erikj
date:    Wed May 30 09:45:24 2018 -0700
summary: 8204091: Configure broken on MIPS when uname returns 
mipsel or mips64el


Latest entry for make/autoconf/jdk-version.m4
changeset:   52724:0bdbf854472f
user:    rriggs
date:    Wed Nov 28 15:53:49 2018 -0500
summary: 4947890: Minimize JNI upcalls in system-properties 
initialization


Latest entry for make/autoconf/lib-bundled.m4
changeset:   48758:ba19a21d727d
user:    erikj
date:    Wed Feb 07 09:48:43 2018 -0800
summary: 8196951: jdk build fails with clang: error: no such file 
or directory: '@LIBZ_CFLAGS@'


Latest entry for make/autoconf/toolchain_windows.m4
changeset:   50639:c12c79a49ca2
user:    erikj
date:    Tue Jun 19 16:44:41 2018 +0200
summary: 8205183: Warning about using VS2017 should be removed

Latest entry for make/common/JarArchive.gmk
changeset:   52595:16609197022c
user:    redestad
date:    Fri Nov 16 23:39:51 2018 +0100
summary: 8061281: Microbenchmark suite build support, directory 
layout and sample benchmarks


Latest entry for make/common/JavaCompilation.gmk
changeset:   52065

Re: RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Erik Joelsson]

Looks good.

/Erik

On 2018-12-03 06:19, Magnus Ihse Bursie wrote:
The commands for hg log is missing -l1, which will limit the log to 
just the revision specified. Instead, all revisions from repo creation 
will now be included, and the script fails to work.


I will publish a separate changeset with missed copyright year updates 
in the build system.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01


/Magnus

Re: RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Tim Bell]

Magnus:

On 12/03/18 08:16, Alan Bateman wrote:



On 03/12/2018 16:10, Magnus Ihse Bursie wrote:

The commands for hg log is missing -l1, which will limit the log to
just the revision specified. Instead, all revisions from repo creation
will now be included, and the script fails to work.

I will publish a separate changeset with missed copyright year updates
in the build system.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01



This looks okay to me.

For the follow-on "missing copyright year updates" then maybe the entire
source should be done rather than doing it in piecemeal.

-Alan


Looks good to me as well.

Tim

Re: RFR: JDK-8214311 dtrace gensrc has missing dependencies

[Tim Bell]

Magnus:


Building with JOBS=1 on solaris discovered that the dtrace gensrc has
not all dependencies properly declared. This has not been discovered
until now, since Solaris machines is typically very parallel, and the
needed prerequisites has just happened to have been generated before.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214311
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8214311-fix-dtrace-gensrc-dependencies/webrev.01


Looks good.

Tim

Re: RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Magnus Ihse Bursie]

On 2018-12-03 17:16, Alan Bateman wrote:



On 03/12/2018 16:10, Magnus Ihse Bursie wrote:
The commands for hg log is missing -l1, which will limit the log to 
just the revision specified. Instead, all revisions from repo 
creation will now be included, and the script fails to work.


I will publish a separate changeset with missed copyright year 
updates in the build system.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01



This looks okay to me.

Thanks.


For the follow-on "missing copyright year updates" then maybe the 
entire source should be done rather than doing it in piecemeal.


I just created JDK-8214718... Anyway, I'm not sure that's a good idea. 
The script gives an indication of files that should be updated, but it's 
not 100% correct and someone will need to verify that it does not 
trigger for changes that should not result in a copyright year update 
(like updating the copyright header). The script, as it is, tries to 
filter out changesets that it should not consider, but that looked very 
brittle, and I did in fact remove that check before running the script 
on the build system code. Perhaps not component knowledge is needed, but 
laying the burden on a single person to do this grunt work for all of 
the code base is perhaps not fair either. For me, I'd like to know that 
the code I'm responsible for is correct.


/Magnus

RFR: JDK-8214718 Update missing copyright year in build system

[Magnus Ihse Bursie]

Not all changes in 2018 in the build system were accompanied by updates 
of the copyright year. Add 2018 to files that were actually modified in 
2018, but did not get the copyright line updated.


I have found this list by running the update_copyright_year script, and 
then manually verified that the files were indeed modified in 2018, and 
was not a "dummy" change (such as modifying the copyright header...). 
Files with "dummy" changes were reverted. I have also manually verified 
that the script did indeed rewrite the header correctly. (I do not trust 
scripts... The only good script is a dead scr... nah. But I do not trust 
them.)


As usual, the easiest way to review a change like this is to look at the 
patch file in webrev.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214718
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214718-update-missing-copyright-year-2018/webrev.01


And, for reference, here is the list of the latest changes for every 
file I've updated:


Latest entry for bin/idea.sh
changeset:   52517:59065e5d56ec
user:    stuefe
date:    Wed Nov 14 09:19:31 2018 +0100
summary: 8213591: running bin/idea.sh in Cygwin: generated project 
cannot be imported


Latest entry for make/BuildStatic.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/Bundles.gmk
changeset:   52774:56ca125c973b
user:    shurailine
date:    Thu Nov 29 06:34:46 2018 -0800
summary: 8214309: Enhance makefiles to allow generating JCov 
instrumented build


Latest entry for make/CompileDemos.gmk
changeset:   50831:59c6972e39fa
user:    prr
date:    Fri Jun 22 13:21:23 2018 -0700
summary: 8205494: Convert or remove all AWT applet demos

Latest entry for make/CompileToolsHotspot.gmk
changeset:   50330:2cbc42a5764b
user:    dlong
date:    Thu May 31 10:38:05 2018 -0700
summary: 8202670: Update Graal

Latest entry for make/CopyInterimCLDRConverter.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/CreateBuildJdkCopy.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/CreateJmods.gmk
changeset:   50142:f348e5d4769b
user:    erikj
date:    Fri May 11 08:39:21 2018 -0700
summary: 8202914: Let custom makefile override jmod intput dir locations

Latest entry for make/GenerateModuleSummary.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/GensrcModuleInfo.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -0700
summary: 8211724: Change mkdir -p to MakeDir macro where possible

Latest entry for make/InterimImage.gmk
changeset:   49207:2a25589b5971
user:    redestad
date:    Mon Mar 12 19:36:59 2018 +0100
summary: 8199469: Disable generate-jli-classes when building 
interim-image


Latest entry for make/ZipSource.gmk
changeset:   50590:5fa19bad622d
user:    erikj
date:    Fri Jun 15 09:53:28 2018 -0700
summary: 8204973: Add build support for filtering translations

Latest entry for make/autoconf/boot-jdk.m4
changeset:   51822:f3c1945fa8aa
user:    ihse
date:    Thu Sep 20 18:39:53 2018 +0200
summary: 8210960: Allow --with-boot-jdk-jvmargs to work during configure

Latest entry for make/autoconf/build-aux/config.guess
changeset:   50311:04c8eba70a59
user:    erikj
date:    Wed May 30 09:45:24 2018 -0700
summary: 8204091: Configure broken on MIPS when uname returns mipsel 
or mips64el


Latest entry for make/autoconf/jdk-version.m4
changeset:   52724:0bdbf854472f
user:    rriggs
date:    Wed Nov 28 15:53:49 2018 -0500
summary: 4947890: Minimize JNI upcalls in system-properties 
initialization


Latest entry for make/autoconf/lib-bundled.m4
changeset:   48758:ba19a21d727d
user:    erikj
date:    Wed Feb 07 09:48:43 2018 -0800
summary: 8196951: jdk build fails with clang: error: no such file or 
directory: '@LIBZ_CFLAGS@'


Latest entry for make/autoconf/toolchain_windows.m4
changeset:   50639:c12c79a49ca2
user:    erikj
date:    Tue Jun 19 16:44:41 2018 +0200
summary: 8205183: Warning about using VS2017 should be removed

Latest entry for make/common/JarArchive.gmk
changeset:   52595:16609197022c
user:    redestad
date:    Fri Nov 16 23:39:51 2018 +0100
summary: 8061281: Microbenchmark suite build support, directory 
layout and sample benchmarks


Latest entry for make/common/JavaCompilation.gmk
changeset:   52065:dea8a62cdfc3
user:    erikj
date:    Tue Oct 09 14:57:23 2018 -

Re: RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Alan Bateman]

On 03/12/2018 16:10, Magnus Ihse Bursie wrote:
The commands for hg log is missing -l1, which will limit the log to 
just the revision specified. Instead, all revisions from repo creation 
will now be included, and the script fails to work.


I will publish a separate changeset with missed copyright year updates 
in the build system.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01



This looks okay to me.

For the follow-on "missing copyright year updates" then maybe the entire 
source should be done rather than doing it in piecemeal.


-Alan

RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Magnus Ihse Bursie]

The commands for hg log is missing -l1, which will limit the log to just the 
revision specified. Instead, all revisions from repo creation will now be 
included, and the script fails to work.

I will publish a separate changeset with missed copyright year updates in the 
build system.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01

/Magnus

RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Magnus Ihse Bursie]

The commands for hg log is missing -l1, which will limit the log to just 
the revision specified. Instead, all revisions from repo creation will 
now be included, and the script fails to work.


I will publish a separate changeset with missed copyright year updates 
in the build system.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01


/Magnus

RFR: JDK-8214710 Fix hg log in update_copyright_year.sh

[Magnus Ihse Bursie]

The commands for hg log is missing -l1, which will limit the log to just 
the revision specified. Instead, all revisions from repo creation will 
now be included, and the script fails to work.


I will publish a separate changeset with missed copyright year updates 
in the build system.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214710
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214710-fix-update-copyright-year/webrev.01


/Magnus

Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken

[Volker Simonis]

Cool, thanks!
I'll push then...
On Mon, Dec 3, 2018 at 2:37 PM Magnus Ihse Bursie
 wrote:
>
> On 2018-12-03 14:31, Volker Simonis wrote:
>
> Hi Magnus, Erik,
>
> do I understand you correctly that you both are fine with my proposed
> fix and that we leave all the other enhancements/discussion for the
> future?
>
> Yes, sorry I was not clear. Your fix looks good to me.
>
> We can save the rest of the discussion of reproducible builds for a rainy day.
>
> /Magnus
>
> Thank you and best regards,
> Volker
>
> On Mon, Dec 3, 2018 at 12:27 PM Magnus Ihse Bursie
>  wrote:
>
> On 2018-11-30 19:03, Volker Simonis wrote:
>
> On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson  
> wrote:
>
> Hello Volker,
>
> The fix looks good. Thanks for finding and fixing it!
>
> Thanks!
>
> Now for some history on why THIS_FILE. The short story is that it's for
> more reproducible and comparable builds.
>
> When we started the build infra project, one of the design decisions was
> to use absolute paths everywhere to avoid having to keep track of the
> current directory, and to make all command lines in the build be simply
> copy and paste in a terminal to rerun.
>
> A consequence of this was that the __FILE__ macro then also expands to
> absolute paths. This made binary build comparisons much harder. Very
> often (especially in the build infra project itself) we use elaborate
> comparison methods to verify that a build change does not change the
> output of the build in any unwanted way. We then introduced the
> THIS_FILE macro to get rid of the absolute paths baked into our binaries
> which got rid of a huge source of binary noise preventing reproducible
> builds.
>
> Note that two different builds with slightly different output
> directories (or in the build-infra project case, completely different
> output structure for generated sources) will generate absolute source
> paths of different lengths. This will cause otherwise equivalent
> binaries to differ greatly due to different alignment, not just because
> of different contents in those strings.
>
> With this change, we could count on object files (at least for GCC) to
> always end up binary equivalent.
>
> In my long term vision, I would like to get the OpenJDK build even more
> reproducible, but it's currently not a high priority task. I would be
> very hard to convince of reducing the level of reproducible output we
> have though.
>
> Thanks for the background information. But as far as I can see, this
> currently only works because "THIS_FILE" is always empty which of
> course makes builds to various locations highly comparable :) On the
> other hand, HotSpot is not using THIS_FILE at all and "__FILE__" quite
> a lot.
>
> No, it's not. It will work just as well when THIS_FILE once more is fixed, 
> since
> /tmp/foo/src/java.base/.../fooimpl.c will have -DTHIS_FILE="fooimpl.c"
> just as
> /home/chthulu/puny_humans_projects/jdk/src/java.base/.../fooimpl.c will have 
> -DTHIS_FILE="fooimpl.c"
>
> So the builds of fooimpl.c will be identical. Or, at least, not dependent on 
> His R'lyehian Highness choice of directory names.
>
> /Magnus
>
>
>
> Don't get me wrong. I highly appreciate the feature of having absolute
> path names in the build to make all command lines in the build
> self-contained (I use that feature every day :). And I also support
> the goal of making builds even more reproducible. But does this goal
> not apply to hotspot (or is it just on the TODO list ?).
>
> In the end, I'm happy with the current, minimal fix which at least
> gets the logs working again.
>
> And maybe for the follow up change we should then better move all
> "__FILE__" occurrences in HotSpot to "THIS_FILE" instead of getting
> rid of "THIS_FILE"?
>
> Best regards,
> Volker
>
> /Erik
>
> On 2018-11-30 09:05, Volker Simonis wrote:
>
> Hi,
>
> can I please have a review for the following trivial fix:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8214534/
> https://bugs.openjdk.java.net/browse/JDK-8214534
>
> DISCLAIMER: "XS" refers to the size of the fix, not the size of the
> explanation :)
>
> Currently the compilation of native files defines "THIS_FILE" to hold
> the name of the current compilation unit. However, setting "THIS_FILE"
> in NativeCompilation.gmk is broken and results in "THIS_FILE" always
> being the empty string. I first thought that this is just a simple
> quoting issue, but after I couldn't get it working, I found the
> following explanation in the GNU Make manual [1]:
>
> "A common mistake is attempting to use $@ within the prerequisites
> list; this will not work. However, there is a special feature of GNU
> make, secondary expansion (see Secondary Expansion), which will allow
> automatic variable values to be used in prerequisite lists."
>
> I'm not a Make expert, but I think this quote doesn't apply to "$@"
> only, but to all automatic variables. The proposed solution (i.e.
> "Secondary Expansion" [2]) seems overly complex for this problem. I
> think the solution in 

Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken

[Magnus Ihse Bursie]

On 2018-12-03 14:31, Volker Simonis wrote:

Hi Magnus, Erik,

do I understand you correctly that you both are fine with my proposed
fix and that we leave all the other enhancements/discussion for the
future?

Yes, sorry I was not clear. Your fix looks good to me.

We can save the rest of the discussion of reproducible builds for a 
rainy day.


/Magnus


Thank you and best regards,
Volker

On Mon, Dec 3, 2018 at 12:27 PM Magnus Ihse Bursie
 wrote:

On 2018-11-30 19:03, Volker Simonis wrote:

On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson  wrote:

Hello Volker,

The fix looks good. Thanks for finding and fixing it!

Thanks!

Now for some history on why THIS_FILE. The short story is that it's for
more reproducible and comparable builds.

When we started the build infra project, one of the design decisions was
to use absolute paths everywhere to avoid having to keep track of the
current directory, and to make all command lines in the build be simply
copy and paste in a terminal to rerun.

A consequence of this was that the __FILE__ macro then also expands to
absolute paths. This made binary build comparisons much harder. Very
often (especially in the build infra project itself) we use elaborate
comparison methods to verify that a build change does not change the
output of the build in any unwanted way. We then introduced the
THIS_FILE macro to get rid of the absolute paths baked into our binaries
which got rid of a huge source of binary noise preventing reproducible
builds.

Note that two different builds with slightly different output
directories (or in the build-infra project case, completely different
output structure for generated sources) will generate absolute source
paths of different lengths. This will cause otherwise equivalent
binaries to differ greatly due to different alignment, not just because
of different contents in those strings.

With this change, we could count on object files (at least for GCC) to
always end up binary equivalent.

In my long term vision, I would like to get the OpenJDK build even more
reproducible, but it's currently not a high priority task. I would be
very hard to convince of reducing the level of reproducible output we
have though.

Thanks for the background information. But as far as I can see, this
currently only works because "THIS_FILE" is always empty which of
course makes builds to various locations highly comparable :) On the
other hand, HotSpot is not using THIS_FILE at all and "__FILE__" quite
a lot.

No, it's not. It will work just as well when THIS_FILE once more is fixed, since
/tmp/foo/src/java.base/.../fooimpl.c will have -DTHIS_FILE="fooimpl.c"
just as
/home/chthulu/puny_humans_projects/jdk/src/java.base/.../fooimpl.c will have 
-DTHIS_FILE="fooimpl.c"

So the builds of fooimpl.c will be identical. Or, at least, not dependent on 
His R'lyehian Highness choice of directory names.

/Magnus



Don't get me wrong. I highly appreciate the feature of having absolute
path names in the build to make all command lines in the build
self-contained (I use that feature every day :). And I also support
the goal of making builds even more reproducible. But does this goal
not apply to hotspot (or is it just on the TODO list ?).

In the end, I'm happy with the current, minimal fix which at least
gets the logs working again.

And maybe for the follow up change we should then better move all
"__FILE__" occurrences in HotSpot to "THIS_FILE" instead of getting
rid of "THIS_FILE"?

Best regards,
Volker

/Erik

On 2018-11-30 09:05, Volker Simonis wrote:

Hi,

can I please have a review for the following trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2018/8214534/
https://bugs.openjdk.java.net/browse/JDK-8214534

DISCLAIMER: "XS" refers to the size of the fix, not the size of the
explanation :)

Currently the compilation of native files defines "THIS_FILE" to hold
the name of the current compilation unit. However, setting "THIS_FILE"
in NativeCompilation.gmk is broken and results in "THIS_FILE" always
being the empty string. I first thought that this is just a simple
quoting issue, but after I couldn't get it working, I found the
following explanation in the GNU Make manual [1]:

"A common mistake is attempting to use $@ within the prerequisites
list; this will not work. However, there is a special feature of GNU
make, secondary expansion (see Secondary Expansion), which will allow
automatic variable values to be used in prerequisite lists."

I'm not a Make expert, but I think this quote doesn't apply to "$@"
only, but to all automatic variables. The proposed solution (i.e.
"Secondary Expansion" [2]) seems overly complex for this problem. I
think the solution in the patch is much simpler and works "just as
well" :)

The other question is of course why do we need "THIS_FILE" at all? It
is used for various native logs in the class library (not in HotSpot)
which use the value of "THIS_FILE" to decorate their output with the
current file name. On the other hand, we alread

Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken

[Volker Simonis]

Hi Magnus, Erik,

do I understand you correctly that you both are fine with my proposed
fix and that we leave all the other enhancements/discussion for the
future?

Thank you and best regards,
Volker

On Mon, Dec 3, 2018 at 12:27 PM Magnus Ihse Bursie
 wrote:
>
> On 2018-11-30 19:03, Volker Simonis wrote:
>
> On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson  
> wrote:
>
> Hello Volker,
>
> The fix looks good. Thanks for finding and fixing it!
>
> Thanks!
>
> Now for some history on why THIS_FILE. The short story is that it's for
> more reproducible and comparable builds.
>
> When we started the build infra project, one of the design decisions was
> to use absolute paths everywhere to avoid having to keep track of the
> current directory, and to make all command lines in the build be simply
> copy and paste in a terminal to rerun.
>
> A consequence of this was that the __FILE__ macro then also expands to
> absolute paths. This made binary build comparisons much harder. Very
> often (especially in the build infra project itself) we use elaborate
> comparison methods to verify that a build change does not change the
> output of the build in any unwanted way. We then introduced the
> THIS_FILE macro to get rid of the absolute paths baked into our binaries
> which got rid of a huge source of binary noise preventing reproducible
> builds.
>
> Note that two different builds with slightly different output
> directories (or in the build-infra project case, completely different
> output structure for generated sources) will generate absolute source
> paths of different lengths. This will cause otherwise equivalent
> binaries to differ greatly due to different alignment, not just because
> of different contents in those strings.
>
> With this change, we could count on object files (at least for GCC) to
> always end up binary equivalent.
>
> In my long term vision, I would like to get the OpenJDK build even more
> reproducible, but it's currently not a high priority task. I would be
> very hard to convince of reducing the level of reproducible output we
> have though.
>
> Thanks for the background information. But as far as I can see, this
> currently only works because "THIS_FILE" is always empty which of
> course makes builds to various locations highly comparable :) On the
> other hand, HotSpot is not using THIS_FILE at all and "__FILE__" quite
> a lot.
>
> No, it's not. It will work just as well when THIS_FILE once more is fixed, 
> since
> /tmp/foo/src/java.base/.../fooimpl.c will have -DTHIS_FILE="fooimpl.c"
> just as
> /home/chthulu/puny_humans_projects/jdk/src/java.base/.../fooimpl.c will have 
> -DTHIS_FILE="fooimpl.c"
>
> So the builds of fooimpl.c will be identical. Or, at least, not dependent on 
> His R'lyehian Highness choice of directory names.
>
> /Magnus
>
>
>
> Don't get me wrong. I highly appreciate the feature of having absolute
> path names in the build to make all command lines in the build
> self-contained (I use that feature every day :). And I also support
> the goal of making builds even more reproducible. But does this goal
> not apply to hotspot (or is it just on the TODO list ?).
>
> In the end, I'm happy with the current, minimal fix which at least
> gets the logs working again.
>
> And maybe for the follow up change we should then better move all
> "__FILE__" occurrences in HotSpot to "THIS_FILE" instead of getting
> rid of "THIS_FILE"?
>
> Best regards,
> Volker
>
> /Erik
>
> On 2018-11-30 09:05, Volker Simonis wrote:
>
> Hi,
>
> can I please have a review for the following trivial fix:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8214534/
> https://bugs.openjdk.java.net/browse/JDK-8214534
>
> DISCLAIMER: "XS" refers to the size of the fix, not the size of the
> explanation :)
>
> Currently the compilation of native files defines "THIS_FILE" to hold
> the name of the current compilation unit. However, setting "THIS_FILE"
> in NativeCompilation.gmk is broken and results in "THIS_FILE" always
> being the empty string. I first thought that this is just a simple
> quoting issue, but after I couldn't get it working, I found the
> following explanation in the GNU Make manual [1]:
>
> "A common mistake is attempting to use $@ within the prerequisites
> list; this will not work. However, there is a special feature of GNU
> make, secondary expansion (see Secondary Expansion), which will allow
> automatic variable values to be used in prerequisite lists."
>
> I'm not a Make expert, but I think this quote doesn't apply to "$@"
> only, but to all automatic variables. The proposed solution (i.e.
> "Secondary Expansion" [2]) seems overly complex for this problem. I
> think the solution in the patch is much simpler and works "just as
> well" :)
>
> The other question is of course why do we need "THIS_FILE" at all? It
> is used for various native logs in the class library (not in HotSpot)
> which use the value of "THIS_FILE" to decorate their output with the
> current file name. On the other hand,

Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Roman Kennke]

Hi Per,

Thanks for looking again.

I've fixed the ordering in shenandoah-dev:

http://cr.openjdk.java.net/~rkennke/fix-macro-order/webrev.00/

and it will apear in the next round of webrevs.

Thanks,
Roman


> Hi Roman,
> 
> On 11/30/18 10:00 PM, Roman Kennke wrote:
>> 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
> 
> Thanks for fixing. Looks good to me, except it looks like you missed
> adjusting the macro order in the following files:
>  src/hotspot/share/gc/shared/gc_globals.hpp
>  src/hotspot/share/gc/shared/vmStructs_gc.hpp
> 
> cheers,
> Per
> 
>>
>> 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
 next round).

 Vladimir: Roland integrated a bunch of changes to make loop* code look
 better. I can tell that we're not done with C2 yet. Can you look over
 the code and see what is ok, and especially what is not ok, so that we
 can focus our efforts on the relevant parts?

 Updated set of webrevs:
 http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/01/

 Thanks,
 Roman


> Hi,
>
> This is the first round of changes for including Shenandoah GC into
> mainline.
> I divided the review into parts that roughly correspond to the
> mailing lists
> that would normally review it, and I divided it into 'shared' code
> changes and
> 'shenandoah' code changes (actually, mostly additions). The intend
> is to
> eventually
> push them as single 'combined' changeset, once reviewed.
>
> JEP:
>    https://openjdk.java.net/jeps/189
> Bug entry:
>
>   https://bugs.openjdk.java.net/browse/JDK-8214259
>
> Webrevs:
>    http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/
>
> For those who want to see the full change, have a look at the
> shenandoah-complete
> 

Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Per Liden]

Hi Roman,

On 11/30/18 10:00 PM, Roman Kennke wrote:

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


Thanks for fixing. Looks good to me, except it looks like you missed 
adjusting the macro order in the following files:

 src/hotspot/share/gc/shared/gc_globals.hpp
 src/hotspot/share/gc/shared/vmStructs_gc.hpp

cheers,
Per



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
next round).

Vladimir: Roland integrated a bunch of changes to make loop* code look
better. I can tell that we're not done with C2 yet. Can you look over
the code and see what is ok, and especially what is not ok, so that we
can focus our efforts on the relevant parts?

Updated set of webrevs:
http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/01/

Thanks,
Roman



Hi,

This is the first round of changes for including Shenandoah GC into
mainline.
I divided the review into parts that roughly correspond to the mailing lists
that would normally review it, and I divided it into 'shared' code
changes and
'shenandoah' code changes (actually, mostly additions). The intend is to
eventually
push them as single 'combined' changeset, once reviewed.

JEP:
   https://openjdk.java.net/jeps/189
Bug entry:

  https://bugs.openjdk.java.net/browse/JDK-8214259

Webrevs:
   http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/

For those who want to see the full change, have a look at the
shenandoah-complete

directory,
it contains the full combined webrev. Alternatively, there is the file
shenandoah-master.patch
,
which is what I intend to commit (and which should be equivalent to the
'shenandoah-complete' webrev).

Sections to review (at this point) are the following:
  *) shenandoah-gc

     - Actual Shenandoah implementation, almost completely residing in
gc/shenandoah

  *) shared-gc

     - This is mostly boilerplate that is common to any GC

Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Roman Kennke]

Hi Jini,

Thanks for your suggestions. I've added this to Shenandoah's dev:

http://cr.openjdk.java.net/~rkennke/shenandoah-sa/webrev.01/

and it will show up in next round of webrevs.

Thanks,
Roman


> A few comments on the SA changes:
> 
> ==> Could you please add the following lines in
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java from line
> 1120 onwards to avoid the "[Unknown generation]" message with hsdb while
> displaying the Stack Memory for a mutator thread ?
> 
> else if (collHeap instanceof ShenandoahHeap) {
>    ShenandoahHeap heap = (ShenandoahHeap) collHeap;
>    anno = "ShenandoahHeap ";
>    bad = false;
> }
> 
> ==>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/HeapSummary.java
> 
> The printGCAlgorithm() method would need to be updated to read in the
> UseShenandoahGC flag to avoid the default "Mark Sweep Compact GC" being
> displayed with jhsdb jmap -heap.
> 
> ==>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GCName.java
> 
> Could you please add "Shenandoah" to the GCName enum list ?
> 
> ==>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GCCause.java
> 
> Could you please update the GCCause enum values to include these:
> 
>     _shenandoah_stop_vm,
>     _shenandoah_allocation_failure_evac,
>     _shenandoah_concurrent_gc,
>     _shenandoah_traversal_gc,
>     _shenandoah_upgrade_to_full_gc,
> 
> ==> share/classes/sun/jvm/hotspot/runtime/VMOps.java
> 
> It would be good to add 'ShenandoahOperation' to the VMOps enum (though
> it is probably not in sync now).
> 
> Thank you,
> Jini.
> 
> On 12/1/2018 2:30 AM, Roman Kennke wrote:
>> 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
 next round).

 Vladimir: Roland integrated a bunch of changes to make loop* code look

Re: [PATCH v2] Add support for SoftFloat library on ARM

[Magnus Ihse Bursie]

Comment inline.

On 2018-11-30 14:38, Jakub Vaněk wrote:

Hi David,

I'm sending an alternative patch for handling the problem with
__aeabi_*_glibc on arm softfloat builds. This patch adds integration
code for SoftFloat-3e library. It adds appropriate configure options
and a switchable implementation of __aeabi_*_glibc/__aeabi_*_extlib
functions.

There are now two implementations of the _glibc functions (now renamed
to _extlib). The simpler one simply wraps __aeabi_* functions. The
other one wraps SoftFloat-3 API. By using this mechanism, the building
user can choose what happens. The external library is not required - if
the path to its static library/include directory is not specified on
configure commandline, standard system functions are used
automatically.

Thanks,

Jakub

# HG changeset patch
# User Jakub Vaněk 
# Date 1543498682 -3600
#  Thu Nov 29 14:38:02 2018 +0100
# Node ID bc14ee6f50c73703229f979e78b93bcef12ae106
# Parent  a96844b3a929cc2eb92fe7963be8aec603f24a83
Add support for SoftFloat library on ARM

diff --git a/doc/building.html b/doc/building.html
--- a/doc/building.html
+++ b/doc/building.html
@@ -53,6 +53,7 @@
  X11
  ALSA
  libffi
+SoftFloat
  
  Build Tools Requirements
  Autoconf
@@ -415,6 +416,12 @@
  To install on an rpm-based Linux, try running sudo yum install 
libffi-devel.
  
  Use --with-libffi= if configure does not 
properly locate your libffi files.
+SoftFloat
+http://www.jhauser.us/arithmetic/SoftFloat.html";>Berkeley SoftFloat-3 can be used on ARM 
processors without FPU to slightly enhance the arithmetic precision of some floating point operations. It is not required, system 
softfp routines can be used without any problems. The precision loss is extremely small, but http://mail.openjdk.java.net/pipermail/aarch32-port-dev/2016-November/000611.html";>the JCK detects 
it.
+
+To install the library, you will have to download its source and build it for the target 
platform. To do so, take a look in the build/Linux-ARM-VFPv2-GCC 
subdirectory.
+
+Use --with-softfloat-lib= and --with-softfloat-include= to specify 
the path to the softfloat.a archive and the source/include directory. If you omit them or use 
--without-softfloat-*, standard system libraries will be used instead.
  Build Tools Requirements
  Autoconf
  The JDK requires http://www.gnu.org/software/autoconf";>Autoconf; on 
all platforms. At least version 2.69 is required.
@@ -486,6 +493,7 @@
  --with-x= - Set the path to X11
  --with-alsa= - Set the path to ALSA
  --with-libffi= - Set the path to libffi
+--with-softfloat-lib=, --with-softfloat-include= - 
Set the path to SoftFloat library and include directory.
  --with-jtreg= - Set the path to JTReg. See Running Tests
  
  Certain third-party libraries used by the JDK (libjpeg, giflib, libpng, lcms and zlib) are included in the JDK repository. 
The default behavior of the JDK build is to use this version of these libraries, but they might be replaced by an external version. To 
do so, specify system as the  option in these arguments. (The 
default is bundled).
diff --git a/doc/building.md b/doc/building.md
--- a/doc/building.md
+++ b/doc/building.md
@@ -527,6 +527,24 @@
  Use `--with-libffi=` if `configure` does not properly locate your libffi
  files.
  
+### SoftFloat

+
+[Berkeley SoftFloat-3](http://www.jhauser.us/arithmetic/SoftFloat.html)
+can be used on ARM processors without FPU to slightly enhance
+the arithmetic precision of some floating point operations. It is not
+required, system softfp routines can be used without any problems.
+The precision loss is extremely small, but [the JCK detects it](
+http://mail.openjdk.java.net/pipermail/aarch32-port-dev/2016-November/000611.html).
+
+  * To install the library, you will have to download its source and build it
+for the target platform. To do so, take a look in the
+`build/Linux-ARM-VFPv2-GCC` subdirectory.
+
+Use `--with-softfloat-lib=` and `--with-softfloat-include=`
+to specify the path to the `softfloat.a` archive and the `source/include`
+directory. If you omit them or use `--without-softfloat-*`, standard
+system libraries will be used instead.
+
  ## Build Tools Requirements
  
  ### Autoconf

@@ -694,6 +712,8 @@
* `--with-x=` - Set the path to [X11](#x11)
* `--with-alsa=` - Set the path to [ALSA](#alsa)
* `--with-libffi=` - Set the path to [libffi](#libffi)
+  * `--with-softfloat-lib=`, `--with-softfloat-include=` -
+Set the path to [SoftFloat](#softfloat) library and include directory.
* `--with-jtreg=` - Set the path to JTReg. See [Running Tests](
  #running-tests)
  
diff --git a/make/autoconf/lib-softfloat.m4 b/make/autoconf/lib-softfloat.m4

new file mode 100644
--- /dev/null
+++ b/make/autoconf/lib-softfloat.m4
@@ -0,0 +1,93 @@
+#
+# Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+#
+# This code is free 

RFR: JDK-8214311 dtrace gensrc has missing dependencies

[Magnus Ihse Bursie]

Building with JOBS=1 on solaris discovered that the dtrace gensrc has 
not all dependencies properly declared. This has not been discovered 
until now, since Solaris machines is typically very parallel, and the 
needed prerequisites has just happened to have been generated before.


Bug: https://bugs.openjdk.java.net/browse/JDK-8214311
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8214311-fix-dtrace-gensrc-dependencies/webrev.01


/Magnus

Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken

[Magnus Ihse Bursie]

On 2018-11-30 19:03, Volker Simonis wrote:

On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson  wrote:

Hello Volker,

The fix looks good. Thanks for finding and fixing it!


Thanks!


Now for some history on why THIS_FILE. The short story is that it's for
more reproducible and comparable builds.

When we started the build infra project, one of the design decisions was
to use absolute paths everywhere to avoid having to keep track of the
current directory, and to make all command lines in the build be simply
copy and paste in a terminal to rerun.

A consequence of this was that the __FILE__ macro then also expands to
absolute paths. This made binary build comparisons much harder. Very
often (especially in the build infra project itself) we use elaborate
comparison methods to verify that a build change does not change the
output of the build in any unwanted way. We then introduced the
THIS_FILE macro to get rid of the absolute paths baked into our binaries
which got rid of a huge source of binary noise preventing reproducible
builds.

Note that two different builds with slightly different output
directories (or in the build-infra project case, completely different
output structure for generated sources) will generate absolute source
paths of different lengths. This will cause otherwise equivalent
binaries to differ greatly due to different alignment, not just because
of different contents in those strings.

With this change, we could count on object files (at least for GCC) to
always end up binary equivalent.

In my long term vision, I would like to get the OpenJDK build even more
reproducible, but it's currently not a high priority task. I would be
very hard to convince of reducing the level of reproducible output we
have though.


Thanks for the background information. But as far as I can see, this
currently only works because "THIS_FILE" is always empty which of
course makes builds to various locations highly comparable :) On the
other hand, HotSpot is not using THIS_FILE at all and "__FILE__" quite
a lot.
No, it's not. It will work just as well when THIS_FILE once more is 
fixed, since

/tmp/foo/src/java.base/.../fooimpl.c will have -DTHIS_FILE="fooimpl.c"
just as
/home/chthulu/puny_humans_projects/jdk/src/java.base/.../fooimpl.c will 
have -DTHIS_FILE="fooimpl.c"


So the builds of fooimpl.c will be identical. Or, at least, not 
dependent on His R'lyehian Highness choice of directory names.


/Magnus




Don't get me wrong. I highly appreciate the feature of having absolute
path names in the build to make all command lines in the build
self-contained (I use that feature every day :). And I also support
the goal of making builds even more reproducible. But does this goal
not apply to hotspot (or is it just on the TODO list ?).

In the end, I'm happy with the current, minimal fix which at least
gets the logs working again.

And maybe for the follow up change we should then better move all
"__FILE__" occurrences in HotSpot to "THIS_FILE" instead of getting
rid of "THIS_FILE"?

Best regards,
Volker


/Erik

On 2018-11-30 09:05, Volker Simonis wrote:

Hi,

can I please have a review for the following trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2018/8214534/
https://bugs.openjdk.java.net/browse/JDK-8214534

DISCLAIMER: "XS" refers to the size of the fix, not the size of the
explanation :)

Currently the compilation of native files defines "THIS_FILE" to hold
the name of the current compilation unit. However, setting "THIS_FILE"
in NativeCompilation.gmk is broken and results in "THIS_FILE" always
being the empty string. I first thought that this is just a simple
quoting issue, but after I couldn't get it working, I found the
following explanation in the GNU Make manual [1]:

"A common mistake is attempting to use $@ within the prerequisites
list; this will not work. However, there is a special feature of GNU
make, secondary expansion (see Secondary Expansion), which will allow
automatic variable values to be used in prerequisite lists."

I'm not a Make expert, but I think this quote doesn't apply to "$@"
only, but to all automatic variables. The proposed solution (i.e.
"Secondary Expansion" [2]) seems overly complex for this problem. I
think the solution in the patch is much simpler and works "just as
well" :)

The other question is of course why do we need "THIS_FILE" at all? It
is used for various native logs in the class library (not in HotSpot)
which use the value of "THIS_FILE" to decorate their output with the
current file name. On the other hand, we already have the standard,
predefined "__FILE__" macro which could be used instead (and indeed,
if "THIS_FILE" isn't defined, the various logging routines fall back
to using "__FILE__").

The only explanation I could come up for having "THIS_FILE" until now
is that "__FILE__" may contain the full path name of the compilation
unit while we only want the simple file name (without path) in the
log. However, in order to solve this "path" problem, we c

Re: RFR (round 1), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Per Liden]

Hi,

On 11/28/18 3:24 PM, Roman Kennke wrote:

Hi Per,


Hi Roman,

On 11/26/18 10:39 PM, Roman Kennke wrote:
[...]

    *) shared-serviceability



   - The usual code to support another GC


Just had a quick look at the SA part. I was thinking you'd have the same
problem as ZGC here, with regards to parsing the heap and potentially
reading garbage when you step on a Klass* which had been unloaded?


Possible. I am myself not very familiar with SA. I guess it depends on
how SA does it: if it iterates objects via CH::object_iterate() (e.g.
same entry point as, e.g., heap-dumping code), then we should be fine.
We're kicking off a traversal rather than straight scan there. If
however SA somehow makes a raw scan itself, then we'd have the problem
you describe.


The SA does a raw scan itself, which is the root of the problem.
ObejctHeap.iterateLiveRegions() will locate the first object in a region
by doing

   OopHandle handle = bottom.addOffsetToAsOopHandle(0);

and to get the next object it does

   handle.addOffsetToAsOopHandle(obj.getObjectSize());

and you'll crash. So I'm afraid this will not work for Shenandoah either.


Alright. I'll 'disable' it like you did with ZGC then. Thanks for
pointing it out.

I'm wondering: this would crash with G1 and
+ClassUnloadingWithConcurrentMark too, then?


That's a very good point and I think you're actually right. Would be 
good if someone from the G1 camp could confirm.


cheers,
Per