Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v24]

[liach]

On Tue, 14 Jun 2022 01:25:02 GMT, Joe Darcy  wrote:

>> This is an early review of changes to better model JVM access flags, that is 
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>> 
>> Language level modifiers and JVM level access flags are closely related, but 
>> distinct. There are concepts that overlap in the two domains (public, 
>> private, etc.), others that only have a language-level modifier (sealed), 
>> and still others that only have an access flag (synthetic).
>> 
>> The existing java.lang.reflect.Modifier class is inadequate to model these 
>> subtleties. For example, the bit positions used by access flags on different 
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
>> methods. Just having a raw integer does not provide sufficient context to 
>> decode the corresponding language-level string. Methods like 
>> Modifier.methodModifiers() were introduced to cope with this situation.
>> 
>> With additional modifiers and flags on the horizon with projects like 
>> Valhalla, addressing the existent modeling deficiency now ahead of time is 
>> reasonable before further strain is introduced.
>> 
>> This PR in its current form is meant to give the overall shape of the API. 
>> It is missing implementations to map from, say, method modifiers to access 
>> flags, taking into account overlaps in bit positions.
>> 
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
>> once the API is further along.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Make mask fields final in ModuleDescriptor.

src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 127:

> 125:  * 0x0020}.
> 126:  */
> 127: SUPER(0x_0020, false, Set.of(Location.CLASS)),

Should we document that this flag won't appear in `Class#accessFlags` no matter 
if it's declared in the class file?

src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 300:

> 298: /**
> 299:  * {@return a set of access flags for the given mask value
> 300:  * appropriate for the location in question}

Should we specify that the returned set is unmodifiable/immutable?

-

PR: https://git.openjdk.org/jdk/pull/7445

Re: RFR: 8287186: JDK modules participating in preview [v2]

[liach]

On Wed, 8 Jun 2022 18:24:35 GMT, Paul Sandoz  wrote:

>> Allow JDK modules that use preview features (preview language features or 
>> preview API features from dependent modules) to participate without the need 
>> to compile with `--enable-preview`.
>> 
>> It's difficult to enable participation using an annotation due to the nature 
>> in which symbols are encountered when processing source as there is no 
>> guaranteed order to the processing of certain symbols.
>> 
>> Instead a JDK module participates if the `java.base` package 
>> `jdk.internal.javac` is exported to that module (@lahodaj clever idea!). An 
>> internal annotation `jdk.internal.javac.ParticipatesInPreview` can be 
>> declared on the module. Such a declaration cannot be enforced but does by 
>> its use require the `jdk.internal.javac`'s export list to be updated.
>> 
>> The modules `jdk.incubator.vector` and `jdk.incubator.concurrent` have been 
>> updated accordingly, both of which participate in preview APIs (APIs in 
>> `java.lang.foreign` and `Thread.ofVirtual`, respectively).
>
> Paul Sandoz has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Let java.management participate in preview features.

Just curious, is it still up to incubator modules' discretion to avoid 
accidental user access to preview content via the modules without enabling 
preview, like the `PreviewFeatures.ensureEnabled()` in `StructuredTaskScope`?

-

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

Re: RFR: JDK-8287838: Update Float and Double to use snippets

[liach]

On Sun, 5 Jun 2022 21:19:37 GMT, Joe Darcy  wrote:

> Various code blocks in Float and Double would be better as snippets.

src/java.base/share/classes/java/lang/Double.java line 643:

> 641:  * expression below can be used to screen the input string:
> 642:  *
> 643:  *  {@snippet lang="java" :

Mind remove the extra space before `{`?

-

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

Re: RFR: 8287860: Revise usage of volatile in j.u.Locale

[liach]

On Mon, 6 Jun 2022 13:28:44 GMT, Сергей Цыпанов  wrote:

>> src/java.base/share/classes/java/util/Locale.java line 2260:
>> 
>>> 2258:  * Calculated hashcode
>>> 2259:  */
>>> 2260: private transient volatile int hashCodeValue;
>> 
>> We can additionally annotate such cache fields with `@Stable` so as to 
>> enable constant-folding optimizations from the hotspot JIT.
>
> Shouldn't the fields annotated with `@Stable` be `final` as well?

These fields can only be written once besides the default values, but they 
don't have to be written in the static initializer or constructor. So when a 
non-zero hash code is written, it's cached as if it's a final field. For a zero 
value, it would always undergo extra calculations like before and write 
multiple times, but the writes don't matter as it would only write zero, which 
would not yield false hash code if a previously written 0 value is cached.

-

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

Re: RFR: 8287860: Revise usage of volatile in j.u.Locale

[liach]

On Mon, 6 Jun 2022 13:32:21 GMT, liach  wrote:

>> Shouldn't the fields annotated with `@Stable` be `final` as well?
>
> These fields can only be written once besides the default values, but they 
> don't have to be written in the static initializer or constructor. So when a 
> non-zero hash code is written, it's cached as if it's a final field. For a 
> zero value, it would always undergo extra calculations like before and write 
> multiple times, but the writes don't matter as it would only write zero, 
> which would not yield false hash code if a previously written 0 value is 
> cached.

> Shouldn't the fields annotated with `@Stable` be `final` as well?

You might have seen `@Stable final` fields. Those are only meaningful for 
arrays, where such caching happens at each array index. For non-arrays, 
`@Stable final` is simply equivalent to `final`, and simple `@Stable` acts as 
what I said above.

-

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

Re: RFR: 8287860: Revise usage of volatile in j.u.Locale

[liach]

On Mon, 6 Jun 2022 12:58:39 GMT, Сергей Цыпанов  wrote:

> - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be 
> volatile, even in case of race in the worst case it is recalculated at most 
> once per thread
> - `defaultLocale` field is read multiple times in `initDefault()`
> - `isoLanguages` is accessed multiple times in `getISOLanguages()`
> - `languageTag` is read twice in `toLanguageTag()`

src/java.base/share/classes/java/util/Locale.java line 2260:

> 2258:  * Calculated hashcode
> 2259:  */
> 2260: private transient volatile int hashCodeValue;

We can additionally annotate such cache fields with `@Stable` so as to enable 
constant-folding optimizations from the hotspot JIT.

-

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

Re: RFR: 8287785: Reduce runtime of java.lang.invoke microbenchmarks

[liach]

On Fri, 3 Jun 2022 11:16:29 GMT, Claes Redestad  wrote:

> - Add explicit run configurations to java.lang.invoke micros, aiming to 
> reduce runtime while maintaining a decently high confidence that there's 
> enough warmup to produce good enough data.
> 
> - Remove several trivial baseline micros, mainly those that only return a 
> static object: It's reasonable to have baseline microbenchmarks when the 
> baseline op is complex and you're mostly interested in checking the overhead 
> of doing the same thing via some MH API, but blackhole operations are now 
> shortcutting very quickly and timings doesn't differ from one type of object 
> to another, so we don't need a multitude of such baseline tests.
> 
> Estimated runtime of `make test TEST=micro:java.lang.micro` (excluding build) 
> drops from just above 28 to just above 3 hours.

test/micro/org/openjdk/bench/java/lang/invoke/LookupAcquire.java line 59:

> 57: @Setup
> 58: public void setup() {
> 59: cached = MethodHandles.lookup();

Since the benchmark using the cached object is removed, should this field be 
removed as well?

-

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

Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]

[liach]

On Thu, 2 Jun 2022 15:19:44 GMT, Сергей Цыпанов  wrote:

>> The clone is needed - as the `List>` may be a custom implementation 
>> of List - so you cannot make any assumption on the concrete implementation 
>> of `toArray`.
>
> But we can make an assumption on the behavior of particular method and the 
> JavaDoc is clear about it. If a user passes an incorrect implementation of 
> the `List` interface then it's their own responsibility.

We can't rule out malicious code holding the array reference it returns in 
toArray and updates that array to cause damage.

-

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

Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]

[liach]

On Fri, 27 May 2022 18:40:32 GMT, XenoAmess  wrote:

>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   do it as naotoj said

'the new' fix should be applied to newHashMap etc. too.

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

On Thu, 21 Apr 2022 00:48:00 GMT, Stuart Marks  wrote:

>> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
>> values by identity. Updated API documentation of these two methods 
>> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>>  to mention such behavior.
>
> Thanks for working on this. Yes I can review and sponsor this change.
> 
> Sorry I got a bit distracted. I started looking at the test here, and this 
> lead me to inquire about what other tests we have for `IdentityHashMap`, and 
> the answer is: not enough. See 
> [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295). (But that 
> should be handled separately.)

@stuart-marks Just curious, would this fix target 19 or 20 at the current state?

-

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v6]

[liach]

On Tue, 31 May 2022 19:33:49 GMT, Mandy Chung  wrote:

>> liach has updated the pull request incrementally with one additional commit 
>> since the last revision:
>> 
>>   Fixes suggested by mandy
>
> Looks good.  Thanks.

@mlchung Would you mind sponsoring this patch?

-

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]

[liach]

On Fri, 20 May 2022 00:10:01 GMT, Mandy Chung  wrote:

>> liach 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 four additional commits since 
>> the last revision:
>> 
>>  - Updates suggested by mandy
>>  - Merge branch 'master' into fix/proxy-single-pass
>>  - Don't need to complexify module cache
>>  - 8284942: Proxy building can just iterate superinterfaces once
>
> Looks good with a couple comments.

@mlchung Would you review again that I've updated per your suggestions? In 
addition, I've moved all proxy class context-related validation into the record 
compact constructor.

-

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

Integrated: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization

[liach]

On Fri, 20 May 2022 03:50:58 GMT, liach  wrote:

> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
> `Class.forName(String)`. `make test 
> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
> `LazyInitializationTest` failing the eager initialization check on the 
> baseline and passing with this patch.
> 
> On a side note, this might reduce the number of methods that can be encoded 
> in a proxy due to code attribute size restrictions; we probably would address 
> that in another issue, as we never mandated a count of methods that the proxy 
> must be able to implement.
> 
> Mandy, would you mind review this?

This pull request has now been integrated.

Changeset: e0382c55
Author:liach 
Committer: Roger Riggs 
URL:   
https://git.openjdk.java.net/jdk/commit/e0382c552348d108e906792ad8ca7067f9f805ec
Stats: 76 lines in 2 files changed: 72 ins; 0 del; 4 mod

8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid 
unnecessary class initialization

Reviewed-by: rriggs, mchung

-

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

Integrated: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

[liach]

On Fri, 20 May 2022 04:55:37 GMT, liach  wrote:

> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
> hash map with a simple lookup, similar to what's done in 
> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

This pull request has now been integrated.

Changeset: 37a51300
Author:liach 
Committer: Roger Riggs 
URL:   
https://git.openjdk.java.net/jdk/commit/37a513003c654297d81fc71b64c604f0ab8075cb
Stats: 86 lines in 1 file changed: 19 ins; 39 del; 28 mod

8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

Reviewed-by: rriggs, mchung

-

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

Withdrawn: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

On Sun, 17 Apr 2022 16:17:30 GMT, liach  wrote:

> Convert dynamic proxies to hidden classes. Modifies the serialization of 
> proxies (requires change in "Java Object Serialization Specification"). Makes 
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
> 
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of 
> the class is allocated... The contents restored appropriately" section, I 
> propose explicitly state that handling of proxies are unspecified as to allow 
> implementation freedom, though I've seen deliberate attempts for proxies to 
> implement interfaces with `readResolve` in order to control their 
> serialization behavior.
>- This is for the existing generated constructor accessor is 
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization 
> constructor calls `invokespecial` on the closest serializable superclass' 
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and 
> should be discouraged in the long term. Calling the existing constructor with 
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in 
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method, 
> implemented by the lambda, will not appear in the stack trace, and isn't too 
> problematic.
> 
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into 
> one. This reduced a lot of code and allowed anchor class (for hidden class 
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of 
> `java.base`. This API is intended for implementation of legacy (pre 
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more 
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959): 
> passes methods computed by proxy generator as class data to the hidden proxy 
> class to reduce generated proxy class size and improve performance.
> 
> In addition, since this change is somewhat large, should we keep the old 
> proxy generator as well and have it toggled through a command-line flag (like 
> the old v49 proxy generator or the old reflection implementation)?
> 
> Please feel free to comment or review. This change definitely requires a CSR, 
> but I have yet to determine what specifications should be changed.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8287353: Use snippet tag instead of pre tag in Javadoc of InterruptedException

[liach]

On Tue, 26 Apr 2022 15:04:15 GMT, Thiago Henrique Hüpner 
 wrote:

> 8287353: Use snippet tag instead of pre tag in Javadoc of InterruptedException

src/java.base/share/classes/java/lang/InterruptedException.java line 35:

> 33:  * this exception.  The following code can be used to achieve
> 34:  * this effect:
> 35:  * {@snippet:

I recommend using `{@snippet lang=java :` instead, like in 
https://github.com/openjdk/jdk/blob/6520843f86f638fe4d1e5b3358fab5799daca654/src/java.compiler/share/classes/javax/tools/JavaFileManager.java#L84

-

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

Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles

[liach]

On Fri, 27 May 2022 14:38:27 GMT, Claes Redestad  wrote:

>> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5266:
>> 
>>> 5264:  */
>>> 5265: public static MethodHandle dropArguments(MethodHandle target, int 
>>> pos, List> valueTypes) {
>>> 5266: return dropArguments(target, pos, valueTypes.toArray(new 
>>> Class[0]).clone(), true);
>> 
>> Isn't this call to `clone()` unnecessary, as `valueTypes.toArray` should 
>> either return the passed empty array, or a newly created array?
>
> It might be a bit too paranoid in this instance (since we don't keep the 
> array around for long), but not cloning the result of calling `toArray` on an 
> arbitrary and possibly adversary `List` could open up for TOCTOU race bugs / 
> attacks. The existing code was being paranoid and copying and I don't want to 
> weaken something that could have security implications without double- and 
> triple-checking that it's safe to do so.

You can probably call the `dropArguments` with `false` for `trusted` instead.

-

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

Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles

[liach]

On Fri, 27 May 2022 14:18:19 GMT, Claes Redestad  wrote:

> In preparation of #8855 this PR refactors the conversions from `List` to 
> array and array to `List`, reducing the number of conversions when calling 
> `MethodHandles.dropArguments` in particular. This remove about ~5% of 
> allocations on the `StringConcatFactoryBootstraps` microbenchmark.

If `parameterList` is too slow for `List.of` copies the backing parameter types 
array, wouldn't calling `JavaUtilCollectionAccess::listFromTrustedArray` a 
better alternative, as it only allocates one wrapper and has better performance 
on `subList` than `Arrays.copyOfRange`?

-

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

Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]

[liach]

On Thu, 26 May 2022 18:08:13 GMT, XenoAmess  wrote:

>> as title.
>
> XenoAmess has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' of https://git.openjdk.java.net/jdk into fix_8284780
>  - Merge remote-tracking branch 'openjdk/master' into fix_8284780
>
># Conflicts:
>#  test/jdk/java/util/HashMap/WhiteBoxResizeTest.java
>  - add 8284780 to test
>  - redo the tests
>  - rename items to elements
>  - add test for newHashSet and newLinkedHashSet
>  - revert much too changes for newHashSet
>  - add more replaces
>  - add more replaces
>  - add more replaces
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/7cb368b3...117918f4

src/java.base/share/classes/java/util/HashSet.java line 130:

> 128:  * @apiNote
> 129:  * To create a {@code HashSet} with an initial capacity that 
> accommodates
> 130:  * an expected number of items, use {@link #newHashSet(int) 
> newHashSet}.

Suggestion:

 * an expected number of elements, use {@link #newHashSet(int) newHashSet}.

src/java.base/share/classes/java/util/HashSet.java line 147:

> 145:  * @apiNote
> 146:  * To create a {@code HashSet} with an initial capacity that 
> accommodates
> 147:  * an expected number of items, use {@link #newHashSet(int) 
> newHashSet}.

Suggestion:

 * an expected number of elements, use {@link #newHashSet(int) newHashSet}.

src/java.base/share/classes/java/util/HashSet.java line 391:

> 389:  *
> 390:  * @param numElementsthe expected number of elements
> 391:  * @param  the type of keys maintained by this set

Suggestion:

 * @param  the type of elements maintained by this set

src/java.base/share/classes/java/util/LinkedHashSet.java line 131:

> 129:  * @apiNote
> 130:  * To create a {@code LinkedHashSet} with an initial capacity that 
> accommodates
> 131:  * an expected number of items, use {@link #newLinkedHashSet(int) 
> newLinkedHashSet}.

Suggestion:

 * an expected number of elements, use {@link #newLinkedHashSet(int) 
newLinkedHashSet}.

src/java.base/share/classes/java/util/LinkedHashSet.java line 148:

> 146:  * @apiNote
> 147:  * To create a {@code LinkedHashSet} with an initial capacity that 
> accommodates
> 148:  * an expected number of items, use {@link #newLinkedHashSet(int) 
> newLinkedHashSet}.

Suggestion:

 * an expected number of elements, use {@link #newLinkedHashSet(int) 
newLinkedHashSet}.

src/java.base/share/classes/java/util/LinkedHashSet.java line 212:

> 210:  *
> 211:  * @param numElementsthe expected number of elements
> 212:  * @param  the type of keys maintained by this set

Suggestion:

 * @param  the type of elements maintained by this set

-

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v4]

[liach]

On Fri, 27 May 2022 01:55:25 GMT, Mandy Chung  wrote:

>> liach 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 eight additional commits 
>> since the last revision:
>> 
>>  - Merge branch 'master' into proxy-class-forname
>>  - Move the try catch block as it doesn't throw checked exceptions
>>  - remove unused field
>>  - whitespace
>>  - Copyright year
>>  - typo
>>  - 8285401: Proxy class initializer should use 3-arg `Class.forName` to 
>> avoid unnecessary class initialization
>>  - Test for eager initialization
>
> test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 53:
> 
>> 51: new Class[]{ Intf.class },
>> 52: (proxy, method, args) -> null);
>> 53: Assert.assertFalse(initialized, "parameter type initialized 
>> eagerly");
> 
> This expects "parameter type initialized eagerly" to be false.   This may 
> cause confusion to the reader.  Maybe just simply "initialized expected: 
> false" and a comment would help too.  Similarly for line 56.

I renamed the message to "parameter type initialized unnecessarily", which 
should be more clear.

-

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v5]

[liach]

> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
> `Class.forName(String)`. `make test 
> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
> `LazyInitializationTest` failing the eager initialization check on the 
> baseline and passing with this patch.
> 
> On a side note, this might reduce the number of methods that can be encoded 
> in a proxy due to code attribute size restrictions; we probably would address 
> that in another issue, as we never mandated a count of methods that the proxy 
> must be able to implement.
> 
> Mandy, would you mind review this?

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

  Improve test messages

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8800/files
  - new: https://git.openjdk.java.net/jdk/pull/8800/files/1262e8fa..1090df4a

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

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

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v5]

[liach]

> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
> hash map with a simple lookup, similar to what's done in 
> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

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

  Doesn't hurt to be more specific

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8801/files
  - new: https://git.openjdk.java.net/jdk/pull/8801/files/6d171268..6aa89eff

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

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

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]

[liach]

On Thu, 26 May 2022 22:55:02 GMT, Mandy Chung  wrote:

>> liach 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 three additional commits 
>> since the last revision:
>> 
>>  - Merge branch 'master' into proxy-primitivetypeinfo
>>  - Convert PrimitiveTypeInfo to an enum
>>  - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo
>
> Have you considered including the opcode for load and return instruction 
> instead of `opcodeOffset`?

Hi @mlchung Mandy, I've changed the offset to seperate load and return opcodes, 
which is more in-line with how other fields in `PrimitiveTypeInfo` are used. In 
addition, I specified that the `wrapperClassName` is an internal name for 
clarification. Would you mind review this again, and review #8800, which avoids 
eager class initialization for parameter types used by proxy?

-

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]

[liach]

On Thu, 26 May 2022 22:51:39 GMT, Mandy Chung  wrote:

>> liach 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 three additional commits 
>> since the last revision:
>> 
>>  - Merge branch 'master' into proxy-primitivetypeinfo
>>  - Convert PrimitiveTypeInfo to an enum
>>  - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969:
> 
>> 967: // single-char BaseType descriptor (see JVMS section 4.3.2)
>> 968: String baseTypeString = wrapper.basicTypeString();
>> 969: wrapperClassName = 
>> dotToSlash(wrapper.wrapperType().getName());
> 
> Suggestion:
> 
> wrapperClassName = wrapper.wrapperType().descriptorString();
> 
> 
> It may worth to replace similar use of `dotToSlash(c.getName())` pattern.

Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor 
(`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor 
construction.

-

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v4]

[liach]

> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
> hash map with a simple lookup, similar to what's done in 
> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

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

  Make primitive type info more reader friendly

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8801/files
  - new: https://git.openjdk.java.net/jdk/pull/8801/files/96c0835e..6d171268

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

  Stats: 26 lines in 1 file changed: 7 ins; 1 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8801.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]

[liach]

On Thu, 26 May 2022 22:33:29 GMT, Mandy Chung  wrote:

>> The original check and `Modules.addOpen` calls were added in 
>> [8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the 
>> `invokeDefault` support was added.
>> 
>> See: 
>> https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
>>  or #313
>
> Ah, I see. That assert was added in PR #313 as a clarification to the current 
> proxy implementation but not required by the 
> `InvocationHandle::invokeDefault`.   It could have been added before the 
> default method support.

It appears to me that it's required by `proxyClassLookup` to perform reflective 
access on the dynamic module or the module where the package-private interface 
is located.

-

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v6]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

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

  Fixes suggested by mandy

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/20edc525..9d7ebaab

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

  Stats: 13 lines in 1 file changed: 4 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v19]

[liach]

On Thu, 31 Mar 2022 18:48:39 GMT, Jim Laskey  wrote:

>> We propose to provide a runtime anonymous carrier class object generator; 
>> java.lang.runtime.Carrier. This generator class is designed to share 
>> anonymous classes when shapes are similar. For example, if several clients 
>> require objects containing two integer fields, then Carrier will ensure that 
>> each client generates carrier objects using the same underlying anonymous 
>> class. 
>> 
>> See JBS for details.
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Change Carriers API

Will this be potentially revived if there's any progress in templated strings?

-

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]

[liach]

On Thu, 26 May 2022 21:32:53 GMT, Mandy Chung  wrote:

>> liach 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:
>> 
>>  - Group proxy location validation all into the class context constructor
>>  - Merge branch 'master' into fix/proxy-single-pass
>>  - Update
>>  - Updates suggested by mandy
>>  - Merge branch 'master' into fix/proxy-single-pass
>>  - Don't need to complexify module cache
>>  - 8284942: Proxy building can just iterate superinterfaces once
>
> src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
> 
>> 511: 
>> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
>> 513: // Required for default method invocation
> 
> Is this comment true? 
> 
> The module of the proxy class opens the package to `java.base` if the proxy 
> interface is non-public in a named module or if all proxy interfaces are 
> public but a proxy interface is not unconditionally exported.

The original check and `Modules.addOpen` calls were added in 
[8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the 
`invokeDefault` support was added.

See: 
https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
 or #313

-

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]

[liach]

> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
> hash map with a simple lookup, similar to what's done in 
> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

liach 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 three additional commits since the last 
revision:

 - Merge branch 'master' into proxy-primitivetypeinfo
 - Convert PrimitiveTypeInfo to an enum
 - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8801/files
  - new: https://git.openjdk.java.net/jdk/pull/8801/files/be9987bb..96c0835e

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

  Stats: 37306 lines in 626 files changed: 8637 ins; 27109 del; 1560 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8801.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

liach 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:

 - Group proxy location validation all into the class context constructor
 - Merge branch 'master' into fix/proxy-single-pass
 - Update
 - Updates suggested by mandy
 - Merge branch 'master' into fix/proxy-single-pass
 - Don't need to complexify module cache
 - 8284942: Proxy building can just iterate superinterfaces once

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/26bde80b..20edc525

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

  Stats: 37340 lines in 627 files changed: 8639 ins; 27112 del; 1589 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v4]

[liach]

> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
> `Class.forName(String)`. `make test 
> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
> `LazyInitializationTest` failing the eager initialization check on the 
> baseline and passing with this patch.
> 
> On a side note, this might reduce the number of methods that can be encoded 
> in a proxy due to code attribute size restrictions; we probably would address 
> that in another issue, as we never mandated a count of methods that the proxy 
> must be able to implement.
> 
> Mandy, would you mind review this?

liach 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 eight additional commits since the last 
revision:

 - Merge branch 'master' into proxy-class-forname
 - Move the try catch block as it doesn't throw checked exceptions
 - remove unused field
 - whitespace
 - Copyright year
 - typo
 - 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid 
unnecessary class initialization
 - Test for eager initialization

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8800/files
  - new: https://git.openjdk.java.net/jdk/pull/8800/files/82f8eeb9..1262e8fa

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

  Stats: 37306 lines in 626 files changed: 8637 ins; 27109 del; 1560 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8800.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8800/head:pull/8800

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]

[liach]

On Fri, 20 May 2022 22:18:42 GMT, liach  wrote:

>> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace 
>> the hash map with a simple lookup, similar to what's done in 
>> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)
>
> liach has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Convert PrimitiveTypeInfo to an enum

I would prefer to look up in a `Map.ofEntries` than to switch on string class 
names, which is both faster and clearer. The even faster heuristic of using 
perfect hash table has already been proven slower than the list of if 
statements in JDK-8284880, referred to in the pull request description.

-

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

Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]

[liach]

On Tue, 24 May 2022 21:37:52 GMT, XenoAmess  wrote:

>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add test for newHashSet and newLinkedHashSet

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 354:

> 352: rsc("rsls", size, cap, () -> {
> 353: try {
> 354: Field mapField = 
> HashSet.class.getDeclaredField("map");

For clarity, consider using var handle/method handle and keep the reflection 
code in the `WhiteBoxResizeTest` constructor.

-

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]

[liach]

On Tue, 24 May 2022 17:26:52 GMT, Roger Riggs  wrote:

> Did you consider switch (class) {...} in PrimitiveTypeInfo.get?

I don't think we can switch on class instances yet. Even with preview enabled, 
best I can get is (for type incompatibility of `Class` and `Class`, 
etc.)

return switch (cl) {
case Class e && e == int.class -> 1;
case Class e && e == long.class -> 2;
case Class e && e == boolean.class -> 3;
case Class e && e == short.class -> 4;
case Class e && e == byte.class -> 5;
case Class e && e == char.class -> 6;
case Class e && e == float.class -> 7;
case Class e && e == double.class -> 8;
default -> 0;
};

to avoid type incompatibility; this is in turn implemented by a bootstrap 
method and a loop, which is of course obviously much slower.

-

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v3]

[liach]

On Tue, 24 May 2022 05:36:30 GMT, Jaikiran Pai  wrote:

>> liach has updated the pull request incrementally with one additional commit 
>> since the last revision:
>> 
>>   Move the try catch block as it doesn't throw checked exceptions
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 605:
> 
>> 603: mv.visitLdcInsn(Type.getObjectType(dotToSlash(className)));
>> 604: mv.visitMethodInsn(INVOKEVIRTUAL, JL_CLASS,
>> 605: "getClassLoader", "()" + LJL_CLASSLOADER, false);
> 
> Hello @liach, should this instead be using the (application supplied) 
> `loader` returned by the call to `ProxyGenerator.getClassLoader()` or maybe 
> the `loader` member in the `ProxyGenerator` itself?

This is equivalent to `jdk.proxy5.$Proxy5.class.getClassLoader()` in Java 
source code, so this is exactly the application-supplied loader, which also 
uses the same loader as the previous behavior of `forName` calls.

If you want to pass the loader from `ProxyGenerator` to the proxy, it requires 
complex tricks. Hidden classes won't work due to serialization incompatibility; 
accessor methods would be defined in `jdk.internal` and exported specifically 
to the proxy modules, but writing a class so each proxy gets its loader while 
what I wrote can already do is overkill.

> test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 56:
> 
>> 54: 
>> 55: value.m(new Parameter());
>> 56: Assert.assertTrue(initialized, "parameter type initialized after 
>> instantiation");
> 
>> "parameter type initialized after instantiation"
> 
> Since this is the text that gets displayed/reported when the assertion fails, 
> should this instead be "parameter type not initialized"?

The rendered text for testng assert is `message expected: value actual: value`, 
so on a mismatch, it would print `parameter type initialized after 
instantiation expected: true actual: false`

-

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

Re: RFR: 8286849: Use @Stable for generic repositories

[liach]

On Tue, 17 May 2022 04:40:50 GMT, liach  wrote:

> Generic repositories, the implementation detail for generic information in 
> core reflection, can be updated to use the `@Stable` annotation to replace 
> their `volatile` access. Their existing accessor code is already safe, 
> reading the cache fields only once.
> 
> In addition, fixed potentially non-thread-safe `genericInfo` access in 
> `Method`, `Field`, and `RecordComponent`.

This should offer slight performance benefit if a reflection object has its 
generic information accessed frequently, as reading a stable field is faster 
than reading a volatile field for cached objects. If the `genericInfo` fields 
are updated by JVMTI then they need to be declared `volatile` instead. In 
addition, some of the `genericInfo` fields may have been not thread-safe for 
their multiple field reads.

-

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

Re: RFR: 8284640: CollectorImpl class could be a record class

[liach]

On Mon, 11 Apr 2022 13:08:43 GMT, altrisi  wrote:

> Changes the definition of `CollectorImpl` to be a record.

@stuart-marks Mind review this simple cleanup?

-

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]

[liach]

On Fri, 20 May 2022 20:02:28 GMT, Roger Riggs  wrote:

>> liach has updated the pull request incrementally with one additional commit 
>> since the last revision:
>> 
>>   Convert PrimitiveTypeInfo to an enum
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 932:
> 
>> 930:  */
>> 931: private static final class PrimitiveTypeInfo {
>> 932: private static final PrimitiveTypeInfo BYTE = new 
>> PrimitiveTypeInfo(byte.class, 0);
> 
> Can this be `private enum PrimitiveTypeInfo...` or perhaps a record class?

Thanks. I've converted it into an enum following the suit of 
`sun.invoke.util.Wrapper`. I think an enum fits better here as we don't need 
the canonical constructor and object methods of a record.

-

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

Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]

[liach]

> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
> hash map with a simple lookup, similar to what's done in 
> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

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

  Convert PrimitiveTypeInfo to an enum

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8801/files
  - new: https://git.openjdk.java.net/jdk/pull/8801/files/16f3de8f..be9987bb

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

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

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v2]

[liach]

On Fri, 20 May 2022 11:51:09 GMT, ExE Boss  wrote:

>> liach has updated the pull request incrementally with one additional commit 
>> since the last revision:
>> 
>>   remove unused field
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 608:
> 
>> 606: mv.visitMethodInsn(INVOKEVIRTUAL, JL_CLASS,
>> 607: "getClassLoader", "()" + LJL_CLASSLOADER, false);
>> 608: mv.visitVarInsn(ASTORE, 0);
> 
> Shouldn’t this go before `mv.visitLabel(L_startBlock)`?

Done.

-

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v3]

[liach]

> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
> `Class.forName(String)`. `make test 
> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
> `LazyInitializationTest` failing the eager initialization check on the 
> baseline and passing with this patch.
> 
> On a side note, this might reduce the number of methods that can be encoded 
> in a proxy due to code attribute size restrictions; we probably would address 
> that in another issue, as we never mandated a count of methods that the proxy 
> must be able to implement.
> 
> Mandy, would you mind review this?

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

  Move the try catch block as it doesn't throw checked exceptions

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8800/files
  - new: https://git.openjdk.java.net/jdk/pull/8800/files/64a70479..82f8eeb9

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

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

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

RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

[liach]

Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the 
hash map with a simple lookup, similar to what's done in 
[JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)

-

Commit messages:
 - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo

Changes: https://git.openjdk.java.net/jdk/pull/8801/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8801=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287064
  Stats: 79 lines in 1 file changed: 14 ins; 41 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8801.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v2]

[liach]

> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
> `Class.forName(String)`. `make test 
> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
> `LazyInitializationTest` failing the eager initialization check on the 
> baseline and passing with this patch.
> 
> On a side note, this might reduce the number of methods that can be encoded 
> in a proxy due to code attribute size restrictions; we probably would address 
> that in another issue, as we never mandated a count of methods that the proxy 
> must be able to implement.
> 
> Mandy, would you mind review this?

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

  remove unused field

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8800/files
  - new: https://git.openjdk.java.net/jdk/pull/8800/files/c1b522d5..64a70479

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

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

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

RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization

[liach]

Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of 
`Class.forName(String)`. `make test 
TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new 
`LazyInitializationTest` failing the eager initialization check on the baseline 
and passing with this patch.

On a side note, this might reduce the number of methods that can be encoded in 
a proxy due to code attribute size restrictions; we probably would address that 
in another issue, as we never mandated a count of methods that the proxy must 
be able to implement.

Mandy, would you mind review this?

-

Commit messages:
 - whitespace
 - Copyright year
 - typo
 - 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid 
unnecessary class initialization
 - Test for eager initialization

Changes: https://git.openjdk.java.net/jdk/pull/8800/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8800=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285401
  Stats: 78 lines in 2 files changed: 74 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8800.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8800/head:pull/8800

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v4]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

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

  Update

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/502dad82..26bde80b

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

  Stats: 8 lines in 1 file changed: 5 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]

[liach]

On Fri, 20 May 2022 00:08:36 GMT, Mandy Chung  wrote:

> a named module can't have unnamed package

Since this is mandated by the Java Language Specification 7.4.2, I am tempted 
to change the thrown exception to `InternalError`, but I cannot find any 
restriction in the JVM Specification that prevents declaring a package-private 
superinterface in the unnamed package of a named module. If we can find a 
reference in the JVM Specification, I'm more than glad to refer to that in a 
comment and throw `InternalError` instead.

-

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

liach 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 four additional commits since the last 
revision:

 - Updates suggested by mandy
 - Merge branch 'master' into fix/proxy-single-pass
 - Don't need to complexify module cache
 - 8284942: Proxy building can just iterate superinterfaces once

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/d58bb7e0..502dad82

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

  Stats: 259862 lines in 4149 files changed: 190963 ins; 46360 del; 22539 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v2]

[liach]

On Thu, 21 Apr 2022 03:44:18 GMT, liach  wrote:

>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
>> the interfaces are iterated twice. The two passes can be merged into one, 
>> yielding the whole proxy definition context (module, package, whether 
>> there's package-private interface) when determining the module.
>> 
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a 
>> good cleanup on its own.
>
> liach has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Don't need to complexify module cache

Can anyone, such as Mandy, review this cleanup?

-

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

RFR: 8286849: Use @Stable for generic repositories

[liach]

Generic repositories, the implementation detail for generic information in core 
reflection, can be updated to use the `@Stable` annotation to replace their 
`volatile` access. Their existing accessor code is already safe, reading the 
cache fields only once.

In addition, fixed potentially non-thread-safe `genericInfo` access in 
`Method`, `Field`, and `RecordComponent`.

-

Commit messages:
 - 8286849: Use @Stable for generic repositories

Changes: https://git.openjdk.java.net/jdk/pull/8742/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8742=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286849
  Stats: 45 lines in 11 files changed: 29 ins; 0 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8742.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8742/head:pull/8742

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline

[liach]

On Thu, 12 May 2022 17:17:37 GMT, Jorn Vernee  wrote:

> Hi,
> 
> This PR brings over commits from the panama-foreign repo. These commits 
> mostly pertain to the switch to ASM and away from MethodHandle combinators 
> for the binding recipe specialization. But, there is one more commit which 
> adds freeing of downcall stubs, since those changes were mostly Java as well.
> 
> Thanks,
> Jorn

test/micro/org/openjdk/bench/java/lang/foreign/LinkUpcall.java line 61:

> 59: BLANK = lookup().findStatic(LinkUpcall.class, "blank", 
> MethodType.methodType(void.class));
> 60: } catch (ReflectiveOperationException e) {
> 61: throw new BootstrapMethodError(e);

You probably mean exception in initializer error.

-

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

Re: RFR: JDK-8286347: incorrect use of `{@link}`

[liach]

On Fri, 6 May 2022 23:30:44 GMT, Jonathan Gibbons  wrote:

> Please review a small doc fix to update incorrect use of `{@link}` on arrays 
> of primitive types.

This is fixed in 8282191 (JEP 424), which is already integrated.

-

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

Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec

[liach]

On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy  wrote:

> While doing a CSR review of another issue, I noticed some cases in 
> InputStream and OutputStream what would benefit from being upgraded to 
> implSpec and related javadoc tags.
> 
> The "A subclass must provide an implementation of this method." statements on 
> several abstract methods don't add much value, but I chose to leave them in 
> for this request.
> 
> Please also review the corresponding CSR: 
> https://bugs.openjdk.java.net/browse/JDK-8286605

src/java.base/share/classes/java/io/OutputStream.java line 151:

> 149:  * The {@code write} method of {@code OutputStream} calls
> 150:  * the write method of one argument on each of the bytes to be
> 151:  * written out. Subclasses are encouraged to override this method and

Shouldn't the "subclasses" information be part of the API Note?

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

On Thu, 21 Apr 2022 00:48:00 GMT, Stuart Marks  wrote:

>> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
>> values by identity. Updated API documentation of these two methods 
>> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>>  to mention such behavior.
>
> Thanks for working on this. Yes I can review and sponsor this change.
> 
> Sorry I got a bit distracted. I started looking at the test here, and this 
> lead me to inquire about what other tests we have for `IdentityHashMap`, and 
> the answer is: not enough. See 
> [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295). (But that 
> should be handled separately.)

@stuart-marks I have updated the tests to be based off that from 
[JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295). Anything else 
that needs an update?

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v4]

[liach]

> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
> values by identity. Updated API documentation of these two methods 
> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>  to mention such behavior.

liach 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 six additional commits since the last 
revision:

 - Move tests
 - Merge branch 'master' into fix/identityhashmap-default
 - Fix assertions
 - Revamp test and changes. Let ci run the tests
 - Fix indent
 - 8178355: IdentityHashMap uses identity-based comparison for values 
everywhere except remove(K,V) and replace(K,V,V)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8259/files
  - new: https://git.openjdk.java.net/jdk/pull/8259/files/fe91721d..c8468ce2

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

  Stats: 53894 lines in 1878 files changed: 35482 ins; 8470 del; 9942 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8259/head:pull/8259

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

Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]

[liach]

On Wed, 4 May 2022 14:57:19 GMT, Jaikiran Pai  wrote:

>> Stuart Marks has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Assertions over return values. Some refinement of equals() testing.
>>  - Add comment about Map.Entry identity not guaranteed.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
> 
>> 498: Box newKey = new Box(k1a);
>> 499: Box newVal = new Box(v1a);
>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true; 
>> return newVal; });
> 
> More of a curiosity than a review comment - I see that various places in this 
> PR use a boolean array with one element instead of just a boolean type. Is 
> that a personal coding preference or is there something more to it?

This just serves as a modifiable boolean like an AtomicBoolean. Remember 
lambdas can only use final local var references (due to how they work), and it 
cannot access or modify the local variable in the caller method.

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v3]

[liach]

> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
> values by identity. Updated API documentation of these two methods 
> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>  to mention such behavior.

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

  Fix assertions

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8259/files
  - new: https://git.openjdk.java.net/jdk/pull/8259/files/dd416079..fe91721d

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

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

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

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

On Sun, 17 Apr 2022 16:17:30 GMT, liach  wrote:

> Convert dynamic proxies to hidden classes. Modifies the serialization of 
> proxies (requires change in "Java Object Serialization Specification"). Makes 
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
> 
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of 
> the class is allocated... The contents restored appropriately" section, I 
> propose explicitly state that handling of proxies are unspecified as to allow 
> implementation freedom, though I've seen deliberate attempts for proxies to 
> implement interfaces with `readResolve` in order to control their 
> serialization behavior.
>- This is for the existing generated constructor accessor is 
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization 
> constructor calls `invokespecial` on the closest serializable superclass' 
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and 
> should be discouraged in the long term. Calling the existing constructor with 
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in 
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method, 
> implemented by the lambda, will not appear in the stack trace, and isn't too 
> problematic.
> 
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into 
> one. This reduced a lot of code and allowed anchor class (for hidden class 
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of 
> `java.base`. This API is intended for implementation of legacy (pre 
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more 
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959): 
> passes methods computed by proxy generator as class data to the hidden proxy 
> class to reduce generated proxy class size and improve performance.
> 
> In addition, since this change is somewhat large, should we keep the old 
> proxy generator as well and have it toggled through a command-line flag (like 
> the old v49 proxy generator or the old reflection implementation)?
> 
> Please feel free to comment or review. This change definitely requires a CSR, 
> but I have yet to determine what specifications should be changed.

So, after reading the updated valhalla documentation, namely after the expert 
group decides to represent identity vs value with flags without touching 
inheritance (so saves serialization breakage when there's no spurious 
`IdentityObject`) and that `LambdaMetafactory` will reject identity or value 
interfaces per [Value Objects JEP](https://openjdk.java.net/jeps/8277163), I 
wonder about the future of dynamic proxies as well. I expect proxies to reject 
identity or value interfaces like `LambdaMetafactory`, and we most likely need 
a new API, like Remi's, if we wish to support identity/value interfaces.

Also for deserialization, since we have `Unsafe.allocateInstance`, we might 
alternatively replace the `anew` bytecode with such a call for the native 
serialization constructor if we do serialize and deserialize any hidden class 
like proxies, without touching the security part.

-

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

Re: RFR: 8284640: CollectorImpl class could be a record class

[liach]

On Mon, 11 Apr 2022 13:08:43 GMT, altrisi  wrote:

> Changes the definition of `CollectorImpl` to be a record.

src/java.base/share/classes/java/util/stream/Collectors.java line 197:

> 195:  * @param  the type of the result
> 196:  */
> 197: static record CollectorImpl(Supplier supplier,

Suggestion:

record CollectorImpl(Supplier supplier,

[Nested records are implicitly 
static.](https://docs.oracle.com/javase/specs/jls/se18/html/jls-8.html#jls-8.10-220)

-

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

Re: RFR: 8285295: Need better testing for IdentityHashMap

[liach]

On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks  wrote:

> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch 
> in the bug report that breaks `IdentityHashMap` now causes several cases in 
> this new test to fail. There's more that could be done, but the new tests 
> cover most of the core functions of `IdentityHashMap`. Unfortunately it seems 
> difficult to merge this with the existing, comprehensive Collections tests 
> (e.g., MOAT.java) because those tests implicity rely on `equals()`-based 
> contract instead of the special-purpose `==`-based contract used by 
> `IdentityHashMap`.

Just curious, will entrySet/keySet/values tests be here or in a separate file? 
They are identity-based collections as well, as shown by their 
contains/removeAll/retainAll.

test/jdk/java/util/IdentityHashMap/Basic.java line 75:

> 73: private  void checkContents(Collection c, BiPredicate p, 
> E... given) {
> 74: @SuppressWarnings("unchecked")
> 75: E[] contents = (E[]) c.toArray();

Maybe worthy to note that entry set toArray may return entries different from 
what the iterator returns, and some predicates may thus fail.

test/jdk/java/util/IdentityHashMap/Basic.java line 77:

> 75: E[] contents = (E[]) c.toArray();
> 76: 
> 77: assertEquals(c.size(), given.length);

I believe testng has the expected values in front in the `assertEquals` 
methods, as embodied in the exception messages, so this should be 
`assertEquals(given.length, c.size());`. Applies to other places.

-

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

Re: RFR: 8285658: Fix two typos in the spec of j.u.random.RandomGenerator [v2]

[liach]

On Tue, 26 Apr 2022 16:55:44 GMT, Raffaello Giulietti  
wrote:

>> The spec of the interface `java.util.random.RandomGenerator` is slightly 
>> incorrect when it discusses `float` and `double` random values.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8285658: Fix two typos in the spec of j.u.random.RandomGenerator

Should we use `{@value }` javadoc tag, which renders the literal in HTML but 
includes a link to the constant?

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

On Thu, 21 Apr 2022 00:48:00 GMT, Stuart Marks  wrote:

>> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
>> values by identity. Updated API documentation of these two methods 
>> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>>  to mention such behavior.
>
> Thanks for working on this. Yes I can review and sponsor this change.
> 
> Sorry I got a bit distracted. I started looking at the test here, and this 
> lead me to inquire about what other tests we have for `IdentityHashMap`, and 
> the answer is: not enough. See 
> [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295). (But that 
> should be handled separately.)

@stuart-marks Updated. In addition, for API docs change, should we add apiNote 
on object equality code, like call computeIfPresent?

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v2]

[liach]

> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
> values by identity. Updated API documentation of these two methods 
> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>  to mention such behavior.

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

  Revamp test and changes. Let ci run the tests

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8259/files
  - new: https://git.openjdk.java.net/jdk/pull/8259/files/9bb9ef57..dd416079

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

  Stats: 281 lines in 3 files changed: 191 ins; 88 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8259/head:pull/8259

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

Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v2]

[liach]

> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, 
> the interfaces are iterated twice. The two passes can be merged into one, 
> yielding the whole proxy definition context (module, package, whether there's 
> package-private interface) when determining the module.
> 
> Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
> cleanup on its own.

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

  Don't need to complexify module cache

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8281/files
  - new: https://git.openjdk.java.net/jdk/pull/8281/files/08796879..d58bb7e0

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

  Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

On Thu, 21 Apr 2022 01:01:25 GMT, Stuart Marks  wrote:

>> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
>> values by identity. Updated API documentation of these two methods 
>> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>>  to mention such behavior.
>
> src/java.base/share/classes/java/util/IdentityHashMap.java line 1412:
> 
>> 1410: i = nextKeyIndex(i, len);
>> 1411: }
>> 1412: }
> 
> Unfortunately there's some mostly-duplicate code here. However, there's 
> similar logic and code sprinkled throughout this class, so _more_ duplication 
> isn't necessarily the wrong thing to do. However, trying to unify this logic 
> results in much more intrusive refactoring, which is harder to review, and 
> which isn't backed up with tests (see JDK-8285295) which I wouldn't encourage 
> pursuing right now. In other words, I'm ok with this duplicate logic.

On intrusive logic: I planned address it in 
https://bugs.openjdk.java.net/browse/JDK-8277520 (#6532), and if this one is 
integrated first, it may be possible to fix it up there.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v9]

[liach]

On Wed, 20 Apr 2022 16:30:19 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove uselsee MIN_SKIP_BUFFER_SIZE

src/java.base/share/classes/java/io/InputStream.java line 62:

> 60: 
> 61: private byte[] skipBufferReference(long remaining) {
> 62: int size = (int) Math.min(MAX_SKIP_BUFFER_SIZE, remaining);

This is now redundant. Can change parameter to (int size) directly

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v8]

[liach]

On Wed, 20 Apr 2022 16:07:17 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove useless skipBuffer

src/java.base/share/classes/java/io/InputStream.java line 57:

> 55: private static final int MAX_SKIP_BUFFER_SIZE = 2048;
> 56: 
> 57: private static final int MIN_SKIP_BUFFER_SIZE = 128;

@jmehrens mentioned that this one can be removed as well since it's not used.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v7]

[liach]

On Fri, 15 Apr 2022 18:56:37 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   change to liach operation.

src/java.base/share/classes/java/io/InputStream.java line 62:

> 60: 
> 61: /** Skip buffer, null until allocated */
> 62: private byte[] skipBuffer = null;

This can be removed too as you have the soft reference already.

-

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

On Fri, 15 Apr 2022 05:58:32 GMT, liach  wrote:

> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
> values by identity. Updated API documentation of these two methods 
> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>  to mention such behavior.

@stuart-marks Mind peek at this given you are the assignee of the original JBS 
issue?

-

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

Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v4]

[liach]

On Tue, 19 Apr 2022 20:15:08 GMT, XenoAmess  wrote:

>> as title.
>
> XenoAmess 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 two new 
> commits since the last revision:
> 
>  - migrate HashSet usages
>  - migrate LinkedHashSet usage

Need to add apiNote documentation section to capacity-based constructors like 
for maps.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v7]

[liach]

On Tue, 19 Apr 2022 18:04:03 GMT, XenoAmess  wrote:

> Reader uses a lock object and it appears that InputStream locks on this (per 
> make/reset) I would assume now that you have some object member fields you 
> need to hold some lock while accessing those. Even though single thread 
> access would be the expected case.

Locks are expensive. We just use buffers that may or may not be shared by 
different threads (for the java memory model), because locking objects is too 
costly performance-wise.

-

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

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

On Sun, 17 Apr 2022 16:17:30 GMT, liach  wrote:

> Convert dynamic proxies to hidden classes. Modifies the serialization of 
> proxies (requires change in "Java Object Serialization Specification"). Makes 
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
> 
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of 
> the class is allocated... The contents restored appropriately" section, I 
> propose explicitly state that handling of proxies are unspecified as to allow 
> implementation freedom, though I've seen deliberate attempts for proxies to 
> implement interfaces with `readResolve` in order to control their 
> serialization behavior.
>- This is for the existing generated constructor accessor is 
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization 
> constructor calls `invokespecial` on the closest serializable superclass' 
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and 
> should be discouraged in the long term. Calling the existing constructor with 
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in 
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method, 
> implemented by the lambda, will not appear in the stack trace, and isn't too 
> problematic.
> 
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into 
> one. This reduced a lot of code and allowed anchor class (for hidden class 
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of 
> `java.base`. This API is intended for implementation of legacy (pre 
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more 
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959): 
> passes methods computed by proxy generator as class data to the hidden proxy 
> class to reduce generated proxy class size and improve performance.
> 
> In addition, since this change is somewhat large, should we keep the old 
> proxy generator as well and have it toggled through a command-line flag (like 
> the old v49 proxy generator or the old reflection implementation)?
> 
> Please feel free to comment or review. This change definitely requires a CSR, 
> but I have yet to determine what specifications should be changed.

Fixing deserialization is not particularly hard. A longer term goal is to 
declare a readResolve for proxies (presumably by extra methods on invocation 
handlers) to convert serialization result to something else

-

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

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

On Mon, 18 Apr 2022 20:42:48 GMT, Remi Forax  wrote:

> The third parameter of defineProxy() is a lambda which is called for each 
> method that can be overriden, either toString/equals/hashCode but also any 
> default methods,
if the lambda return true, the method is overriden, otherwise the default 
implementation is used.

Not quite, I mean calling default implementations in the super interface, 
consider:

interface Root { void cleanUp(); }
interface FeatureOne extends Root { default void cleanUp() { /* do something */ 
} }
interface FeatureTwo extends Root { default void cleanUp() { /* do something */ 
} }

My proxy implements both feature one and two, but in your API, there is no way 
for my `cleanUp` to call both `FeatureOne.super.cleanUp();` and 
`FeatureTwo.super.cleanUp();`. You should probably expose the lookup in your 
linker too, especially that you enabled nest access and you can just use that 
lookup to resolve, say, an implementation static method in the proxy user class.

Also the "delegate" in your API would significantly benefit from 
https://bugs.openjdk.java.net/browse/JDK-8282798 (#7744), too.

-

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

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

On Sun, 17 Apr 2022 16:17:30 GMT, liach  wrote:

> Convert dynamic proxies to hidden classes. Modifies the serialization of 
> proxies (requires change in "Java Object Serialization Specification"). Makes 
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
> 
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of 
> the class is allocated... The contents restored appropriately" section, I 
> propose explicitly state that handling of proxies are unspecified as to allow 
> implementation freedom, though I've seen deliberate attempts for proxies to 
> implement interfaces with `readResolve` in order to control their 
> serialization behavior.
>- This is for the existing generated constructor accessor is 
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization 
> constructor calls `invokespecial` on the closest serializable superclass' 
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and 
> should be discouraged in the long term. Calling the existing constructor with 
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in 
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method, 
> implemented by the lambda, will not appear in the stack trace, and isn't too 
> problematic.
> 
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into 
> one. This reduced a lot of code and allowed anchor class (for hidden class 
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of 
> `java.base`. This API is intended for implementation of legacy (pre 
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more 
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959): 
> passes methods computed by proxy generator as class data to the hidden proxy 
> class to reduce generated proxy class size and improve performance.
> 
> In addition, since this change is somewhat large, should we keep the old 
> proxy generator as well and have it toggled through a command-line flag (like 
> the old v49 proxy generator or the old reflection implementation)?
> 
> Please feel free to comment or review. This change definitely requires a CSR, 
> but I have yet to determine what specifications should be changed.

I strongly recommend a new API over revamping `Proxy` itself. 
https://github.com/forax/hidden_proxy would be a good prototype that serves 
most needs of the current Proxy API (except a few, like invoking default 
superinterface method). With hidden classes, I don't see much value in leaving 
the new API's produced instance in separate modules; what we have for hidden 
classes should be fine for the most part.

Imo the main obstacle is still the handling of serialization. The annotations 
are serializable, but currently hidden classes do not work with serialization 
at all and must use `writeReplace` and `readResolve`. And how to migrate 
annotations off proxies without breaking serialization is still a question as 
well. Maybe we can upgrade invocation handlers to allow them to declare custom 
`readResolve` logic for the proxy to facilitate migration away. How the new API 
will treat serialization is still undetermined.

-

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

RFR: 8284942: Proxy building can just iterate superinterfaces once

[liach]

Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, the 
interfaces are iterated twice. The two passes can be merged into one, yielding 
the whole proxy definition context (module, package, whether there's 
package-private interface) when determining the module.

Split from #8278. Helpful for moving proxies to hidden classes, but is a good 
cleanup on its own.

-

Commit messages:
 - 8284942: Proxy building can just iterate superinterfaces once

Changes: https://git.openjdk.java.net/jdk/pull/8281/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8281=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8284942
  Stats: 64 lines in 1 file changed: 16 ins; 25 del; 23 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8281.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281

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

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

On Sun, 17 Apr 2022 16:17:30 GMT, liach  wrote:

> Convert dynamic proxies to hidden classes. Modifies the serialization of 
> proxies (requires change in "Java Object Serialization Specification"). Makes 
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
> 
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of 
> the class is allocated... The contents restored appropriately" section, I 
> propose explicitly state that handling of proxies are unspecified as to allow 
> implementation freedom, though I've seen deliberate attempts for proxies to 
> implement interfaces with `readResolve` in order to control their 
> serialization behavior.
>- This is for the existing generated constructor accessor is 
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization 
> constructor calls `invokespecial` on the closest serializable superclass' 
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and 
> should be discouraged in the long term. Calling the existing constructor with 
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in 
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method, 
> implemented by the lambda, will not appear in the stack trace, and isn't too 
> problematic.
> 
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into 
> one. This reduced a lot of code and allowed anchor class (for hidden class 
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of 
> `java.base`. This API is intended for implementation of legacy (pre 
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more 
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959): 
> passes methods computed by proxy generator as class data to the hidden proxy 
> class to reduce generated proxy class size and improve performance.
> 
> In addition, since this change is somewhat large, should we keep the old 
> proxy generator as well and have it toggled through a command-line flag (like 
> the old v49 proxy generator or the old reflection implementation)?
> 
> Please feel free to comment or review. This change definitely requires a CSR, 
> but I have yet to determine what specifications should be changed.

I now think that this should be split into probably four separate patches:
1. Merge the 2 passes of scanning interfaces, and store the 
exported/non-exported package within the classloader value info for dynamic 
modules
2. Use the JLIA access to obtain lookup than spin an accessor in every proxy 
class
3. Update serialization constructor generation to use method handles (as in 
#1830) for hidden classes, don't change the serialization specs for now, as it 
affects security.

And finally, convert proxies to hidden classes like in this pr. There will be 
an option (most likely command line) to restore old proxies. And in the new 
proxies, method objects can be passed as class data, like in the current patch. 
The jshell test change will be in the final conversion patch.

-

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

RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[liach]

Convert dynamic proxies to hidden classes. Modifies the serialization of 
proxies (requires change in "Java Object Serialization Specification"). Makes 
the proxies hidden in stack traces. Removes duplicate logic in proxy building.

The main compatibility changes and their rationales are:
1. Modification to the serialization specification: In the "An instance of the 
class is allocated... The contents restored appropriately" section, I propose 
explicitly state that handling of proxies are unspecified as to allow 
implementation freedom, though I've seen deliberate attempts for proxies to 
implement interfaces with `readResolve` in order to control their serialization 
behavior.
   - This is for the existing generated constructor accessor is bytecode-based, 
which cannot refer to hidden classes.
   - An alternative is to preserve the behavior, where the serialization 
constructor calls `invokespecial` on the closest serializable superclass' 
no-arg constructor, like in #1830 by @DasBrain.
   - My rationale against preservation is such super calls are unsafe and 
should be discouraged in the long term. Calling the existing constructor with a 
dummy argument, as in my implementation, would be more safe.
2. The disappearance of proxies in stack traces.
   - Same behavior exists in lambda expressions: For instance, in `((Runnable) 
() -> { throw new Error(); }).run();`, the `run` method, implemented by the 
lambda, will not appear in the stack trace, and isn't too problematic.

A summary of the rest of the changes:
1. Merged the two passes of determining module and package of the proxy into 
one. This reduced a lot of code and allowed anchor class (for hidden class 
creation) selection be done together as well.
2. Exposed internal API for obtaining a full-privileged lookup to the rest of 
`java.base`. This API is intended for implementation of legacy (pre 
`MethodHandles.Lookup`) caller sensitive public APIs so they don't need more 
complex tricks to obtain proper permissions as lookups.
3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959): 
passes methods computed by proxy generator as class data to the hidden proxy 
class to reduce generated proxy class size and improve performance.

In addition, since this change is somewhat large, should we keep the old proxy 
generator as well and have it toggled through a command-line flag (like the old 
v49 proxy generator or the old reflection implementation)?

Please feel free to comment or review. This change definitely requires a CSR, 
but I have yet to determine what specifications should be changed.

-

Commit messages:
 - Change proxy serialization to work with hidden classes (require spec update)
 - Formatting
 - Move to hidden class and methods in class data
 - Implement anchors and remove proxyClassLookup factory

Changes: https://git.openjdk.java.net/jdk/pull/8278/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8278=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8242888
  Stats: 400 lines in 6 files changed: 122 ins; 222 del; 56 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8278.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8278/head:pull/8278

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

Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

On Fri, 15 Apr 2022 13:53:59 GMT, ExE Boss  wrote:

>> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
>> values by identity. Updated API documentation of these two methods 
>> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>>  to mention such behavior.
>
> test/jdk/java/util/IdentityHashMap/DefaultRemoveReplace.java line 36:
> 
>> 34: final String key = "key";
>> 35: final String internedValue = "value";
>> 36: final String constructedValue = new String(new char[]{'v', 'a', 
>> 'l', 'u', 'e'});
> 
> Using:
> Suggestion:
> 
> final String constructedValue = new String(internedValue);
> 
> will allow the internal `String.value` array to be shared: 
> <https://github.com/openjdk/jdk/blob/bdf8a2a2050393e91800786f8d5a5d6805f936eb/src/java.base/share/classes/java/lang/String.java#L259-L265>

I know. This is just to get rid of intellij idea's warning.

-

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

RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)

[liach]

Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare 
values by identity. Updated API documentation of these two methods to mention 
such behavior.

-

Commit messages:
 - Fix indent
 - 8178355: IdentityHashMap uses identity-based comparison for values 
everywhere except remove(K,V) and replace(K,V,V)

Changes: https://git.openjdk.java.net/jdk/pull/8259/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8259=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8178355
  Stats: 165 lines in 2 files changed: 132 ins; 32 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8259/head:pull/8259

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v6]

[liach]

On Thu, 14 Apr 2022 18:52:25 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   refine jmh

Now this makes sense, that with a skip buffer of 2048, skipping by 2048 has a 
similar performance as skipping by 8192 for field-backed ones.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v4]

[liach]

On Wed, 13 Apr 2022 21:58:06 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add jmh

test/micro/org/openjdk/bench/java/io/InputStreamSkipBenchmark.java line 54:

> 52: @Benchmark
> 53: public long testSkip0(Data data) throws IOException {
> 54: TestBaseInputStream0 testBaseInputStream = new 
> TestBaseInputStream0(data.inputStreamSize);

Instead of creating 3 methods with identical bodies, I recommend using an enum 
to represent the type of buffers. An example at 
https://github.com/openjdk/jdk/blob/7a9844cb1cd18c18ce097741cba7db1148c83de0/test/micro/org/openjdk/bench/java/util/HashMapBench.java#L65-L71

For enum values, you can name them like `LOCAL_VARIABLE`, `FIELD`, 
`SOFT_REFERENCE`, which is more descriptive than current "0, 1, 2," etc.

test/micro/org/openjdk/bench/java/io/InputStreamSkipBenchmark.java line 127:

> 125: 
> 126: @Override
> 127: public int read(byte[] b, int off, int len) throws IOException {

I suggest we actually write into the byte array to better simulate overheads 
(maybe by generating a number with `ThreadLocalRandom`). Otherwise, the 
benchmark may have exaggerated the performance gains of large skips.

To simulate overhead on each read call, you can perform some extra activity 
consumed by the blackhole (possibly pass jmh blackhole through input stream 
constructor)

-

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]

[liach]

On Wed, 13 Apr 2022 21:12:40 GMT, ExE Boss  wrote:

>> Joe Darcy 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 26 additional commits 
>> since the last revision:
>> 
>>  - Respond to review feedback.
>>  - Merge branch 'master' into JDK-8266670
>>  - Make workding changes suggested in review feedback.
>>  - Merge branch 'master' into JDK-8266670
>>  - Typo fix; add implSpec to Executable.
>>  - Appease jcheck.
>>  - Fix some bugs found by inspection, docs cleanup.
>>  - Merge branch 'master' into JDK-8266670
>>  - Initial support for accessFlags methods
>>  - Add mask to access flag functionality.
>>  - ... and 16 more: 
>> https://git.openjdk.java.net/jdk/compare/afd02683...14980605
>
> src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 167:
> 
>> 165:  * but is optional in the dynamic phase, during execution.
>> 166:  */
>> 167: STATIC(AccessFlag.STATIC.mask()),
> 
> This is actually `AccessFlag.STATIC_PHASE` (`0x0040`), and not 
> `AccessFlag.STATIC` (`0x0008`):
> Suggestion:
> 
> STATIC(AccessFlag.STATIC_PHASE.mask()),

> In the current hodgepodge AccessFlag, we have STATIC and STATIC_PHASE, and 
> the incorrect ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC) call 
> is much more subtle, especially to new users of this class. Arguably, this 
> misuse would be way worse than that in the distinct enum case.

Oops, didn't know this already happened. Good spot right there.

-

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]

[liach]

On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy  wrote:

>> This is an early review of changes to better model JVM access flags, that is 
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>> 
>> Language level modifiers and JVM level access flags are closely related, but 
>> distinct. There are concepts that overlap in the two domains (public, 
>> private, etc.), others that only have a language-level modifier (sealed), 
>> and still others that only have an access flag (synthetic).
>> 
>> The existing java.lang.reflect.Modifier class is inadequate to model these 
>> subtleties. For example, the bit positions used by access flags on different 
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
>> methods. Just having a raw integer does not provide sufficient context to 
>> decode the corresponding language-level string. Methods like 
>> Modifier.methodModifiers() were introduced to cope with this situation.
>> 
>> With additional modifiers and flags on the horizon with projects like 
>> Valhalla, addressing the existent modeling deficiency now ahead of time is 
>> reasonable before further strain is introduced.
>> 
>> This PR in its current form is meant to give the overall shape of the API. 
>> It is missing implementations to map from, say, method modifiers to access 
>> flags, taking into account overlaps in bit positions.
>> 
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
>> once the API is further along.
>
> Joe Darcy 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 26 additional commits since 
> the last revision:
> 
>  - Respond to review feedback.
>  - Merge branch 'master' into JDK-8266670
>  - Make workding changes suggested in review feedback.
>  - Merge branch 'master' into JDK-8266670
>  - Typo fix; add implSpec to Executable.
>  - Appease jcheck.
>  - Fix some bugs found by inspection, docs cleanup.
>  - Merge branch 'master' into JDK-8266670
>  - Initial support for accessFlags methods
>  - Add mask to access flag functionality.
>  - ... and 16 more: 
> https://git.openjdk.java.net/jdk/compare/0053820b...14980605

Here, I argue for the case of splitting different types of access flags into 
distinct enums, such as `MemberAccessFlag`, `ModuleRequiresAccessFlag`, 
`ModuleExportsAccessFlag` implementing a sealed interface `AccessFlag` that 
keeps all its existing methods, since most of those access flags will never be 
returned in the same collection. The `accessFlags()` method should return a 
`Set`, which is 1. safe as the set is read-only; 2. can 
be overridden with `Set` (For forward compability, 
we can make `MemberAccessFlag` an interface with static final fields, 
implemented by some package-private enum in case we want to split it into more 
specific types down the road).

But you may ask, what about `SYNTHETIC`, `MANDATED`, `FINAL`? Aren't they 
shared by almost all of the locations? What if users test with incorrect enum 
instance, such as looking for the `MemberAccessFlag.SYNTHETIC` in 
`moduleDescriptor.accessFlags()`?

This problem can be prevented:
1. by making `ModuleDescriptor.accessFlags()` return `Set`, thus the IDE can easily detect misuse when they 
insert the access flag of a different type;
2. In the current hodgepodge `AccessFlag`, we have `STATIC` and `STATIC_PHASE`, 
and the incorrect `ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC)` 
call is much more subtle, especially to new users of this class. Arguably, this 
misuse would be way worse than that in the distinct enum case.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v3]

[liach]

On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   moving nr declaration from the beginning of the method to where it's 
> actually used

Yeah forgot about that part. Thus I guess using soft reference would be a 
better way to deal with this extra memory use issue. So something like

private SoftReference skipBuffer;

private byte[] skipBuffer(long remaining) {
int size = (int) Math.min(MAX_SKIP_BUFFER_SIZE, remaining);
SoftReference ref = this.skipBuffer;
byte[] buffer;
if (ref == null || (buffer = ref.get()) == null || buffer.length < size) {
buffer = new byte[size];
this.skipBuffer = new SoftReference(buffer);
}
return buffer;
}


This should be thread-safe, and we can just call `byte[] skipBuffer = 
skipBuffer(remaining);`

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v3]

[liach]

On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   moving nr declaration from the beginning of the method to where it's 
> actually used

On a side note for unifying the skip buffer implementation of reader vs input 
stream: For the input stream subclasses in the JDK that have their own skip 
with buffering logic (as described in 
https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they almost 
always have only local-variable skip buffers (not kept as fields for reuse), 
and their buffers' max sizes are smaller that provided by the InputStream class.

Imo we should check the usage of `skip` in other projects; in JDK it's like 
skipping 2 bytes in certain image formats, and I would expect usages like 
reading class file attribute name and size then skip by the read size.

> This change may be problematic for servers with a large number connections 
> and an input stream for each connection. It could add up to 2k to the 
> footprint of each connection when skip is used.

If per-object allocation is a problem, would it be feasible to allocate a 
static soft reference to a max-sized skip buffer? It can be potentially shared 
with the `Reader` class, too.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v2]

[liach]

On Wed, 13 Apr 2022 14:45:31 GMT, Roger Riggs  wrote:

>> src/java.base/share/classes/java/io/InputStream.java line 557:
>> 
>>> 555: 
>>> 556: while (remaining > 0) {
>>> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
>> 
>> I recommend moving `nr` declaration from the beginning of the method to 
>> where it's actually used (here)
>
> The check for `skipBuffer.length < size` makes it appear that the buffer can 
> be re-allocated.
> If it is allocated once then only the `skipBuffer == null` is needed.
> 
> The code may be simpler if the 'size' variable is removed.
> 
> byte[] skipBuffer = this.skipBuffer;
> if (skipBuffer == null) {
> this.skipBuffer = skipBuffer = 
> new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ? 
> MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
> }
> while (remaining > 0) {
> int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length, 
> remaining));

It indeed is reallocated when the existing one is not large enough.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v2]

[liach]

On Tue, 12 Apr 2022 22:15:22 GMT, XenoAmess  wrote:

> What subclasses of InputStream in the JDK do not override skip(n)?

>From a peek, the majority of subclasses do not override `skip(long)`. Most 
>overrides are delegations or optimizations for random access structures; but 
>there are some that create their custom local variable skip buffers, usually 
>of size 512 or minimum of 512 and the skip size. These custom skip buffer ones 
>IMO should have their overrides removed.

> Most sequential streams are open for a relatively short period of time, the 
> lifetime of the
> memory for the buffer won't change the memory usage enough to notice.

True, and with good use of try-with-resources, these instance fields' array 
allocations shouldn't be too much of a problem compared to allocation in every 
skip(long) call.

> If the concern is about tying up memory then allocate the buffer once and
don't resize it. Each resize consumes extra memory and gc cycles to reclaim the 
last buffer.
> Use the requested size but at least nnn and at most MAX_SKIP_BUFFER_SIZE.

Shouldn't be too problematic, as most skip usages in JDK as I see are skipping 
small number of bytes like 2 or 4, or like skipping over attributes of Java 
class files. A minimum skip buffer size isn't that helpful, as I don't think we 
often see skip calls with slowly incremental sizes.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v2]

[liach]

On Tue, 12 Apr 2022 22:19:18 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add MIN_SKIP_BUFFER_SIZE

src/java.base/share/classes/java/io/InputStream.java line 557:

> 555: 
> 556: while (remaining > 0) {
> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));

I recommend moving `nr` declaration from the beginning of the method to where 
it's actually used (here)

-

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

Re: RFR: 8284702: Add @since for java.time.LocalDate.EPOCH

[liach]

On Tue, 12 Apr 2022 03:21:00 GMT, Glavo  wrote:

> `java.time.LocalDate.EPOCH` was introduced in Java 9, but there is no 
> corresponding `@since` in the javadoc. The absence of `@since` makes it 
> impossible for IDEs to check for misuse of it, it may be misused by users 
> targeting Java 8 and crash at runtime.

Hmm, isn't this misuse detected by `--release 8` in javac flags?

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object

[liach]

On Mon, 11 Apr 2022 22:31:01 GMT, Brian Burkhalter  wrote:

> Even so, it does appear that prior buffer allocations could be "lost" due to 
> concurrent use of skip resulting in a "minor" memory leak in the heap.

The minor leak, though maybe somewhat wasteful, is the necessary cost for 
thread safety as opposed to locking or synchronizing to reliably retrieve a 
previously allocated array from another thread.

Another concern about this change is that now the skip buffer is no longer 
immediately GC'd when exiting the scope but will live on till the input stream 
is no longer reachable. For instance, if an input stream is skipped and then 
kept for long, this may be a performance regression instead for the increased 
memory use.

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object

[liach]

On Sat, 9 Oct 2021 17:08:50 GMT, XenoAmess  wrote:

> in extream situation, when doing this.skipBuffer = skipBuffer in Thread B, it 
> might make this.skipBuffer to a byte[6] in thread A, and thus cause a 
> IndexOutofBoundException in Thread A.

No, it won't. The later `skipBuffer` references are made to the local variable; 
so even though the `new byte[6]` may replace the cached `new byte[10]` skip 
buffer in instance field, in thread A, it is still using the old `new byte[10]` 
which is stored in the local variable/stack, and everything just proceeds fine 
(only shortcoming is that the 10-length skip buffer is wasted and recycled)

-

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

Re: RFR: 8284638: store skip buffers in InputStream Object

[liach]

On Fri, 8 Oct 2021 21:19:36 GMT, XenoAmess  wrote:

> @jmehrens what about this then?
> I think it safe now(actually this mechanism is learned from Reader)

You should really post your changes to jdk-dev at openjdk.java.net before 
submitting these so you can know if a change is potentially good or bad. Since 
jdk pull requests require issue numbers, you can ask people on the list if this 
is a good idea; if it is, someone on the list may create an issue for you, 
which probably means your suggestion is valid, and only then you should send a 
pull request.

You need to get a ticket so this pr may get merged. probably through suggesting 
enhancement on https://bugs.java.com (recommended, takes time but guarantees 
response) or asking on the mailing list

src/java.base/share/classes/java/io/InputStream.java line 550:

> 548: 
> 549: if ((skipBuffer == null) || (skipBuffer.length < size)) {
> 550: skipBuffer = new byte[size];

In the `Reader` class, `skipBuffer` access is thread-safe as it's behind a 
synchronization on the `lock`. This one isn't and is subject to weird java 
memory models: in extreme cases, if the `skipBuffer` is set to non-null on 
another thread, it may be non-null on first field read and null on second field 
read.

Hence you should write code like this instead:

byte[] skipBuffer = this.skipBuffer;
if ((skipBuffer == null) || (skipBuffer.length < size)) {
this.skipBuffer = skipBuffer = new byte[size];
}

so you only read the field once and get consistent results.

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap [v12]

[liach]

On Sat, 9 Apr 2022 15:25:47 GMT, XenoAmess  wrote:

> > Quick question: If the maps are intended to be fixed-size, can't the users 
> > just call `new HashMap<>(size, 1)`, increasing the growth factor to prevent 
> > growth?
> 
> @liach this questions equals question : "why default load factor be 0.75 not 
> 1" In short, when larger the load factor, smaller memory use, and larger 
> hash-collide rate, larger crud time cost. when smaller the load factor, 
> larger memory use, and smaller hash-collide rate, smaller crud time cost. As 
> for why default load factor be 0.75 but not 1, it is historical to me. it is 
> already 0.75 when I first learn java and never changed then. But I guess 
> there be some empirical formula behind it.

No. I mean when the growth factor is 1 and exactly `size` key-value pairs are 
put into the map, the map will not grow; but when it's 0.75, the map will 
preemptively grow erroneously. Changing the growth factor is effectively no-op 
for a hash map that will not have more elements added later during its usage.

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap [v12]

[liach]

On Wed, 6 Apr 2022 16:02:31 GMT, XenoAmess  wrote:

>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use (double) DEFAULT_LOAD_FACTOR instead of 0.75

Quick question: If the maps are intended to be fixed-size, can't the users just 
call `new HashMap<>(size, 1)`, increasing the growth factor to prevent growth?

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]

[liach]

On Wed, 6 Apr 2022 01:13:11 GMT, Stuart Marks  wrote:

>> src/java.base/share/classes/java/util/LinkedHashMap.java line 804:
>> 
>>> 802:  * @since 19
>>> 803:  */
>>> 804: public static  LinkedHashMap newLinkedHashMap(int 
>>> numMappings) {
>> 
>> `LinkedHashMap` may be often extended for it has a `protected boolean 
>> removeEldestEntry(Entry)`. Should we make a separate factory method for such 
>> instances (with functional implementation) or just expose 
>> `HashMap.calculateHashMapCapacity`?
>
> Good question. Having to subclass and override this method in order to 
> provide a removal policy has always seemed rather clumsy. However, it's the 
> supported approach, and it's done fairly frequently in the wild. A new 
> subclass requires that the caller invoke `new` on that specific subclass, 
> which in turn must choose which superclass constructor to call. This means 
> that a static factory method can't be used. The alternatives would be to 
> expose another constructor or to expose `calculateHashMapCapacity` as you 
> suggest. A new constructor might also need to control the load factor and the 
> ordering policy (insertion vs access order) so that's a fairly complicated 
> overload to consider.
> 
> Exposing the calculate method might help but that's mostly just a wrapper 
> around a small computation. As we've seen it's easy to get this computation 
> wrong, but exposing a method that _just_ does this computation seems like a 
> really narrow case.
> 
> (Still another alternative would be to pass a lambda expression that's called 
> at the appropriate time. That would involve adding a `BiPredicate, 
> Map.Entry>` to yet another constructor overload. This could work but it 
> doesn't seem any simpler.)
> 
> The need for subclassing LinkedHashMap and overriding this method might also 
> be reduced by the addition of new APIs from the Sequenced Collections 
> proposal (https://openjdk.java.net/jeps/8280836). One simply needs to call 
> `pollFirstEntry` at the right time. That might remove the need to have some 
> expiration policy plugged directly into the map itself.
> 
> I'm not inclined to add more APIs to cover what seems to be a fairly narrow 
> case, but we might keep this in mind to see if anything useful pops up.

True. My initial idea was to pass a lambda expression if we do make another 
factory.

However, when I look at the constructor parameters, I realized that for those 
evicting-queue-like use cases, it's more preferable for users to just call


new LinkedHashMap<>(cacheSize + 1, 1, true) {
protected boolean removeEldestEntry(Entry entry) {
return size() > cacheSize;
}
}

which preallocates the cache, avoids growth when the cache is full, and removes 
by least-recently-used.

IMO this use case is better suited by the existing APIs (like the call above) 
than a new factory method, especially that many libraries, like guava or 
caffeine, offer more efficient caches.

So yes, this patch in the current state looks good. In the JDK, it seems there 
are a few cases of linked hash map evicting by size that can have the map 
initialized to be presized and avoid growth. But that's off-topic here.

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]

[liach]

On Sat, 2 Apr 2022 22:46:19 GMT, XenoAmess  wrote:

>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   revert changes in jdk.compile

src/java.base/share/classes/java/util/LinkedHashMap.java line 804:

> 802:  * @since 19
> 803:  */
> 804: public static  LinkedHashMap newLinkedHashMap(int 
> numMappings) {

`LinkedHashMap` may be often extended for it has a `protected boolean 
removeEldestEntry(Entry)`. Should we make a separate factory method for such 
instances (with functional implementation) or just expose 
`HashMap.calculateHashMapCapacity`?

-

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

Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v17]

[liach]

On Tue, 29 Mar 2022 13:58:56 GMT, ExE Boss  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add condy static methods
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 335:
> 
>> 333: CarrierArray(int primitiveCount, int objectCount) {
>> 334: this.primitives =
>> 335: primitiveCount != 0 ? new long[(primitiveCount + 1) 
>> / 2] : null;
> 
> It might be better to use an unsigned shift here:
> Suggestion:
> 
> primitiveCount != 0 ? new long[(primitiveCount + 1) >>> 
> 1] : null;

doesn't the compiler (either javac or hotspot) automatically do that?

-

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

Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v3]

[liach]

On Thu, 10 Mar 2022 08:52:17 GMT, Сергей Цыпанов  wrote:

>> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with 
>> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when 
>> called with vararg of size 0, 1, 2.
>> 
>> In general replacement of `Arrays.asList()` with `List.of()` is dubious as 
>> the latter is null-hostile, however in some cases we are sure that arguments 
>> are non-null. Into this PR I've included the following cases (in addition to 
>> those where the argument is proved to be non-null at compile-time):
>> - `MethodHandles.longestParameterList()` never returns null
>> - parameter types are never null
>> - interfaces used for proxy construction and returned from 
>> `Class.getInterfaces()` are never null
>> - exceptions types of method signature are never null
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8282662: Revert dubious changes in MethodType

Just curious, this issue asks to replace set constructions with `Set.of`, but 
there is no such code changes in this pull request. Is there any set creation 
patterns that aren't detected by your ide cleanups, such as 
`Collections.addAll(set, elements)` for creating hash set contents from array?

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap [v7]

[liach]

On Sat, 26 Mar 2022 12:14:25 GMT, XenoAmess  wrote:

>> src/java.base/share/classes/java/util/HashMap.java line 2584:
>> 
>>> 2582: 
>>> 2583: /**
>>> 2584:  * Creates a new, empty HashMap with an initial table size
>> 
>> You probably wanna allow for a non-NEW instance for the corner case where 
>> the given size is 0 - no elements.
>
>> You probably wanna allow for a non-NEW instance for the corner case where 
>> the given size is 0 - no elements.
> 
> @ChrisHegarty I guess we shouldn't.
> 
> I want to make it 100% equals to `new HashMap()` constructor, thus migrate 
> all usecases.
> 
> So if we apply this, and when the original usage use this map object as a 
> lock, or put some elements after the call(sometimes people cannot decide if 
> they would really put elements in this map), bad things would happen.
> 
> Besides, there already a function` Map.of()` for such functionality, so 
> programmer should use that instead if they really want a shared empty map.

hash maps are modifiable. empty instances can be changed, and returning such 
shared instances are inherently unsafe.

-

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

Re: RFR: 8282178: Replace simple iterations of Map.entrySet with Map.forEach calls

[liach]

On Wed, 23 Feb 2022 22:33:42 GMT, liach  wrote:

> Replaces simple `for (Map.Entry entry : map.entrySet())` with 
> `map.forEach((k, v) ->)` calls. This change is better for thread-safety and 
> reduces allocation for some map implementations.
> 
> A more in-depth description of benefits is available at 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-February/086201.html
>  and at the JBS issue itself.
> 
> A [jmh 
> comparison](https://jmh.morethan.io/?sources=https://gist.githubusercontent.com/liach/0c0f79f0c0b9b78f474d65cda2c5f7b5/raw/4f2a160c51164aefdfac6ab5a19bdbc8c65f5fcf/base-results.json,https://gist.githubusercontent.com/liach/0c0f79f0c0b9b78f474d65cda2c5f7b5/raw/4f2a160c51164aefdfac6ab5a19bdbc8c65f5fcf/head-results.json)
>  on the performance of the existing `HashMapBench` shows that the performance 
> of `putAll` for `HashMap` has not significantly changed.

In a short summary, on mailing list, stuart says this doesn't benefit 
concurrent maps; Remi proposed to deprecate the synchronized collections for 
the peculiarities of iteration; kevin said that this change improves code 
clarity.

Hence, I still believe changing to foreach calls in putAll can potentially 
benefit adding from maps that have expensive iterators and entry wrappers.

-

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