Re: Code Review Request, 8152237 Support BigInteger.TWO

2016-03-23 Thread Attila Szegedi
Sorry for beating the dead horse of ObjectIdentifier.java change, but I’d suggest that if that code is later revisited, it be changed to "first.compareTo(BigInteger.TWO) > 0" instead of “… == 1”. Comparing the return value of compareTo to zero (instead of relying on specific set of return value

Re: JDK 9 proposal: allocating ByteBuffers on heterogeneous memory

2016-04-01 Thread Attila Szegedi
I can think of several differences. For one, you can’t presume the availability of a filesystem (Java doesn’t require that the host system give it access to a filesystem), nor the ability of the filesystem to expose all desired kinds of memory as files. Next, every such solution is OS specific a

Re: RFR: 8155600 - Performance optimization of Arrays.asList().iterator()

2016-04-28 Thread Attila Szegedi
On Thu, Apr 28, 2016 at 3:52 PM, Aleksey Shipilev < aleksey.shipi...@oracle.com> wrote: > *) next(): Missing braces in throw new NSEE() block; > *) next(): Why loading this.a into local? I agree with Aleksey but I'd go as far as to say that not only this.a is unnecessary, but i as well. The bo

Re: MethodHandle for array length

2016-05-12 Thread Attila Szegedi
Hi Uwe, If you’re targetting JDK 9 only, you could also consider basing your language’s dynamic operations on Dynalink; it’s integrated into JDK 9 and is specifically meant as a way to ease implementation of languages that have dynamic types. It happens to have a predefined GET_LENGTH[1] operat

Re: Add Predicate.of(), Consumer.of(), etc.

2015-05-06 Thread Attila Szegedi
On May 6, 2015, at 12:37 PM, Paul Sandoz wrote: > > > On May 2, 2015, at 11:31 PM, Remi Forax wrote: > >> Hi all, >> today, I stubble on a variant of JDK-8050818 [1], >> trying to call negate() on a lambda which is not yet a Predicate (due to >> target typing) which requires either to cast th

AbstractList etc. functionality as interfaces with default methods?

2015-05-08 Thread Attila Szegedi
So I’m in a position where I’d need to have a class implement List, but it already extends something else, so I can’t have it extend AbstractList, which leaves me with a lot of boilerplate methods to implement. Would it seem like a good idea to reimagine AbstractList and friends as interfaces w

Re: AbstractList etc. functionality as interfaces with default methods?

2015-05-08 Thread Attila Szegedi
Well then those would obviously be left out, documenting that it’s the responsibility of the implementor to provide them. This would be true in general for other interfaces as well that have a specification for expected meaning of equals/hashCode. There’s still a lot of boilerplate code that co

Re: RFR 8072002: The spec on javax.script.Compilable contains a typo and confusing inconsistency

2015-05-19 Thread Attila Szegedi
+1 > On May 19, 2015, at 2:15 PM, A. Sundararajan > wrote: > > Please review http://cr.openjdk.java.net/~sundar/8072002/webrev.00/ for > https://bugs.openjdk.java.net/browse/JDK-8072002 > > Thanks, > -Sundar

Re: RFR 8068978: All versions of javax.script.ScriptEngine.eval(...) method may clarify ScriptException throwing

2015-05-25 Thread Attila Szegedi
+1 > On May 25, 2015, at 2:54 PM, A. Sundararajan > wrote: > > Please review http://cr.openjdk.java.net/~sundar/8068978/webrev.00/ for > https://bugs.openjdk.java.net/browse/JDK-8068978 > > Thanks, > -Sundar

Re: RFR: 8098547: (tz) Support tzdata2015e

2015-06-25 Thread Attila Szegedi
FWIW, he do have one new test failure in Nashorn now, it seems related. Can you confirm it is caused by your changes? [testng] Test test/script/basic/NASHORN-627.js failed at line 1 - [testng] expected: 'Sun Dec 21 1969 00:00:00 GMT+0100 (CET) -95400 1969-12-20T23:00:00.000Z' [test

Re: RFR: 8098547: (tz) Support tzdata2015e

2015-06-25 Thread Attila Szegedi
. > 2015e tzdata changes haven't been pushed to jdk9-dev forest yet. > > Where the 1969 date coming from ? Is there some rollover calculation > happening ? > > Regards, > Sean. > > On 25/06/2015 09:05, Attila Szegedi wrote: >> FWIW, he do have one new test fa

Re: RFR: 8098547: (tz) Support tzdata2015e

2015-06-25 Thread Attila Szegedi
gt; > Looks like it's a regression caused by the fix to 8008577, where the default > locale data switched to Unicode Consortium's CLDR. Would you please file an > issue? > > Naoto > > On 6/25/15 5:49 AM, Attila Szegedi wrote: >> Yeah, basically instantiating a JavaS

Re: JEP 276: Dynamic Linking of Language-Defined Object Models

2015-10-18 Thread Attila Szegedi
> On Oct 18, 2015, at 10:49 AM, Jochen Theodorou wrote: > > On 17.10.2015 13:30, Martijn Verburg wrote: >> This looks very, very promising. Would it help to get the language >> maintainers of the most popular scripting/dynamic JVM languages involved >> ASAP? Happy to contact Groovy, Clojure, S

Re: JEP 276: Dynamic Linking of Language-Defined Object Models

2015-10-18 Thread Attila Szegedi
Sure. For what’s it worth, this is for all practical purposes Dynalink, in the form I’ve been developing for years as a standalone library on GitHub and presenting about it at JVM Language Summits over several years, so I’d expect a significant amount of awareness is there. The primary motivat

Re: JEP 276: Dynamic Linking of Language-Defined Object Models

2015-10-19 Thread Attila Szegedi
On Oct 19, 2015, at 10:46 AM, Jochen Theodorou wrote: > since it is dynalink there is I guess only one master linker in the end. Can > you point me to some code showing how the composition of linkers is done to > refresh my memory on that? Sure, here’s how Nashorn does it: http://hg.openjdk.j

Re: JEP 276: Dynamic Linking of Language-Defined Object Models

2015-10-20 Thread Attila Szegedi
> On Oct 20, 2015, at 10:17 AM, Jochen Theodorou wrote: > > On 19.10.2015 12:56, Attila Szegedi wrote: >> On Oct 19, 2015, at 10:46 AM, Jochen Theodorou > <mailto:blackd...@gmx.org>> wrote: >> >>> since it is dynalink there is I guess only one master

Re: JEP 276: Dynamic Linking of Language-Defined Object Models

2015-10-22 Thread Attila Szegedi
Folks, here’s some updates on this JEP, mostly making the code and docs available to the community for evaluation. Where’s the code? = The JDK 9 sandbox repo now has a branch named “JEP-276-branch” where the work is ongoing. If you aren’t familiar with the JDK 9 sandbox, you can

Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

2015-11-19 Thread Attila Szegedi
Please review JDK-8141338 "Move jdk.internal.dynalink package to jdk.dynalink" for . This is basically the implementation step for integrating JEP 276. This changeset will introduce a new public API that has CCC approval (request 8075866), and is

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

2015-11-20 Thread Attila Szegedi
rs. Since you are on this file, can you move jdk.scripting.nashorn >> to MAIN_MODULES as well? >> >> Mandy >> >>> On Nov 19, 2015, at 3:15 PM, Attila Szegedi wrote: >>> >>> Please review JDK-8141338 "Move jdk.internal.dynalink package to

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

2015-11-20 Thread Attila Szegedi
On Nov 20, 2015, at 4:10 PM, Alan Bateman wrote: > > On 19/11/2015 23:15, Attila Szegedi wrote: >> Please review JDK-8141338 "Move jdk.internal.dynalink package to >> jdk.dynalink" for <https://bugs.openjdk.java.net/browse/JDK-8141338>. This >>

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

2015-11-23 Thread Attila Szegedi
ugh to verify that JDK9 builds fine with these changes. Thanks, Attila. > On Nov 20, 2015, at 4:41 PM, Attila Szegedi wrote: > > Thanks for pointing out these, Mandy; I will make the changes you suggested. > > Attila. > >> On Nov 20, 2015, at 6:10 AM, Mandy Chung wrote:

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

2015-11-23 Thread Attila Szegedi
> On Nov 23, 2015, at 4:40 PM, Alan Bateman wrote: > > > > On 23/11/2015 15:27, Attila Szegedi wrote: >> Folks, >> >> I integrated the changes Mandy suggested; please review these (build >> related) changes: >> jdk9 top level: >> <http

Re: [9] RFR (XS): 8060483: NPE with explicitCastArguments unboxing null

2014-10-15 Thread Attila Szegedi
+1 On Wed, Oct 15, 2014 at 2:12 PM, Vladimir Ivanov < vladimir.x.iva...@oracle.com> wrote: > http://cr.openjdk.java.net/~vlivanov/8060483/webrev.00/ > https://bugs.openjdk.java.net/browse/JDK-8060483 > > Recent changes broke MHs.explicitCastArguments for null values when > unboxing taking place.

Re: [9] RFR (XS): 8060483: NPE with explicitCastArguments unboxing null

2014-11-02 Thread Attila Szegedi
Thanks for fixing this. Can we expect a backport to 8u-dev soon? On Oct 15, 2014, at 4:46 PM, Attila Szegedi wrote: > +1 > > On Wed, Oct 15, 2014 at 2:12 PM, Vladimir Ivanov < > vladimir.x.iva...@oracle.com> wrote: > >> http://cr.openjdk.java.net/~vlivanov/

Re: [concurrency-interest] We need to add blocking methods to CompletionStage!

2016-09-26 Thread Attila Szegedi
Not at all, you could just have a call to cancel() block until the future completes. *ducks* Attila. > On 25 Sep 2016, at 16:34, Viktor Klang wrote: > > If that truely is the case then the only way of implementing a readonly > Future is by throwing an exception from cancel... > > -- > Cheer

Re: RFR: 8166365: Small immutable collections should provide optimized implementations when possible

2017-01-11 Thread Attila Szegedi
Collections.java: 4987 @Override 4988 public int hashCode() { 4989 return (k == null ? 0 : k.hashCode()) ^ 4990(v == null ? 0 : v.hashCode()); Why not use Objects.hashCode here too, that is return Objects.hashCode(k) ^ Objects.hashCode(v) ? Attila

Re: java.util.Pair

2017-07-15 Thread Attila Szegedi
In application code I agree that semantics-free tuples are a code smell, but if you're developing a library of generally usable algorithms a need for a type that is a product of two types can naturally arise, without further semantics provided (or required) by the context. In a sense the existe

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-02-07 Thread Attila Szegedi
On Sun, 10 Jan 2021 23:30:58 GMT, Peter Levart wrote: >>> How frequent are situations where the two classloaders are unrelated? >> >> I think they'd be extremely unlikely. The only user of these right now is >> Dynalink's type converter factory. I _can imagine_ a situation where there's >> a c

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-02-09 Thread Attila Szegedi
On Tue, 9 Feb 2021 10:57:05 GMT, Hannes Wallnöfer wrote: >> Attila Szegedi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Rename ClassLoaderRelation to RetentionDirection; it is better >> self-document

Integrated: 8198540: Dynalink leaks memory when generating type converters

2021-02-09 Thread Attila Szegedi
On Sat, 2 Jan 2021 14:45:51 GMT, Attila Szegedi wrote: > _(NB: For this leak to occur, an application needs to be either creating and > discarding linkers frequently, or creating and discarding class loaders > frequently while creating Dynalink type converters for classes in them.

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

2021-02-17 Thread Attila Szegedi
8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" - Commit messages: - 8261483: Try to eliminate flakiness of the test by extending its allowed runtime and reducing the memory Changes: https://git.ope

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

2021-02-18 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 differen

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

2021-02-18 Thread Attila Szegedi
On Wed, 17 Feb 2021 19:44:35 GMT, Attila Szegedi wrote: > 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with > "AssertionError: Should have GCd a method handle by now" @plevart can I bother you for a follow-up review of my original issue? (Alternat

Re: RFR: 8261290: Improve error message for NumberFormatException on null input

2021-02-21 Thread Attila Szegedi
On Sat, 20 Feb 2021 21:44:12 GMT, Joe Darcy wrote: > In Integer and Long, several of the parsing methods are specified to throw > NumberFormatException on a null input rather than an NPE. That behavior is > not proposed to be modified. However, I think it is reasonable for the > NumberFormatEx

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

2021-02-21 Thread Attila Szegedi
On Fri, 19 Feb 2021 11:22:57 GMT, Peter Levart wrote: >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 😄. Short story: because all data is scoped to objects created wi

Class.getRecordComponents security checks

2021-02-21 Thread Attila Szegedi
Hey folks, Why are security checks for Class.getRecordComponents as strict as those for e.g. getDeclaredMethods? I would’ve expected they’d be as strict as those for e.g. getMethods. Specifically, the difference is the: > “the caller's class loader is not the same as the class loader of this cl

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 differen

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. > &g

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. > &g

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 proba

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

2021-02-28 Thread Attila Szegedi
> 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with > "AssertionError: Should have GCd a method handle by now" Attila Szegedi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: 82

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

2021-02-28 Thread Attila Szegedi
On Sun, 28 Feb 2021 07:20:19 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. > &g

RFR: 8262503: Support records in Dynalink

2021-03-01 Thread Attila Szegedi
8262503: Support records in Dynalink - Commit messages: - Document behavior changes - Add support for records to Dynalink - Test the desired functionality Changes: https://git.openjdk.java.net/jdk/pull/2767/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2767&range=00

Integrated: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"

2021-03-02 Thread Attila Szegedi
On Wed, 17 Feb 2021 19:44:35 GMT, Attila Szegedi wrote: > 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with > "AssertionError: Should have GCd a method handle by now" This pull request has now been integrated. Changeset: d185a6c5 Author: Att

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

2021-03-02 Thread Attila Szegedi
On Mon, 1 Mar 2021 11:09:31 GMT, Peter Levart wrote: >> Attila Szegedi 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 one add

Re: Class.getRecordComponents security checks

2021-03-20 Thread Attila Szegedi
> On 2021. Feb 21., at 21:57, Remi Forax wrote: > > - Mail original - >> De: "Attila Szegedi" >> À: "core-libs-dev" >> Envoyé: Dimanche 21 Février 2021 21:14:48 >> Objet: Class.getRecordComponents security check

Re: RFR: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation

2021-03-27 Thread Attila Szegedi
On Sat, 27 Mar 2021 15:19:37 GMT, Attila Szegedi wrote: > I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods > had a lot of code duplication among themselves, and even within each method. > I refactored them into a modern unified implementation. While the

RFR: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation

2021-03-27 Thread Attila Szegedi
I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods had a lot of code duplication among themselves, and even within each method. I refactored them into a modern unified implementation. While there I also took the opportunity to introduce `Objects.requireNonNull` in place o

Re: RFR: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation [v2]

2021-03-28 Thread Attila Szegedi
On Sat, 27 Mar 2021 15:46:36 GMT, Alan Bateman 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. The pull reque

Re: RFR: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation [v2]

2021-03-28 Thread Attila Szegedi
quireNonNull` in place of null > checks followed by NPE throws, mark private fields final where possible, use > lambdas for `doPrivileged` block, use `List.of` and `List.copyOf` where > possible, and generally sanitize/deduplicate. Attila Szegedi has refreshed the contents of this pull

Re: RFR: 8262503: Support records in Dynalink

2021-03-30 Thread Attila Szegedi
On Tue, 30 Mar 2021 12:38:41 GMT, Athijegannathan Sundararajan wrote: >> 8262503: Support records in Dynalink > > Looks good Thank you for the review, @sundararajana! - PR: https://git.openjdk.java.net/jdk/pull/2767

Integrated: 8262503: Support records in Dynalink

2021-03-30 Thread Attila Szegedi
On Sun, 28 Feb 2021 16:46:31 GMT, Attila Szegedi wrote: > 8262503: Support records in Dynalink This pull request has now been integrated. Changeset: b08d6383 Author: Attila Szegedi URL: https://git.openjdk.java.net/jdk/commit/b08d6383 Stats: 165 lines in 8 files changed: 153

Re: RFR: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation [v2]

2021-03-30 Thread Attila Szegedi
On Tue, 30 Mar 2021 12:43:18 GMT, Athijegannathan Sundararajan 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

Integrated: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation

2021-03-30 Thread Attila Szegedi
On Sat, 27 Mar 2021 15:19:37 GMT, Attila Szegedi wrote: > I noticed that `javax.script.ScriptEngineManager` `getEngineByXxx` methods > had a lot of code duplication among themselves, and even within each method. > I refactored them into a modern unified implementation. While the

Re: Review CSR JDK-8266760: Remove sun.misc.Unsafe::defineAnonymousClass

2021-05-11 Thread Attila Szegedi
I'm fine with the removal, although if you remember we use it in Nashorn too[0], and when I tried to replace it with the hidden classes facility the issue we run into was that hidden classes' methods didn't show up in stack traces the way anonymous classes' methods did[1]. Since Nashorn uses anonym

Re: JDK Dynalink only works with unconditionally exported packages

2021-05-11 Thread Attila Szegedi
On 2021. May 11., at 1:51, Thiago Henrique Hupner wrote: > > Hi all. > > I've been testing the JDK Dynalink recently and > I think I've found a bug. > > The class jdk.dynalink.beans.CheckRestrictedPackage checks > if a package is restricted. > > So, I have a class that has some static methods.

Re: RFR: 8268113: Re-use Long.hashCode() where possible [v9]

2021-07-13 Thread Attila Szegedi
On Wed, 2 Jun 2021 14:11:07 GMT, Сергей Цыпанов wrote: >> Сергей Цыпанов 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 ten additional >> comm

Comparator.reversed() type inference issue

2020-06-07 Thread Attila Szegedi
Hi folks, I'm teaching Java lately, and while teaching streams I use that good old chestnut, the word count example. I'm baffled with some type inference woes, though… trying to specify a reverse comparator using Comparator.reversed() makes javac sad, e.g. this does not compile[1]: Map

Review request for JDK-8252124: Restore Dynalink tests

2020-08-20 Thread Attila Szegedi
Hi folks, long time since I actively popped up here. I’m taking some time to maintain Dynalink, eyeing Java 16 for the changes. Since the nashorn-dev list is (presumably? I haven’t bother checking) defunct, I asked around and Sundar suggested core-libs-dev is the right list to post review reque

Review request for JDK-8251538: Modernize and lint Dynalink code

2020-08-20 Thread Attila Szegedi
Following up on the previous e-mail, here’s the modernization and linting work on the existing Dynalink codebase: Please review JDK-8251538 "Modernize and lint Dynalink code" at for The

Re: Review request for JDK-8251538: Modernize and lint Dynalink code

2020-08-23 Thread Attila Szegedi
On 2020. Aug 20., at 23:43, Remi Forax wrote: > > - Mail original - >> De: "Attila Szegedi" >> À: "core-libs-dev" >> Envoyé: Jeudi 20 Août 2020 22:40:53 >> Objet: Review request for JDK-8251538: Modernize and lint Dynalink code >

Re: Review request for JDK-8252124: Restore Dynalink tests

2020-08-23 Thread Attila Szegedi
ate copyright year on tests - > especially because there have been changes like package removal (flat). > > -Sundar > > >> On 2020. Aug 20., at 22:20, Attila Szegedi wrote: >> >> Hi folks, >> >> long time since I actively popped up here. I’m ta

RFR: 8198540: Dynalink leaks memory when generating type converters

2021-01-02 Thread Attila Szegedi
_(NB: For this leak to occur, an application needs to be either creating and discarding linkers frequently, or creating and discarding class loaders frequently while creating Dynalink type converters for classes in them. Neither is typical usage, although they can occur in dynamic code loading e

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v2]

2021-01-02 Thread Attila Szegedi
hat when a `TypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by vir

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v3]

2021-01-02 Thread Attila Szegedi
hat when a `TypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by virtu

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

2021-01-08 Thread Attila Szegedi
On Mon, 4 Jan 2021 13:31:41 GMT, Peter Levart wrote: >> Just a small suggestion using `MethodHandles.empty` instead of >> `MethodHandles.constant().asType().dropArguments()`. > > Hi Attila, > > This looks good. > > If I understand correctly, your `BiClassValue` is a holder for a `BiFunction`

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

2021-01-08 Thread Attila Szegedi
ypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by virtue of cr

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

2021-01-08 Thread Attila Szegedi
On Fri, 8 Jan 2021 11:06:17 GMT, Peter Levart wrote: > So what do you think of this variant: I like it. I originally kept the fields volatile so that we don't observe stale values on `getForward`/`getReverse`, but you're right in that even if we do, the correct value will be observed when doin

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Attila Szegedi
ypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by virtue of creatin

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

2021-01-08 Thread Attila Szegedi
On Fri, 8 Jan 2021 13:32:08 GMT, Peter Levart wrote: >>> IIUC, your changes mostly all flow from the decision to declare the fields >>> as non-volatile; if they were still declared as volatile then it'd be >>> impossible to observe null in them, I think (correct me if I'm wrong; it >>> seems l

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v6]

2021-01-10 Thread Attila Szegedi
ypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by virtue of crea

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-01-10 Thread Attila Szegedi
ypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by vir

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-01-10 Thread Attila Szegedi
On Fri, 8 Jan 2021 18:58:03 GMT, Peter Levart wrote: >> So, are you saying the solution where I kept the fields volatile and >> initialized them with `Map.of()` is safe? If so, that'd be good news; I'm >> inclined these days to write as much null-free code as possible :-) > > Yes, the pre-initi

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

2021-01-10 Thread Attila Szegedi
On Sun, 10 Jan 2021 17:18:37 GMT, Peter Levart wrote: > How frequent are situations where the two classloaders are unrelated? I think they'd be extremely unlikely. The only user of these right now is Dynalink's type converter factory. I _can imagine_ a situation where there's a conversion from

Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v2]

2021-01-10 Thread Attila Szegedi
On Sun, 10 Jan 2021 16:35:17 GMT, Johannes Kuhn wrote: >> Simple fix - one line change: >> https://openjdk.github.io/cr/?repo=jdk&pr=2000&range=00#sdiff-0. >> >> Most of the changes come from an additional test that fails without this fix: >> >> Error: Unable to load main class somelib.t

Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]

2021-01-11 Thread Attila Szegedi
On Mon, 11 Jan 2021 05:12:11 GMT, Johannes Kuhn wrote: >> Simple fix - one line change: >> https://openjdk.github.io/cr/?repo=jdk&pr=2000&range=00#sdiff-0. >> >> Most of the changes come from an additional test that fails without this fix: >> >> Error: Unable to load main class somelib.t