Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v3]

2021-02-27 Thread Aleksey Shipilev 
On Sat, 27 Feb 2021 20:01:10 GMT, Attila Szegedi  wrote:

>> 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with 
>> "AssertionError: Should have GCd a method handle by now"
>
> Attila Szegedi has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8261483: Eliminate flakiness of the tests by using iteration number limit 
> and explicitly running GC

Changes requested by shade (Reviewer).

test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 32:

> 30:  * @run main/othervm -XX:+UseParallelGC TypeConverterFactoryMemoryLeakTest
> 31:  * @run main/othervm -XX:+UseZGC TypeConverterFactoryMemoryLeakTest
> 32:  * @run main/othervm -XX:+UseShenandoahGC 
> TypeConverterFactoryMemoryLeakTest

Ah, here is a test trivia. Some configurations do not have either ZGC or 
Shenandoah. So you need to check GC availabilty before adding `@run`. For 
consistency, checking every GC availability is even better. Usually done by 
splitting the `@test` blocks, and adding `@requires` tags: 
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTest.java#L37-L107

-

PR: https://git.openjdk.java.net/jdk/pull/2617

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v2]

2021-02-27 Thread Aleksey Shipilev 
On Sat, 27 Feb 2021 20:10:15 GMT, Attila Szegedi  wrote:

>> test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 79:
>> 
>>> 77: 
>>> 78: public static void main(String[] args) {
>>> 79: for (int count = 0; count < MAX_ITERATIONS; ++count) {
>> 
>> Here and later: use postfix `count++`, regular style?
>
> Old habits die hard; I think I started doing this about 30 years ago when 
> writing C code against a compiler on Atari ST that emitted more efficient 
> MC68000 code for `++i` than for `i++`. I guess it's time to unlearn this :-) 
> Of course, if you wanted to get the fastest code, you would've counted 
> _decrementing to zero_ to let the compiler use the DBRA (decrement and 
> branch) instruction.
> 
> It's funny how I _don't_ miss those days.

Good!

-

PR: https://git.openjdk.java.net/jdk/pull/2617

Re: RFR: 8173970: jar tool should have a way to extract to a directory

2021-02-27 Thread Jaikiran Pai 

Hello Alan,

On 27/02/21 2:23 pm, Alan Bateman wrote:


Yes, the option name will need to be agreed. It would be useful to 
enumerate the options that the other tools are using to specify the 
location where to extract. If you see JBS issues mentioning tar -C not 
supporting chdir when extracting then it might be Solaris tar, which 
isn't the same as GNU tar which has different options. It might be 
better to look at more main stream tools, like unzip although jar -d 
is already taken. It would be nice if there were some consistency with 
other tools in the JDK that doing extracting (The jmod and jimage 
extract commands use use --dir for example).


I had a look at both tar and unzip commands on MacOS and Linux (CentOS) 
setup that I had access to.


--
tar on MacOS:
--

tar --version
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.6

The version of this tool has:

-C directory, --cd directory, --directory directory
 In c and r mode, this changes the directory before adding 
the following files.
 In x mode, change directories after opening the archive 
but before extracting

 entries from the archive.

A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and 
extracts the foo.tar.gz from current directory to a target directory 
/tmp/bar/


--
tar on CentOS:
--

tar --version
tar (GNU tar) 1.26

This version of the tool has:

Common options:
   -C, --directory=DIR
  change to directory DIR

Although the wording isn't clear that, when used with -x, it extracts to 
the directory specified in -C, it does indeed behave that way.


Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works 
fine and extracts the foo.tar.gz from current directory to a target 
directory /tmp/bar/


---
unzip on both MacOS and CentOS:
---

unzip -v
UnZip 6.00 of 20 April 2009, by Info-ZIP.  Maintained by C. Spieler.

This version of the tool has:

[-d exdir]
  An optional directory to which to extract files.  By 
default, all files and sub-
  directories are recreated in the current directory; the 
-d option allows extrac-
  tion in an arbitrary directory (always assuming one has 
permission to  write  to
  the  directory).  This option need not appear at the end 
of the command line; it
  is also accepted before the zipfile specification (with  
the  normal  options),
  immediately  after  the zipfile specification, or between 
the file(s) and the -x
  option.  The option and directory may be concatenated 
without  any  white  space
  between  them,  but  note  that  this may cause normal 
shell behavior to be sup-
  pressed.  In particular, ``-d ~'' (tilde) is expanded by 
Unix C shells into  the
  name of the user's home directory, but ``-d~'' is treated 
as a literal subdirec-

  tory ``~'' of the current directory.

unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from 
current directory to /tmp/bar/


---
jimage and jmod
---

The jimage and jmod as you note use the --dir option for extracting to 
that specified directory.



Those were the tools I looked at. I think using the -d option with -x 
for the jar command is ruled out since it already is used for a 
different purpose, although for a different "main" operation of the jar 
command.


As for using --dir for this new feature, I don't think it alone will be 
enough. Specifically, I couldn't find a "short form" option for the 
--dir option in the jimage or jmod commands. For the jar extract feature 
that we are discussing here, I think having a short form option (in 
addition to the longer form) is necessary to have it match the usage 
expectations of similar other options that the jar command exposes. So 
even if we do choose --dir as the long form option, we would still need 
a short form for it and since -d is already taken for something else, we 
would still need to come up with a different one. The short form of this 
option could be -C (see below).



I think reusing the -C option, for this new feature, perhaps is a good 
thing. The -C is currently used by the update and create "main" 
operation of the jar command and the man page for this option states:


-C dir
  When creating (c) or updating (u) a JAR file, this option 
temporarily changes
  the directory while processing files specified by the 
file operands. Its
  operation is intended to be similar to the -C option of 
the UNIX tar utility.For
  example, the following command changes to the classes 
directory and adds the

  Bar.class file from that directory to my.jar:

  jar uf my.jar -C classes Bar.class


Using the -C option would indeed align it with the tar command.

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v2]

2021-02-27 Thread Attila Szegedi 
On Fri, 26 Feb 2021 08:03:26 GMT, Peter Levart  wrote:

>> The good test would be trying with all current GCs (Serial, Parallel, G1, 
>> Shenandoah, ZGC):
>> 
>> make run-test TEST=jdk/dynalink TEST_VM_OPTS=-XX:+UseSerialGC
>> 
>> Also, make sure that GH actions are able to run this test. You probably need 
>> to enable the workflow here -- https://github.com/szegedi/jdk/actions -- and 
>> then trigger the workflow manually on your branch (or push another commit). 
>> Then "Checks" tab would show the results.
>
>> > I would like 1st to understand why the MH created in the 
>> > TestLinker.convertToType is actually GCed at all.
>> 
>> The whole original issue was about allowing these objects to be GCd smile.
>> Short story: because all data is scoped to objects created within `makeOne` 
>> method.
> 
> Right, below description helped me understand the "chain of domination - i.e. 
> hard references that keep the BiClassValueRoot (extends ClassValue) 
> reachable". That doesn't explain fully why converters are not GC-ed as soon 
> as the associated BiClassValueRoot is eligible for GC. The secret lies in the 
> implementation of ClassValue...
> 
>> 
>> Longer story: It is GC'd because its reachability is dominated by the 
>> `DynamicLinker` object created on [line 
>> 96](https://github.com/openjdk/jdk/blob/3afec32bdf49703bb3537b36248f0550caae6d87/test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java#L96).
>>  It and everything it dominates (`LinkerServicesImpl` object holding the 
>> `TypeConverterFactory` object holding the `BiClassValue` objects holding the 
>> method handles) become garbage as soon as the method exits.
> 
> That is not entirely true. What becomes garbage is the objects leading to the 
> BiClassValueRoot (extends ClassValue) instance including the BiClassValueRoot 
> instance. But it is not the ClassValue instance that keeps the associated 
> T value(s) reachable. The associated values are strongly reachable from the 
> associated Class instances. The Class class contains an instance field of 
> type ClassValueMap which is responsible for keeping the associated values. 
> This answers my question why the associated converter is GCed only after 12 
> new converters are created for the same source/target type pair. It is not 
> because creating new converters would cause memory pressure and consequently 
> trigger GC (I tried adding explicit System.gc() calls and they did not 
> expedite the GCing of the converter). It is because ClassValue logic 
> expunges associated T values from Class.ClassValueMap only at times when it 
> has to re-arrange the map's internal structures and that eventually happens 
> on new inser
 tions for the same Class instance and different ClassValue instance.
> 
> That doesn't mean type converter caching is flawed in any way. It just means 
> that some converters will be retained even though they are practically not 
> "reachable" any more through the mechanism of caching. That doesn't mean that 
> any ClassLoader leaks are possible either.
> 
>> This works because of the pains I took in `BiClassValue` to ensure we don't 
>> leak any of the infrastructural objects through implicit `this$0` 
>> outer-class references into the `ClassValue`s. That allows the `ClassValue` 
>> objects to be GCd and removed from the classes' class-value mapping. This 
>> ordinarily doesn't happen as most `ClassValue` objects are bound to static 
>> fields, but in `TypeConverterFactory` the `BiClassValue` instances are 
>> specific to the `TypeConverterFactory` instance, so they are expected to be 
>> reclaimable by GC if the converter factory itself goes away.
>> 
>> > And after that, why it is not GCed before 12 invocations to makeOne() are 
>> > made.
>> 
>> Because GC is at liberty to delay recognizing an object is phantom 
>> reachable? I don't think we need to read too much into this? Correct me if I 
>> don't see something.
> 
> As described above, GC has nothing to do with this "delay". It is caused by 
> the way ClassValue keeps associated values reachable from the associated 
> Class instances (that appear semantically as keys, but in fact it's the other 
> way arround - ClassValue instances are weakly reachable keys and Class 
> instances hold the maps).
> 
>> 
>> > What would be more interesting to test is to create a converter between a 
>> > type loaded by a custom (child) ClassLoader and a system type. After that, 
>> > release all hard references to the custom ClassLoader and see if the 
>> > converter gets GC-ed as a result. WDYT?
>> 
>> That does sound like 
>> [TypeConverterFactoryRetentionTests.testSystemLoaderToOtherLoader](https://github.com/openjdk/jdk/blob/3afec32bdf49703bb3537b36248f0550caae6d87/test/jdk/jdk/dynalink/TypeConverterFactoryRetentionTests.java#L118)
>>  True, it tests whether the _class loader_ itself gets released, not the 
>> converter, but the loader would hardly get released while there's a 
>> converter with the class from that loader

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v2]

2021-02-27 Thread Attila Szegedi 
On Thu, 25 Feb 2021 15:34:10 GMT, Aleksey Shipilev  wrote:

>> Attila Szegedi has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR.
>
> test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 28:
> 
>> 26:  * @bug 8198540
>> 27:  * @summary Test TypeConverterFactory is not leaking method handles
>> 28:  * @run main/othervm -Xmx4M TypeConverterFactoryMemoryLeakTest
> 
> I think `-Xmx4m` is risking it on some platforms that cannot go that low 
> heap. Maybe do 128M, and bulk up the test allocations, so that GC definitely 
> triggers?

I removed the `-Xmx` option altogether on @plevart's suggestion to invoke 
`System.gc` explicitly instead. I added multiple @run directives, with all 
current GCs. (BTW, do I need to add `@requires vm.gc.Shenandoah` to the tests 
if I include Shenandoah?)

It's funny how ZGC and Shenandoah need 1 iteration less than all other GCs. If 
a test needs 12 iterations with most GCs, ZGC and Shenandoah need 11. If a test 
needs 2 iterations with others, these two will get it done in 1.

-

PR: https://git.openjdk.java.net/jdk/pull/2617

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v2]

2021-02-27 Thread Attila Szegedi 
On Thu, 25 Feb 2021 15:34:30 GMT, Aleksey Shipilev  wrote:

>> Attila Szegedi has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR.
>
> test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 79:
> 
>> 77: 
>> 78: public static void main(String[] args) {
>> 79: for (int count = 0; count < MAX_ITERATIONS; ++count) {
> 
> Here and later: use postfix `count++`, regular style?

Old habits die hard; I think I started doing this about 30 years ago when 
writing C code against a compiler on Atari ST that emitted more efficient 
MC68000 code for `++i` than for `i++`. I guess it's time to unlearn this :-) Of 
course, if you wanted to get the fastest code, you would've counted 
_decrementing to zero_ to let the compiler use the DBRA (decrement and branch) 
instruction.

It's funny how I _don't_ miss those days.

-

PR: https://git.openjdk.java.net/jdk/pull/2617

Re: RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" [v3]

2021-02-27 Thread Attila Szegedi 
> 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with 
> "AssertionError: Should have GCd a method handle by now"

Attila Szegedi has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  8261483: Eliminate flakiness of the tests by using iteration number limit and 
explicitly running GC

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2617/files
  - new: https://git.openjdk.java.net/jdk/pull/2617/files/3afec32b..f5922c48

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2617=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2617=01-02

  Stats: 25 lines in 2 files changed: 11 ins; 3 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2617.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2617/head:pull/2617

PR: https://git.openjdk.java.net/jdk/pull/2617

Re: RFR: 8173970: jar tool should have a way to extract to a directory

2021-02-27 Thread Lance Andersen 
HI Jaikiran,

I am more than happy to work with you through the CSR process

Lets get the discussion going regarding possible option names on core-libs-dev 
and then we can move forward :-)

Have a great weekend

On Feb 26, 2021, at 9:03 PM, Jaikiran Pai 
mailto:jai.forums2...@gmail.com>> wrote:


On 27/02/21 1:17 am, Lance Andersen wrote:

I believe this would also warrant a CSR to be created as well as updates to the 
jar man page.


I haven't created a CSR before, so I will need some guidance on that part. Is 
it usually created after all the implementation details have been decided upon 
or should it be created now?

-Jaikiran


[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v6]

2021-02-27 Thread Jie Fu 
> Hi all,
> 
> Vector API fails to work when:
>  - case 1: MaxVectorSize is set to <=8, or
>  - case 2: C2 is disabled
> 
> The reason is that {max/preferred} VectorShape initialization fails in both 
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns 
> unreasonable values (0 for case 1 and -1 for case 2).
> 
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
> 
> Testing:
>   - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
> 
> Thanks.
> Best regards,
> Jie
> 
> [1] 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364

Jie Fu has updated the pull request incrementally with one additional commit 
since the last revision:

  Add requires vm.compiler2.enabled

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2722/files
  - new: https://git.openjdk.java.net/jdk/pull/2722/files/b67b232d..79402411

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2722=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=04-05

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722

PR: https://git.openjdk.java.net/jdk/pull/2722

Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v5]

2021-02-27 Thread Jie Fu 
On Sat, 27 Feb 2021 10:58:16 GMT, Vladimir Ivanov  wrote:

>> Jie Fu has updated the pull request with a new target base due to a merge or 
>> a rebase. The incremental webrev excludes the unrelated changes brought in 
>> by the merge/rebase. The pull request contains seven additional commits 
>> since the last revision:
>> 
>>  - Fix in jdk/incubator/vector/VectorShape.java
>>  - Merge branch 'master' into JDK-8262096
>>  - Revert changes
>>  - Remove -XX:TieredStopAtLevel=3
>>  - Update the jtreg test
>>  - The numerator should be 8 (byte)
>>  - 8262096: Vector API fails to work due to VectorShape initialization 
>> exception
>
> test/jdk/jdk/incubator/vector/PreferredSpeciesTest.java line 42:
> 
>> 40:  * @modules jdk.incubator.vector java.base/jdk.internal.vm.vector
>> 41:  * @run testng/othervm -XX:MaxVectorSize=8 PreferredSpeciesTest
>> 42:  * @run testng/othervm -XX:MaxVectorSize=4 PreferredSpeciesTest
> 
> `-XX:MaxVectorSize` is C2-specific. It's better to specify either 
> `-XX:-IgnoreUnrecognizedVMOptions` or `@requires vm.compiler2.enabled`.

`@requires vm.compiler2.enabled` had been added.
Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/2722

Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v5]

2021-02-27 Thread Vladimir Ivanov 
On Sat, 27 Feb 2021 03:23:12 GMT, Jie Fu  wrote:

>> Hi all,
>> 
>> Vector API fails to work when:
>>  - case 1: MaxVectorSize is set to <=8, or
>>  - case 2: C2 is disabled
>> 
>> The reason is that {max/preferred} VectorShape initialization fails in both 
>> cases.
>> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns 
>> unreasonable values (0 for case 1 and -1 for case 2).
>> 
>> Vector API should not depend on C2 to run.
>> It should work even there is no JIT compiler since it's a Java-level api.
>> So let's fix it.
>> 
>> Testing:
>>   - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>> 
>> Thanks.
>> Best regards,
>> Jie
>> 
>> [1] 
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
>
> Jie Fu has updated the pull request with a new target base due to a merge or 
> a rebase. The incremental webrev excludes the unrelated changes brought in by 
> the merge/rebase. The pull request contains seven additional commits since 
> the last revision:
> 
>  - Fix in jdk/incubator/vector/VectorShape.java
>  - Merge branch 'master' into JDK-8262096
>  - Revert changes
>  - Remove -XX:TieredStopAtLevel=3
>  - Update the jtreg test
>  - The numerator should be 8 (byte)
>  - 8262096: Vector API fails to work due to VectorShape initialization 
> exception

> For the updated fix, the {max/preferred} shape will be initialized as 
> shape-64-bit if hotspot doesn't support vectorization.

Sounds reasonable.

test/jdk/jdk/incubator/vector/PreferredSpeciesTest.java line 42:

> 40:  * @modules jdk.incubator.vector java.base/jdk.internal.vm.vector
> 41:  * @run testng/othervm -XX:MaxVectorSize=8 PreferredSpeciesTest
> 42:  * @run testng/othervm -XX:MaxVectorSize=4 PreferredSpeciesTest

`-XX:MaxVectorSize` is C2-specific. It's better to specify either 
`-XX:-IgnoreUnrecognizedVMOptions` or `@requires vm.compiler2.enabled`.

-

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2722

Re: RFR: 8173970: jar tool should have a way to extract to a directory

2021-02-27 Thread Alan Bateman 

On 26/02/2021 19:47, Lance Andersen wrote:

Hi Jaikiran,

Thank you for  the proposed patch.

Assuming there is consensus to add support for this enhancement, I think we 
need to discuss what is the correct option.

The jar tool borrows -C from tar for creating/updating a jar and the -C option 
is also a valid option when extracting files from a tar file.

Perhaps keeping symmetry with tar and extend support for -C when extracting a 
jar file would be a better way forward.   Let’s give time for additional input.

I believe this would also warrant a CSR to be created as well as updates to the 
jar man page.

Best
Lance

p.s. I think it would be useful in the future to start the discussion on 
core-libs-dev prior to creating a PR (or leave it as a draft PR) for a feature 
request.

I created JDK-8173970 a few years ago so happy it it getting some attention.

Yes, the option name will need to be agreed. It would be useful to 
enumerate the options that the other tools are using to specify the 
location where to extract. If you see JBS issues mentioning tar -C not 
supporting chdir when extracting then it might be Solaris tar, which 
isn't the same as GNU tar which has different options. It might be 
better to look at more main stream tools, like unzip although jar -d is 
already taken. It would be nice if there were some consistency with 
other tools in the JDK that doing extracting (The jmod and jimage 
extract commands use use --dir for example).


There are other discussion points around the behavior when the target 
directory exists or does not exist, to ensure there is some consistency 
with main stream tools.


Yes, a CSR will be needed.

-Alan

Re: RFR: JDK-8262471: Fix coding style in src/java.base/share/classes/java/lang/CharacterDataPrivateUse.java

2021-02-27 Thread Alan Bateman 
On Fri, 26 Feb 2021 17:50:19 GMT, Christoph Göttschkes  wrote:

> Please review this small patch which fixes the coding style of 
> CharacterDataPrivateUse.java

Looks fine. This source file was a .template until a few weeks ago, probably 
should have fixed the indentation issues when moving it to a .java file.

-

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2754