Re: RFR: 8266310: deadlock while loading the JNI code [v5]

[David Holmes]

On Fri, 21 May 2021 15:49:09 GMT, Aleksei Voitylov  
wrote:

>> Please review this PR which fixes the deadlock in ClassLoader between the 
>> two lock objects - a lock object associated with the class being loaded, and 
>> the ClassLoader.loadedLibraryNames hash map, locked during the native 
>> library load operation.
>> 
>> Problem being fixed:
>> 
>> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile 
>> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is 
>> removed because there's another lock object in the class loader, associated 
>> with the name of the class being loaded. Such objects are stored in 
>> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads 
>> exactly the same class, whose signature is being verified in another thread.
>> 
>> Proposed fix:
>> 
>> The proposed patch suggests to get rid of locking loadedLibraryNames hash 
>> map and synchronize on each entry name, as it's done with class names in see 
>> ClassLoader.getClassLoadingLock(name) method.
>> 
>> The patch introduces nativeLibraryLockMap which holds the lock objects for 
>> each library name, and the getNativeLibraryLock() private method is used to 
>> lazily initialize the corresponding lock object. nativeLibraryContext was 
>> changed to ThreadLocal, so that in any concurrent thread it would have a 
>> NativeLibrary object on top of the stack, that's being currently 
>> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names 
>> of all native libraries loaded - in line with class loading code, it is not 
>> explicitly cleared.
>> 
>> Testing:  jtreg and jck testing with no regressions. A new regression test 
>> was developed.
>
> Aleksei Voitylov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fix whitespace

Hi Aleksei,

Thanks for bearing with me while I got my head round the synchronization 
protocol - piggy-backing on the CHM logic is clever! That locking part all 
makes sense to me now and I think it is a good solution to reduce the scope of 
the deadlock potential.

I'm less clear on the changes to the NativeLibraryContext as I'm not certain 
what it means today, so making it a per-thread mapping is not something I can 
really comment on. I'll leave that aspect to core-libs folk. So my approval 
should not be taken as an approval to push yet.

Thanks,
David

src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 497:

> 495: 
> 496: private static void acquireNativeLibraryLock(String libraryName) {
> 497: nativeLibraryLockMap.compute(libraryName, (name, lock) -> {

This would be clearer if lock were named currentLock as for releaseNLLock

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v6]

[Tagir F . Valeev]

On Wed, 26 May 2021 02:22:42 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - JapaneseImperialCalendar: use switch expressions
>  - Use yield in java.util.Calendar.Builder.build

src/java.base/share/classes/java/util/JapaneseImperialCalendar.java line 1316:

> 1314: JapaneseImperialCalendar jc = getNormalizedCalendar();
> 1315: LocalGregorianCalendar.Date date = jc.jdate;
> 1316: int normalizedYear = date.getNormalizedYear();

This variable was unused

src/java.base/share/classes/java/util/JapaneseImperialCalendar.java line 1423:

> 1421: case WEEK_OF_MONTH -> {
> 1422: LocalGregorianCalendar.Date jd = 
> jcal.getCalendarDate(Long.MAX_VALUE, getZone());
> 1423: if (date.getEra() == jd.getEra() && date.getYear() == 
> jd.getYear()) {

I inverted the `if` condition here, as it was negated and a much shorter branch 
was in `else`.

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Tagir F . Valeev]

On Tue, 25 May 2021 16:52:06 GMT, Brian Goetz  wrote:

>> Tagir F. Valeev has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   More vertical alignment
>
> src/java.base/share/classes/java/util/JapaneseImperialCalendar.java line 1371:
> 
>> 1369: }
>> 1370: }
>> 1371: }
> 
> Pull value assignment out of switch?

This is a much bigger change which is probably harder to review. I did it, 
please take a look. One point is whether to unwrap the final `else` after 
`yield`:

if (...) {
...
yield ...
} else { // should we remove else?
...
}

I prefer unwrapping, as this reduces the indentation, so I did it. Please tell 
me if this contradicts with the preferred OpenJDK style.

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v6]

[Iris Clark]

On Wed, 26 May 2021 02:22:42 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - JapaneseImperialCalendar: use switch expressions
>  - Use yield in java.util.Calendar.Builder.build

Marked as reviewed by iris (Reviewer).

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v6]

[Tagir F . Valeev]

> Inspired by PR#4088. Most of the changes are done automatically using 
> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
> including indentations, moving comments, extracting common cast out of switch 
> expression branches, etc.
> 
> I also noticed that there are some switches having one branch only in 
> JapaneseImperialCalendar.java. Probably it would be better to replace them 
> with `if` statement?

Tagir F. Valeev has updated the pull request incrementally with two additional 
commits since the last revision:

 - JapaneseImperialCalendar: use switch expressions
 - Use yield in java.util.Calendar.Builder.build

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4161/files
  - new: https://git.openjdk.java.net/jdk/pull/4161/files/f4ad5f14..e8cdf10e

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

  Stats: 155 lines in 2 files changed: 56 ins; 61 del; 38 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4161.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4161/head:pull/4161

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Tagir F . Valeev]

On Tue, 25 May 2021 16:47:15 GMT, Brian Goetz  wrote:

>> Tagir F. Valeev has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   More vertical alignment
>
> src/java.base/share/classes/java/util/Calendar.java line 1507:
> 
>> 1505: }
>> 1506: case "japanese" -> cal = new 
>> JapaneseImperialCalendar(zone, locale, true);
>> 1507: default -> throw new IllegalArgumentException("unknown 
>> calendar type: " + type);
> 
> Agree with Chris' suggestion here.

Done!

> src/java.base/share/classes/java/util/JapaneseImperialCalendar.java line 1124:
> 
>> 1122: return Math.max(LEAST_MAX_VALUES[YEAR], d.getYear());
>> 1123: }
>> 1124: }
> 
> A switch with one element here is kind of weird.  I would turn this into 
> "return switch (field) { ... }", with two cases, YEAR and default.

Done, also at line 1169

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Tagir F . Valeev]

On Tue, 25 May 2021 15:51:38 GMT, Chris Hegarty  wrote:

>> Tagir F. Valeev has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   More vertical alignment
>
> src/java.base/share/classes/java/util/Calendar.java line 1507:
> 
>> 1505: }
>> 1506: case "japanese" -> cal = new 
>> JapaneseImperialCalendar(zone, locale, true);
>> 1507: default -> throw new IllegalArgumentException("unknown 
>> calendar type: " + type);
> 
> In this case, it would be good to yield the result of the switch expression 
> and assign that to a local, rather than assigning in each of the case arms, 
> e.g:
> 
> final Calendar cal = switch (type) {
> case "gregory" -> new GregorianCalendar(zone, locale, true);
> case "iso8601" -> {
> GregorianCalendar gcal = new GregorianCalendar(zone, locale, 
> true);
> // make gcal a proleptic Gregorian
> gcal.setGregorianChange(new Date(Long.MIN_VALUE));
> // and week definition to be compatible with ISO 8601
> setWeekDefinition(MONDAY, 4);
> yield gcal;
> }
> case "buddhist" -> {
> var buddhistCalendar = new BuddhistCalendar(zone, locale);
> buddhistCalendar.clear();
> yield buddhistCalendar;
> }
> case "japanese" -> new JapaneseImperialCalendar(zone, locale, true);
> default -> throw new IllegalArgumentException("unknown calendar type: 
> " + type);
> };

Done, thanks!

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v3]

[Iris Clark]

On Tue, 25 May 2021 21:43:36 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.io`, 
>> `java.math`, and `java.text` packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8267670: Remove redundant code from switch

Marked as reviewed by iris (Reviewer).

-

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

Integrated: 8267452: Delegate forEachRemaining in Spliterators.iterator()

[Tagir F . Valeev]

On Thu, 20 May 2021 10:16:28 GMT, Tagir F. Valeev  wrote:

> Sometimes, `Spliterator::forEachRemaining` has much more optimized 
> implementation, compared to `Spliterator::tryAdvance`. For example, if a 
> `Stream::spliterator` called for a stream that has intermediate operations, 
> `tryAdvance()` may buffer intermediate elements while `forEachRemaining()` 
> just delegates to the `wrapAndCopyInto` (see 
> `StreamSpliterators.AbstractWrappingSpliterator` and its inheritors).
> 
> `Spliterators::iterator` methods (used in particular by `Stream::iterator`) 
> provide adapter iterators that delegate to the supplied spliterator. 
> Unfortunately, they don't have a specialized implementation for 
> `Iterator::forEachRemaining`. As a result, a default implementation is used 
> that delegates to `hasNext`/`next`, which in turn causes the delegation to 
> `tryAdvance`. It's quite simple to implement `Iterator::forEachRemaining` 
> here, taking advantage of possible spliterator optimizations if the iterator 
> client decides to use `forEachRemaining`.
> 
> This patch implements Iterator::forEachRemaining in Spliterators::iterator 
> methods. Also, I nullize the `nextElement` in `Iterator::next` to avoid 
> accidental prolongation of the user's object lifetime when iteration is 
> finished. Finally, a small cleanup: I added the `final` modifier where 
> possible to private fields in `Spliterators`.
> 
> Test-wise, some scenarios are already covered by 
> SpliteratorTraversingAndSplittingTest. However, the resulting iterator is 
> always wrapped into `Spliterators::spliterator`, so usage scenarios are 
> somewhat limited. In particular, calling `hasNext` (without `next`) before 
> `forEachRemaining` was not covered there. I added more tests in 
> `IteratorFromSpliteratorTest` to cover these scenarios. I checked that 
> removing `valueReady = false;` or `action.accept(t);` lines from newly 
> implemented `forEachRemaining` method causes new tests to fail (but old tests 
> don't fail due to this).

This pull request has now been integrated.

Changeset: ac36b7d3
Author:Tagir F. Valeev 
URL:   
https://git.openjdk.java.net/jdk/commit/ac36b7d3e2d521652576fba3b1760586f582544f
Stats: 198 lines in 2 files changed: 193 ins; 0 del; 5 mod

8267452: Delegate forEachRemaining in Spliterators.iterator()

Reviewed-by: psandoz

-

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

Re: RFR: 8267452: Delegate forEachRemaining in Spliterators.iterator() [v3]

[Tagir F . Valeev]

> Sometimes, `Spliterator::forEachRemaining` has much more optimized 
> implementation, compared to `Spliterator::tryAdvance`. For example, if a 
> `Stream::spliterator` called for a stream that has intermediate operations, 
> `tryAdvance()` may buffer intermediate elements while `forEachRemaining()` 
> just delegates to the `wrapAndCopyInto` (see 
> `StreamSpliterators.AbstractWrappingSpliterator` and its inheritors).
> 
> `Spliterators::iterator` methods (used in particular by `Stream::iterator`) 
> provide adapter iterators that delegate to the supplied spliterator. 
> Unfortunately, they don't have a specialized implementation for 
> `Iterator::forEachRemaining`. As a result, a default implementation is used 
> that delegates to `hasNext`/`next`, which in turn causes the delegation to 
> `tryAdvance`. It's quite simple to implement `Iterator::forEachRemaining` 
> here, taking advantage of possible spliterator optimizations if the iterator 
> client decides to use `forEachRemaining`.
> 
> This patch implements Iterator::forEachRemaining in Spliterators::iterator 
> methods. Also, I nullize the `nextElement` in `Iterator::next` to avoid 
> accidental prolongation of the user's object lifetime when iteration is 
> finished. Finally, a small cleanup: I added the `final` modifier where 
> possible to private fields in `Spliterators`.
> 
> Test-wise, some scenarios are already covered by 
> SpliteratorTraversingAndSplittingTest. However, the resulting iterator is 
> always wrapped into `Spliterators::spliterator`, so usage scenarios are 
> somewhat limited. In particular, calling `hasNext` (without `next`) before 
> `forEachRemaining` was not covered there. I added more tests in 
> `IteratorFromSpliteratorTest` to cover these scenarios. I checked that 
> removing `valueReady = false;` or `action.accept(t);` lines from newly 
> implemented `forEachRemaining` method causes new tests to fail (but old tests 
> don't fail due to this).

Tagir F. Valeev has updated the pull request incrementally with one additional 
commit since the last revision:

  Add a comment clarifying the purpose of the test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4124/files
  - new: https://git.openjdk.java.net/jdk/pull/4124/files/9a5e0dd2..cc337995

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

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

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

Re: RFR: 8267452: Delegate forEachRemaining in Spliterators.iterator() [v2]

[Tagir F . Valeev]

On Tue, 25 May 2021 15:49:55 GMT, Paul Sandoz  wrote:

>> Tagir F. Valeev has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Test: formatting; tests for empty spliterator
>
> Oops i missed the bit about testing in the description. That's subtle, the 
> `hasNext` occurs in the assert before the `forEachRemaining`. Could you add a 
> comment that this is the primary purpose of the test since 
> `SpliteratorTraversingAndSplittingTest` cannot do that for a spliterator 
> wrapping an iterator?

@PaulSandoz added a comment, thanks!

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v3]

[Naoto Sato]

On Tue, 25 May 2021 21:43:36 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.io`, 
>> `java.math`, and `java.text` packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8267670: Remove redundant code from switch

Marked as reviewed by naoto (Reviewer).

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v9]

[Brent Christian]

On Tue, 25 May 2021 21:45:33 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Editorial updates
>   Updated java.security properties to include jdk.serialFilterFactory
>   Added test cases to SerialFilterFactoryTest for java.security properties and
>   enabling of the SecurityManager with existing policy permission files
>   Corrected a test that OIS.setObjectInputFilter could not be called twice.
>   Removed a Factory test that was not intended to be committed

src/java.base/share/classes/java/io/ObjectInputFilter.java line 513:

> 511:  * the static JVM-wide filter, or to create a filter from a pattern 
> string.
> 512:  * The static filter factory and the static filter apply to the 
> whole Java runtime,
> 513:  * or "JVM-wide", there is only one of each, for a complete 
> description of

Suggest new sentence after "_...there is only one of each.  For a complete..._"

src/java.base/share/classes/java/io/ObjectInputFilter.java line 551:

> 549: final class Config {
> 550: /**
> 551:  * Lock object for filter and filter factory.

The lock is not used for the filter factory, is it?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 768:

> 766:  * This package private method is *only* called by {@link 
> ObjectInputStream#ObjectInputStream()}
> 767:  * and  {@link ObjectInputStream#ObjectInputStream(InputStream)}.
> 768:  * {@link ObjectInputFilter.Config#serialFilterFactory} does the 
> enforcement.

Is this still true about the enforcement?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 1251:

> 1249:  * Returns REJECTED if either of the filters returns 
> REJECTED,
> 1250:  * otherwise, ALLOWED if either of the filters returns 
> ALLOWED.
> 1251:  * otherwise, returns {@code UNDECIDED}.

Capitalize "Otherwise"

src/java.base/share/classes/java/io/ObjectInputFilter.java line 1256:

> 1254:  * @return REJECTED if either of the filters returns 
> REJECTED,
> 1255:  *  otherwise, ALLOWED if either of the filters 
> returns ALLOWED.
> 1256:  *  otherwise, returns {@code UNDECIDED}.

Otherwise

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v8]

[Brent Christian]

On Tue, 25 May 2021 15:46:37 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Moved utility filter methods to be static on ObjectInputFilter
>Rearranged the class javadoc of OIF to describe the parts of
>deserialization filtering, filters, composite filters, and the filter 
> factory.
>And other review comment updates...
>  - Refactored tests for utility functions to SerialFilterFunctionTest.java
>Deleted confused Config.allowMaxLimits() method
>Updated example to match move of methods to Config
>Added test of restriction on setting the filterfactory after a OIS has 
> been created
>Additional Editorial updates

More suggestions for ObjectInputFilter.java.  I hope they're helpful.

src/java.base/share/classes/java/io/ObjectInputFilter.java line 128:

> 126:  * provides the {@linkplain Config#getSerialFilter static JVM-wide 
> filter} when invoked from the
> 127:  * {@linkplain ObjectInputStream#ObjectInputStream(InputStream) 
> ObjectInputStream constructors}
> 128:  * and replaces the static filter and when invoked from

"...replaces the static filter ~~and~~ when invoked..."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 137:

> 135:  * or to {@linkplain #allowFilter(Predicate, Status) allow} or
> 136:  * {@linkplain #rejectFilter(Predicate, Status) reject} classes based on 
> a
> 137:  * {@linkplain Predicate predicate of a class}.

Maybe a new sentence for the Predicate-based filters? e.g. "Filters can be 
created from a pattern string.  Or they can allow or reject classes based on a 
Predicate."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 205:

> 203:  * // Returns a composite filter of the static JVM-wide filter, a 
> thread-specific filter,
> 204:  * // and the stream-specific filter.
> 205:  * public ObjectInputFilter apply(ObjectInputFilter curr, 
> ObjectInputFilter next) {

`@Override` on `apply` ?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 211:

> 209:  * if (filter != null) {
> 210:  * // Prepend a filter to assert that all classes have 
> been Allowed or Rejected
> 211:  * filter = 
> ObjectInputFilter.Config.rejectUndecidedClass(filter);

~~Config~~ throughout the example

src/java.base/share/classes/java/io/ObjectInputFilter.java line 301:

> 299:  * on the class is {@code true}.
> 300:  * The filter returns {@code ALLOWED} or the {@code otherStatus} 
> based on the predicate
> 301:  * of the {@code non-null} class and {@code UNDECIDED} if the class 
> is {@code null}.

Can this sentence ("_The filter returns...class is null_") be removed? IMO it 
doesn't add to what's covered in the rest of the method doc.

src/java.base/share/classes/java/io/ObjectInputFilter.java line 334:

> 332:  * on the class is {@code true}.
> 333:  * The filter returns {@code REJECTED} or the {@code otherStatus} 
> based on the predicate
> 334:  * of the {@code non-null} class and {@code UNDECIDED} if the class 
> is {@code null}.

Same - can this sentence ("The filter returns...class is null") be removed?

-

Changes requested by bchristi (Reviewer).

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

Re: RFR: 8267123: Remove RMI Activation

[Erik Joelsson]

On Tue, 25 May 2021 18:04:45 GMT, Stuart Marks  wrote:

> This is the implementation of [JEP 407](https://openjdk.java.net/jeps/407).
> 
> This is a fairly straightforward removal of this component.
>  - Activation implementation classes removed
>  - Activation tests removed
>  - adjustments to various comments to remove references to Activation
>  - adjustments to some code to remove special-case support for Activation 
> from core RMI
>  - adjustments to several tests to remove testing and support for, and 
> mentions of Activation
>  - remove `rmid` tool
> 
> (Personally, I found that reviewing using the webrev was easier than 
> navigating github's diff viewer.)

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar

[Joe Wang]

On Tue, 25 May 2021 16:40:53 GMT, Naoto Sato  wrote:

> Please review the fix. The issue was informed yesterday by @amaembo that it 
> offends some code analysis tools.
> Although the fix is to change the condition error, it turned out that 
> `JapaneseImperialCalendar.roll()` did not work for `WEEK_OF_MONTH` and 
> `DAY_OF_MONTH`. There was an inherent issue where `GregorianCalendar` does 
> not follow the `roll()` spec, i.e., "The MONTH must not change when the 
> WEEK_OF_MONTH is rolled." during the Julian/Gregorian transition. JDK-6191841 
> seems to have tried to fix it but it was closed as `Future Project`...
> So I simply fixed the `roll()` issue in `JapaneseImperialCalendar` to follow 
> the spec here, which seems to be the intent of the original author.

Marked as reviewed by joehw (Reviewer).

-

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

RFR: 8267123: Remove RMI Activation

[Stuart Marks]

This is the implementation of [JEP 407](https://openjdk.java.net/jeps/407).

This is a fairly straightforward removal of this component.
 - Activation implementation classes removed
 - Activation tests removed
 - adjustments to various comments to remove references to Activation
 - adjustments to some code to remove special-case support for Activation from 
core RMI
 - adjustments to several tests to remove testing and support for, and mentions 
of Activation
 - remove `rmid` tool

(Personally, I found that reviewing using the webrev was easier than navigating 
github's diff viewer.)

-

Commit messages:
 - Merge branch 'master' into JDK-8267123-Remove-RMI-Activation
 - Merge branch 'master' into remove-rmi-activation
 - Clean up some old comments.
 - Small fixups.
 - First cut at removal of RMI Activation.

Changes: https://git.openjdk.java.net/jdk/pull/4194/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4194=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267123
  Stats: 21982 lines in 243 files changed: 0 ins; 21930 del; 52 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4194.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4194/head:pull/4194

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v3]

[Chris Hegarty]

On Tue, 25 May 2021 18:54:48 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.io`, 
>> `java.math`, and `java.text` packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8267670: Remove redundant code from switch

Marked as reviewed by chegar (Reviewer).

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v3]

[Patrick Concannon]

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

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

  8267670: Remove redundant code from switch

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4182/files
  - new: https://git.openjdk.java.net/jdk/pull/4182/files/adc8af41..e503ed26

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

  Stats: 24 lines in 3 files changed: 4 ins; 3 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4182.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4182/head:pull/4182

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

Integrated: 8267403: tools/jpackage/share/FileAssociationsTest.java#id0 failed with "Error: Bundler "Mac PKG Package" (pkg) failed to produce a package"

[Alexander Matveev]

On Fri, 21 May 2021 04:34:22 GMT, Alexander Matveev  
wrote:

> Looks like another issue similar to hdiutil (JDK-8249395) when process is 
> gone, but we still waiting for it. I was not able to reproduce this issue by 
> running test or pkgbuild separately and conclusion was made based on logs. 
> Fixed in same way as hdiutil issue. Also, I added logging PID for external 
> commands to simplify log analysis. Log will have multiple hdiutil and/or 
> pkgbuild processes, since we running multiple tests in parallel and matching 
> external process to test failure requires looking at command line for 
> particular process, so PID should simplify this.

This pull request has now been integrated.

Changeset: 5aa45f2e
Author:Alexander Matveev 
URL:   
https://git.openjdk.java.net/jdk/commit/5aa45f2edf278bab4403704ab4b6644096f8c077
Stats: 31 lines in 5 files changed: 16 ins; 1 del; 14 mod

8267403: tools/jpackage/share/FileAssociationsTest.java#id0 failed with "Error: 
Bundler "Mac PKG Package" (pkg) failed to produce a package"

Reviewed-by: herrick, asemenyuk

-

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

Re: RFR: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics [v15]

[Sandhya Viswanathan]

> This PR contains Short Vector Math Library support related changes for 
> [JEP-414 Vector API (Second Incubator)](https://openjdk.java.net/jeps/414), 
> in preparation for when targeted.
> 
> Intel Short Vector Math Library (SVML) based intrinsics in native x86 
> assembly provide optimized implementation for Vector API transcendental and 
> trigonometric methods.
> These methods are built into a separate library instead of being part of 
> libjvm.so or jvm.dll.
> 
> The following changes are made:
>The source for these methods is placed in the jdk.incubator.vector module 
> under src/jdk.incubator.vector/linux/native/libsvml and 
> src/jdk.incubator.vector/windows/native/libsvml.
>The assembly source files are named as “*.S” and include files are named 
> as “*.S.inc”.
>The corresponding build script is placed at 
> make/modules/jdk.incubator.vector/Lib.gmk.
>Changes are made to build system to support dependency tracking for 
> assembly files with includes.
>The built native libraries (libsvml.so/svml.dll) are placed in bin 
> directory of JDK on Windows and lib directory of JDK on Linux.
>The C2 JIT uses the dll_load and dll_lookup to get the addresses of 
> optimized methods from this library.
> 
> Build system changes and module library build scripts are contributed by 
> Magnus (magnus.ihse.bur...@oracle.com).
> 
> Looking forward to your review and feedback.
> 
> Performance:
> Micro benchmark Base Optimized Unit Gain(Optimized/Base)
> Double128Vector.ACOS 45.91 87.34 ops/ms 1.90
> Double128Vector.ASIN 45.06 92.36 ops/ms 2.05
> Double128Vector.ATAN 19.92 118.36 ops/ms 5.94
> Double128Vector.ATAN2 15.24 88.17 ops/ms 5.79
> Double128Vector.CBRT 45.77 208.36 ops/ms 4.55
> Double128Vector.COS 49.94 245.89 ops/ms 4.92
> Double128Vector.COSH 26.91 126.00 ops/ms 4.68
> Double128Vector.EXP 71.64 379.65 ops/ms 5.30
> Double128Vector.EXPM1 35.95 150.37 ops/ms 4.18
> Double128Vector.HYPOT 50.67 174.10 ops/ms 3.44
> Double128Vector.LOG 61.95 279.84 ops/ms 4.52
> Double128Vector.LOG10 59.34 239.05 ops/ms 4.03
> Double128Vector.LOG1P 18.56 200.32 ops/ms 10.79
> Double128Vector.SIN 49.36 240.79 ops/ms 4.88
> Double128Vector.SINH 26.59 103.75 ops/ms 3.90
> Double128Vector.TAN 41.05 152.39 ops/ms 3.71
> Double128Vector.TANH 45.29 169.53 ops/ms 3.74
> Double256Vector.ACOS 54.21 106.39 ops/ms 1.96
> Double256Vector.ASIN 53.60 107.99 ops/ms 2.01
> Double256Vector.ATAN 21.53 189.11 ops/ms 8.78
> Double256Vector.ATAN2 16.67 140.76 ops/ms 8.44
> Double256Vector.CBRT 56.45 397.13 ops/ms 7.04
> Double256Vector.COS 58.26 389.77 ops/ms 6.69
> Double256Vector.COSH 29.44 151.11 ops/ms 5.13
> Double256Vector.EXP 86.67 564.68 ops/ms 6.52
> Double256Vector.EXPM1 41.96 201.28 ops/ms 4.80
> Double256Vector.HYPOT 66.18 305.74 ops/ms 4.62
> Double256Vector.LOG 71.52 394.90 ops/ms 5.52
> Double256Vector.LOG10 65.43 362.32 ops/ms 5.54
> Double256Vector.LOG1P 19.99 300.88 ops/ms 15.05
> Double256Vector.SIN 57.06 380.98 ops/ms 6.68
> Double256Vector.SINH 29.40 117.37 ops/ms 3.99
> Double256Vector.TAN 44.90 279.90 ops/ms 6.23
> Double256Vector.TANH 54.08 274.71 ops/ms 5.08
> Double512Vector.ACOS 55.65 687.54 ops/ms 12.35
> Double512Vector.ASIN 57.31 777.72 ops/ms 13.57
> Double512Vector.ATAN 21.42 729.21 ops/ms 34.04
> Double512Vector.ATAN2 16.37 414.33 ops/ms 25.32
> Double512Vector.CBRT 56.78 834.38 ops/ms 14.69
> Double512Vector.COS 59.88 837.04 ops/ms 13.98
> Double512Vector.COSH 30.34 172.76 ops/ms 5.70
> Double512Vector.EXP 99.66 1608.12 ops/ms 16.14
> Double512Vector.EXPM1 43.39 318.61 ops/ms 7.34
> Double512Vector.HYPOT 73.87 1502.72 ops/ms 20.34
> Double512Vector.LOG 74.84 996.00 ops/ms 13.31
> Double512Vector.LOG10 71.12 1046.52 ops/ms 14.72
> Double512Vector.LOG1P 19.75 776.87 ops/ms 39.34
> Double512Vector.POW 37.42 384.13 ops/ms 10.26
> Double512Vector.SIN 59.74 728.45 ops/ms 12.19
> Double512Vector.SINH 29.47 143.38 ops/ms 4.87
> Double512Vector.TAN 46.20 587.21 ops/ms 12.71
> Double512Vector.TANH 57.36 495.42 ops/ms 8.64
> Double64Vector.ACOS 24.04 73.67 ops/ms 3.06
> Double64Vector.ASIN 23.78 75.11 ops/ms 3.16
> Double64Vector.ATAN 14.14 62.81 ops/ms 4.44
> Double64Vector.ATAN2 10.38 44.43 ops/ms 4.28
> Double64Vector.CBRT 16.47 107.50 ops/ms 6.53
> Double64Vector.COS 23.42 152.01 ops/ms 6.49
> Double64Vector.COSH 17.34 113.34 ops/ms 6.54
> Double64Vector.EXP 27.08 203.53 ops/ms 7.52
> Double64Vector.EXPM1 18.77 96.73 ops/ms 5.15
> Double64Vector.HYPOT 18.54 103.62 ops/ms 5.59
> Double64Vector.LOG 26.75 142.63 ops/ms 5.33
> Double64Vector.LOG10 25.85 139.71 ops/ms 5.40
> Double64Vector.LOG1P 13.26 97.94 ops/ms 7.38
> Double64Vector.SIN 23.28 146.91 ops/ms 6.31
> Double64Vector.SINH 17.62 88.59 ops/ms 5.03
> Double64Vector.TAN 21.00 86.43 ops/ms 4.12
> Double64Vector.TANH 23.75 111.35 ops/ms 4.69
> Float128Vector.ACOS 57.52 110.65 ops/ms 1.92
> Float128Vector.ASIN 57.15 117.95 ops/ms 2.06
> Float128Vector.ATAN 22.52 318.74 ops/ms 14.15
> Float128Vector.ATAN2 17.06 246.07 ops/ms 14.42
> 

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v9]

[Roger Riggs]

> JEP 415: Context-specific Deserialization Filters extends the deserialization 
> filtering mechanisms with more flexible and customizable protections against 
> malicious deserialization.  See JEP 415: https://openjdk.java.net/jeps/415.
> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
> extended with additional
> configuration mechanisms and filter utilities.
> 
> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
> `ObjectInputStream`:
> 
> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html

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

  Editorial updates
  Updated java.security properties to include jdk.serialFilterFactory
  Added test cases to SerialFilterFactoryTest for java.security properties and
  enabling of the SecurityManager with existing policy permission files
  Corrected a test that OIS.setObjectInputFilter could not be called twice.
  Removed a Factory test that was not intended to be committed

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3996/files
  - new: https://git.openjdk.java.net/jdk/pull/3996/files/9573ae11..b83da61a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3996=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3996=07-08

  Stats: 245 lines in 8 files changed: 77 ins; 112 del; 56 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3996.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3996/head:pull/3996

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v7]

[Roger Riggs]

On Tue, 25 May 2021 11:18:15 GMT, Chris Hegarty  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Move merge and rejectUndecidedClass methods to OIF.Config
>>   As default methods on OIF, their implementations were not concrete and not 
>> trustable
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 400:
> 
>> 398:  * {@link BinaryOperator {@literal 
>> BinaryOperator}} interface, provide its implementation and
>> 399:  * be accessible via the {@linkplain 
>> ClassLoader#getSystemClassLoader() application class loader}.
>> 400:  * The filter factory configured using the system or security 
>> property during initialization
> 
> What is the expected behaviour if the factory property is to set to a 
> non-class or non-accessible class? The current implementation does (it 
> probably should be more graceful) :
> 
> $ java -Djdk.serialFilterFactory=allow T
> Exception in thread "main" java.lang.ExceptionInInitializerError
>   at 
> java.base/java.io.ObjectInputFilter$Config.(ObjectInputFilter.java:537)
>   at 
> java.base/java.io.ObjectInputStream.(ObjectInputStream.java:394)
>   at T.main(T.java:5)
> Caused by: java.lang.ClassNotFoundException: allow
>   at 
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:636)
>   at 
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182)
>   at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519)
>   at java.base/java.lang.Class.forName0(Native Method)
>   at java.base/java.lang.Class.forName(Class.java:466)
>   at 
> java.base/java.io.ObjectInputFilter$Config.(ObjectInputFilter.java:519)
>   ... 2 more

If the factory class can not be found, the exception must be fatal; 
continuing to run without the filter would be a security risk.
ExceptionInInitializerError was the closest I could find.
I'll improve the message;  Oddly, ExceptionInInitializer does not allow both a 
message and initCause().
And the stacktrace for the ClassNotFoundException is not going to be very 
interesting.

-

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

Re: RFR: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics [v14]

[Sandhya Viswanathan]

> This PR contains Short Vector Math Library support related changes for 
> [JEP-414 Vector API (Second Incubator)](https://openjdk.java.net/jeps/414), 
> in preparation for when targeted.
> 
> Intel Short Vector Math Library (SVML) based intrinsics in native x86 
> assembly provide optimized implementation for Vector API transcendental and 
> trigonometric methods.
> These methods are built into a separate library instead of being part of 
> libjvm.so or jvm.dll.
> 
> The following changes are made:
>The source for these methods is placed in the jdk.incubator.vector module 
> under src/jdk.incubator.vector/linux/native/libsvml and 
> src/jdk.incubator.vector/windows/native/libsvml.
>The assembly source files are named as “*.S” and include files are named 
> as “*.S.inc”.
>The corresponding build script is placed at 
> make/modules/jdk.incubator.vector/Lib.gmk.
>Changes are made to build system to support dependency tracking for 
> assembly files with includes.
>The built native libraries (libsvml.so/svml.dll) are placed in bin 
> directory of JDK on Windows and lib directory of JDK on Linux.
>The C2 JIT uses the dll_load and dll_lookup to get the addresses of 
> optimized methods from this library.
> 
> Build system changes and module library build scripts are contributed by 
> Magnus (magnus.ihse.bur...@oracle.com).
> 
> Looking forward to your review and feedback.
> 
> Performance:
> Micro benchmark Base Optimized Unit Gain(Optimized/Base)
> Double128Vector.ACOS 45.91 87.34 ops/ms 1.90
> Double128Vector.ASIN 45.06 92.36 ops/ms 2.05
> Double128Vector.ATAN 19.92 118.36 ops/ms 5.94
> Double128Vector.ATAN2 15.24 88.17 ops/ms 5.79
> Double128Vector.CBRT 45.77 208.36 ops/ms 4.55
> Double128Vector.COS 49.94 245.89 ops/ms 4.92
> Double128Vector.COSH 26.91 126.00 ops/ms 4.68
> Double128Vector.EXP 71.64 379.65 ops/ms 5.30
> Double128Vector.EXPM1 35.95 150.37 ops/ms 4.18
> Double128Vector.HYPOT 50.67 174.10 ops/ms 3.44
> Double128Vector.LOG 61.95 279.84 ops/ms 4.52
> Double128Vector.LOG10 59.34 239.05 ops/ms 4.03
> Double128Vector.LOG1P 18.56 200.32 ops/ms 10.79
> Double128Vector.SIN 49.36 240.79 ops/ms 4.88
> Double128Vector.SINH 26.59 103.75 ops/ms 3.90
> Double128Vector.TAN 41.05 152.39 ops/ms 3.71
> Double128Vector.TANH 45.29 169.53 ops/ms 3.74
> Double256Vector.ACOS 54.21 106.39 ops/ms 1.96
> Double256Vector.ASIN 53.60 107.99 ops/ms 2.01
> Double256Vector.ATAN 21.53 189.11 ops/ms 8.78
> Double256Vector.ATAN2 16.67 140.76 ops/ms 8.44
> Double256Vector.CBRT 56.45 397.13 ops/ms 7.04
> Double256Vector.COS 58.26 389.77 ops/ms 6.69
> Double256Vector.COSH 29.44 151.11 ops/ms 5.13
> Double256Vector.EXP 86.67 564.68 ops/ms 6.52
> Double256Vector.EXPM1 41.96 201.28 ops/ms 4.80
> Double256Vector.HYPOT 66.18 305.74 ops/ms 4.62
> Double256Vector.LOG 71.52 394.90 ops/ms 5.52
> Double256Vector.LOG10 65.43 362.32 ops/ms 5.54
> Double256Vector.LOG1P 19.99 300.88 ops/ms 15.05
> Double256Vector.SIN 57.06 380.98 ops/ms 6.68
> Double256Vector.SINH 29.40 117.37 ops/ms 3.99
> Double256Vector.TAN 44.90 279.90 ops/ms 6.23
> Double256Vector.TANH 54.08 274.71 ops/ms 5.08
> Double512Vector.ACOS 55.65 687.54 ops/ms 12.35
> Double512Vector.ASIN 57.31 777.72 ops/ms 13.57
> Double512Vector.ATAN 21.42 729.21 ops/ms 34.04
> Double512Vector.ATAN2 16.37 414.33 ops/ms 25.32
> Double512Vector.CBRT 56.78 834.38 ops/ms 14.69
> Double512Vector.COS 59.88 837.04 ops/ms 13.98
> Double512Vector.COSH 30.34 172.76 ops/ms 5.70
> Double512Vector.EXP 99.66 1608.12 ops/ms 16.14
> Double512Vector.EXPM1 43.39 318.61 ops/ms 7.34
> Double512Vector.HYPOT 73.87 1502.72 ops/ms 20.34
> Double512Vector.LOG 74.84 996.00 ops/ms 13.31
> Double512Vector.LOG10 71.12 1046.52 ops/ms 14.72
> Double512Vector.LOG1P 19.75 776.87 ops/ms 39.34
> Double512Vector.POW 37.42 384.13 ops/ms 10.26
> Double512Vector.SIN 59.74 728.45 ops/ms 12.19
> Double512Vector.SINH 29.47 143.38 ops/ms 4.87
> Double512Vector.TAN 46.20 587.21 ops/ms 12.71
> Double512Vector.TANH 57.36 495.42 ops/ms 8.64
> Double64Vector.ACOS 24.04 73.67 ops/ms 3.06
> Double64Vector.ASIN 23.78 75.11 ops/ms 3.16
> Double64Vector.ATAN 14.14 62.81 ops/ms 4.44
> Double64Vector.ATAN2 10.38 44.43 ops/ms 4.28
> Double64Vector.CBRT 16.47 107.50 ops/ms 6.53
> Double64Vector.COS 23.42 152.01 ops/ms 6.49
> Double64Vector.COSH 17.34 113.34 ops/ms 6.54
> Double64Vector.EXP 27.08 203.53 ops/ms 7.52
> Double64Vector.EXPM1 18.77 96.73 ops/ms 5.15
> Double64Vector.HYPOT 18.54 103.62 ops/ms 5.59
> Double64Vector.LOG 26.75 142.63 ops/ms 5.33
> Double64Vector.LOG10 25.85 139.71 ops/ms 5.40
> Double64Vector.LOG1P 13.26 97.94 ops/ms 7.38
> Double64Vector.SIN 23.28 146.91 ops/ms 6.31
> Double64Vector.SINH 17.62 88.59 ops/ms 5.03
> Double64Vector.TAN 21.00 86.43 ops/ms 4.12
> Double64Vector.TANH 23.75 111.35 ops/ms 4.69
> Float128Vector.ACOS 57.52 110.65 ops/ms 1.92
> Float128Vector.ASIN 57.15 117.95 ops/ms 2.06
> Float128Vector.ATAN 22.52 318.74 ops/ms 14.15
> Float128Vector.ATAN2 17.06 246.07 ops/ms 14.42
> 

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Joe Darcy]

On Tue, 25 May 2021 14:57:22 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.io`, 
>> `java.math`, and `java.text` packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon 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:
> 
>  - 8267670: Updated code to use yield
>  - Merge remote-tracking branch 'origin/master' into JDK-8267670
>  - 8267670: Update java.io, java.math, and java.text to use switch expressions

Changes in java.math look fine.

-

Marked as reviewed by darcy (Reviewer).

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Brian Goetz]

On Tue, 25 May 2021 11:49:18 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More vertical alignment

src/java.base/share/classes/java/util/JapaneseImperialCalendar.java line 1124:

> 1122: return Math.max(LEAST_MAX_VALUES[YEAR], d.getYear());
> 1123: }
> 1124: }

A switch with one element here is kind of weird.  I would turn this into 
"return switch (field) { ... }", with two cases, YEAR and default.

src/java.base/share/classes/java/util/JapaneseImperialCalendar.java line 1371:

> 1369: }
> 1370: }
> 1371: }

Pull value assignment out of switch?

-

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

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v2]

[Raffaello Giulietti]

On Fri, 16 Apr 2021 11:30:32 GMT, Raffaello Giulietti 
 wrote:

>> Hello,
>> 
>> here's a PR for a patch submitted on March 2020 
>> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was 
>> a thing.
>> 
>> The patch has been edited to adhere to OpenJDK code conventions about 
>> multi-line (block) comments. Nothing in the code proper has changed, except 
>> for the addition of redundant but clarifying parentheses in some expressions.
>> 
>> 
>> Greetings
>> Raffaello
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   4511638: Double.toString(double) sometimes produces incorrect results

Hi,

a reminder to keep PR [1] alive and to invite interested reviewers to 
comment it.

The corresponding CSR is in [2]


Greetings
Raffaello

---

[1] https://github.com/openjdk/jdk/pull/3402
[2] https://bugs.openjdk.java.net/browse/JDK-8202555

-

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]

[Roger Riggs]

> Methods are added to java.lang.Process to read and write characters and lines 
> from and to a spawned Process.
> The Charset used to encode and decode characters to bytes can be specified or 
> use the
> operating system native encoding as is available from the "native.encoding" 
> system property.

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

  Described charset mapping of malformed chars in outputWriter
  Repeated calls to  inputReader, errorReader, and outputWriter now return the 
same instance
  and check for inconsistent charset argument
  Added warnings about concurrently use of input/output streams and 
readers/writers.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4134/files
  - new: https://git.openjdk.java.net/jdk/pull/4134/files/7724b82c..a5ed7b73

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

  Stats: 161 lines in 2 files changed: 131 ins; 0 del; 30 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4134.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4134/head:pull/4134

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

RFR: 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar

[Naoto Sato]

Please review the fix. The issue was informed yesterday by @amaembo that it 
offends some code analysis tools.
Although the fix is to change the condition error, it turned out that 
`JapaneseImperialCalendar.roll()` did not work for `WEEK_OF_MONTH` and 
`DAY_OF_MONTH`. There was an inherent issue where `GregorianCalendar` does not 
follow the `roll()` spec, i.e., "The MONTH must not change when the 
WEEK_OF_MONTH is rolled." during the Julian/Gregorian transition. JDK-6191841 
seems to have tried to fix it but it was closed as `Future Project`...
So I simply fixed the `roll()` issue in `JapaneseImperialCalendar` to follow 
the spec here, which seems to be the intent of the original author.

-

Commit messages:
 - 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar

Changes: https://git.openjdk.java.net/jdk/pull/4191/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4191=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8187649
  Stats: 46 lines in 3 files changed: 36 ins; 0 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4191.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4191/head:pull/4191

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Brian Goetz]

On Tue, 25 May 2021 11:49:18 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More vertical alignment

src/java.base/share/classes/java/util/Calendar.java line 1507:

> 1505: }
> 1506: case "japanese" -> cal = new 
> JapaneseImperialCalendar(zone, locale, true);
> 1507: default -> throw new IllegalArgumentException("unknown 
> calendar type: " + type);

Agree with Chris' suggestion here.

-

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

Reminder RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

Hi,

a reminder to keep PR [1] alive and to invite interested reviewers to 
comment it.


The corresponding CSR is in [2]


Greetings
Raffaello

---

[1] https://github.com/openjdk/jdk/pull/3402
[2] https://bugs.openjdk.java.net/browse/JDK-8202555

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions

[Brian Goetz]

In the last hunk, you convert

case Collator.IDENTICAL: toAddTo.append('='); break;
case Collator.TERTIARY:  toAddTo.append(','); break;
case Collator.SECONDARY: toAddTo.append(';'); break;
case Collator.PRIMARY:   toAddTo.append('<'); break;
case RESET: toAddTo.append('&'); break;
case UNSET: toAddTo.append('?'); break;


to

case Collator.IDENTICAL -> toAddTo.append('=');
case Collator.TERTIARY  -> toAddTo.append(',');
case Collator.SECONDARY -> toAddTo.append(';');
case Collator.PRIMARY   -> toAddTo.append('<');
case RESET  -> toAddTo.append('&');
case UNSET  -> toAddTo.append('?');

But, you can go further, pulling the toAddTo.append() call out of the switch.  
This was one of the benefits we anticipated with expression switches; that it 
would expose more opportunities to push the conditional logic farther down 
towards the leaves.  I suspect there are other opportunities for this in this 
patch too.

> On May 25, 2021, at 7:57 AM, Patrick Concannon  
> wrote:
> 
> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick
> 
> -
> 
> Commit messages:
> - 8267670: Update java.io, java.math, and java.text to use switch expressions
> 
> Changes: https://git.openjdk.java.net/jdk/pull/4182/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4182=00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8267670
>  Stats: 328 lines in 11 files changed: 1 ins; 187 del; 140 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/4182.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4182/head:pull/4182
> 
> PR: https://git.openjdk.java.net/jdk/pull/4182

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Jan Lahoda]

On Tue, 25 May 2021 16:00:43 GMT, Rémi Forax  wrote:

> > The reason for this integer (which is not a constant in the case of this 
> > switch) is to restart the matching in case guards fail to "match". 
> > Considering the example here:
> > ```
> > class Example {
> > void example(Object o) {
> > switch (o) {
> > case String s && s.length() == 0 ->
> > System.out.println("1st case");
> > case String s && s.length() == 1 ->  // line 6
> > System.out.println("2nd case");  // line 7
> > case String s -> // line 8
> > System.out.println("3rd case");  // line 9
> > default ->   // line 10
> > System.out.println("default case");  // line 11
> > }
> > }
> > }
> > ```
> > 
> > 
> > 
> >   
> > 
> > 
> >   
> > 
> > 
> > 
> >   
> > If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`, 
> > returning `0`. Then the guard will be evaluated `s.length() == 0`. If the 
> > length is not zero, the local variable 3 will be reassigned to `1`(bytecode 
> > index 58, 59) and the whole switch is restarted - just this time, the 
> > matching in the indy will start at index `1`, not `0`, i.e. `indy[...](o, 
> > 1)`. And so on. I believe there is a text explaining the meaning of the 
> > parameter in the javadoc of the bootstrap, and in TransPatterns in javac.
> 
> The problem with this design is that calling example("foo") forces the VM 
> will do 6 checkcast String on "foo", and it doesn't work with sub-patterns. 
> Desugaring the guards as static method like with lambdas seems more 
> promising, indy can be called with the pairs [String, MethodHandle(s -> 
> s.length() == 0)], [String, MethodHandle(s -> s.length() == 0)] and [_,_] 
> (with the guards passed as a constant method handles again like lambdas are 
> desugared).
> It means that the indy needs to capture all local variables that are used in 
> the guards, instead of passing an int.
> 
> I believe that a translation using constant method handles for guards is far 
> more efficient than using an int and a backward branch but it has a higher 
> cost in term of runtime data structures.

I'd like to note this is a preview feature - we can change the desugaring. At 
the same time, I don't think this does not work with sub-patterns (those can be 
easily desugared to guards, I think). Regarding efficiency, it may be a balance 
between classfile overhead (which will be higher if we need to desugar every 
guard to a separate method), and the possibility to tweak the matching at 
runtime.

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Rémi Forax]

On Tue, 25 May 2021 14:22:57 GMT, Jan Lahoda  wrote:

> The reason for this integer (which is not a constant in the case of this 
> switch) is to restart the matching in case guards fail to "match". 
> Considering the example here:
> 
> ```
> class Example {
> void example(Object o) {
> switch (o) {
> case String s && s.length() == 0 ->
> System.out.println("1st case");
> case String s && s.length() == 1 ->  // line 6
> System.out.println("2nd case");  // line 7
> case String s -> // line 8
> System.out.println("3rd case");  // line 9
> default ->   // line 10
> System.out.println("default case");  // line 11
> }
> }
> }
> ```
> 
> If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`, 
> returning `0`. Then the guard will be evaluated `s.length() == 0`. If the 
> length is not zero, the local variable 3 will be reassigned to `1`(bytecode 
> index 58, 59) and the whole switch is restarted - just this time, the 
> matching in the indy will start at index `1`, not `0`, i.e. `indy[...](o, 
> 1)`. And so on. I believe there is a text explaining the meaning of the 
> parameter in the javadoc of the bootstrap, and in TransPatterns in javac.

The problem with this design is that calling example("foo") forces the VM will 
do 6 checkcast String on "foo", and it doesn't work with sub-patterns. 
Desugaring the guards as static method like with lambdas seems more promising, 
indy can be called with the pairs [String, MethodHandle(s -> s.length() == 0)], 
[String, MethodHandle(s -> s.length() == 0)] and [_,_]  (with the guards passed 
as a constant method handles again like lambdas are desugared).
It means that the indy needs to capture all local variables that are used in 
the guards, instead of passing an int.

I believe that a translation using constant method handles for guards is far 
more efficient than using an int and a backward branch but it has a higher cost 
in term of runtime data structures.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Tue, 25 May 2021 09:14:38 GMT, Chris Hegarty  wrote:

>> The spec/code is forthcoming.  
>> ii) is sufficient to prevent ambiguity in which filter is used throughout 
>> the Java runtime; 
>>though it requires a bit of package-private plumbing.
>> 
>> i) is too limiting.  It should be possible for an application to check 
>> whether a filter factory has been provided on the command line (by calling 
>> getSerialFilterFactory) and if not setting the factory itself.  It may also 
>> want to install its own filter factory that delegates to the builtin factory 
>> without needed to re-implement the builtin behavior.
>
>> i) is too limiting. It should be possible for an application to check 
>> whether a filter factory has been provided on the command line (by calling 
>> getSerialFilterFactory) and if not setting the factory itself. It may also 
>> want to install its own filter factory that delegates to the builtin factory 
>> without needed to re-implement the builtin behavior.
> 
> How is this supposed to work in practice?  getSerialFilterFactory always 
> returns a non-null factory, so how does one know whether or not the returned 
> factory is the built-in factory, a factory set by the command line (or 
> security property) ? (without resorting to implementation assumptions)

If the app does not recognize the filter factory (by its class or module) then 
try to replace it.
If it succeeds, the app is good to go.
If it throws an exception then your app is not likely to run and should fail.
Newer APIs generally make it possible to detect the state of things without 
throwing but in this case it would add extra API surface.

Some alternatives,  check the module of the filter returned, if it is java.base 
then replacing it is ok to replace. (yes an implementation assumption).
An alternative would be to add a static method to return the builtin filter;  
creating it if has not already been created.

-

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

Re: RFR: 8267403: tools/jpackage/share/FileAssociationsTest.java#id0 failed with "Error: Bundler "Mac PKG Package" (pkg) failed to produce a package"

[Alexey Semenyuk]

On Fri, 21 May 2021 04:34:22 GMT, Alexander Matveev  
wrote:

> Looks like another issue similar to hdiutil (JDK-8249395) when process is 
> gone, but we still waiting for it. I was not able to reproduce this issue by 
> running test or pkgbuild separately and conclusion was made based on logs. 
> Fixed in same way as hdiutil issue. Also, I added logging PID for external 
> commands to simplify log analysis. Log will have multiple hdiutil and/or 
> pkgbuild processes, since we running multiple tests in parallel and matching 
> external process to test failure requires looking at command line for 
> particular process, so PID should simplify this.

Marked as reviewed by asemenyuk (Reviewer).

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Chris Hegarty]

On Tue, 25 May 2021 11:49:18 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More vertical alignment

src/java.base/share/classes/java/util/Calendar.java line 1507:

> 1505: }
> 1506: case "japanese" -> cal = new 
> JapaneseImperialCalendar(zone, locale, true);
> 1507: default -> throw new IllegalArgumentException("unknown 
> calendar type: " + type);

In this case, it would be good to yield the result of the switch expression and 
assign that to a local, rather than assigning in each of the case arms, e.g:

final Calendar cal = switch (type) {
case "gregory" -> new GregorianCalendar(zone, locale, true);
case "iso8601" -> {
GregorianCalendar gcal = new GregorianCalendar(zone, locale, true);
// make gcal a proleptic Gregorian
gcal.setGregorianChange(new Date(Long.MIN_VALUE));
// and week definition to be compatible with ISO 8601
setWeekDefinition(MONDAY, 4);
yield gcal;
}
case "buddhist" -> {
var buddhistCalendar = new BuddhistCalendar(zone, locale);
buddhistCalendar.clear();
yield buddhistCalendar;
}
case "japanese" -> new JapaneseImperialCalendar(zone, locale, true);
default -> throw new IllegalArgumentException("unknown calendar type: " 
+ type);
};

-

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

Re: RFR: 8267452: Delegate forEachRemaining in Spliterators.iterator() [v2]

[Paul Sandoz]

On Mon, 24 May 2021 07:15:34 GMT, Tagir F. Valeev  wrote:

>> Sometimes, `Spliterator::forEachRemaining` has much more optimized 
>> implementation, compared to `Spliterator::tryAdvance`. For example, if a 
>> `Stream::spliterator` called for a stream that has intermediate operations, 
>> `tryAdvance()` may buffer intermediate elements while `forEachRemaining()` 
>> just delegates to the `wrapAndCopyInto` (see 
>> `StreamSpliterators.AbstractWrappingSpliterator` and its inheritors).
>> 
>> `Spliterators::iterator` methods (used in particular by `Stream::iterator`) 
>> provide adapter iterators that delegate to the supplied spliterator. 
>> Unfortunately, they don't have a specialized implementation for 
>> `Iterator::forEachRemaining`. As a result, a default implementation is used 
>> that delegates to `hasNext`/`next`, which in turn causes the delegation to 
>> `tryAdvance`. It's quite simple to implement `Iterator::forEachRemaining` 
>> here, taking advantage of possible spliterator optimizations if the iterator 
>> client decides to use `forEachRemaining`.
>> 
>> This patch implements Iterator::forEachRemaining in Spliterators::iterator 
>> methods. Also, I nullize the `nextElement` in `Iterator::next` to avoid 
>> accidental prolongation of the user's object lifetime when iteration is 
>> finished. Finally, a small cleanup: I added the `final` modifier where 
>> possible to private fields in `Spliterators`.
>> 
>> Test-wise, some scenarios are already covered by 
>> SpliteratorTraversingAndSplittingTest. However, the resulting iterator is 
>> always wrapped into `Spliterators::spliterator`, so usage scenarios are 
>> somewhat limited. In particular, calling `hasNext` (without `next`) before 
>> `forEachRemaining` was not covered there. I added more tests in 
>> `IteratorFromSpliteratorTest` to cover these scenarios. I checked that 
>> removing `valueReady = false;` or `action.accept(t);` lines from newly 
>> implemented `forEachRemaining` method causes new tests to fail (but old 
>> tests don't fail due to this).
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Test: formatting; tests for empty spliterator

Oops i missed the bit about testing in the description. That's subtle, the 
`hasNext` occurs in the assert before the `forEachRemaining`. Could you add a 
comment that this is the primary purpose of the test since 
`SpliteratorTraversingAndSplittingTest` cannot do that for a spliterator 
wrapping an iterator?

-

Marked as reviewed by psandoz (Reviewer).

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v8]

[Roger Riggs]

> JEP 415: Context-specific Deserialization Filters extends the deserialization 
> filtering mechanisms with more flexible and customizable protections against 
> malicious deserialization.  See JEP 415: https://openjdk.java.net/jeps/415.
> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
> extended with additional
> configuration mechanisms and filter utilities.
> 
> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
> `ObjectInputStream`:
> 
> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html

Roger Riggs has updated the pull request incrementally with two additional 
commits since the last revision:

 - Moved utility filter methods to be static on ObjectInputFilter
   Rearranged the class javadoc of OIF to describe the parts of
   deserialization filtering, filters, composite filters, and the filter 
factory.
   And other review comment updates...
 - Refactored tests for utility functions to SerialFilterFunctionTest.java
   Deleted confused Config.allowMaxLimits() method
   Updated example to match move of methods to Config
   Added test of restriction on setting the filterfactory after a OIS has been 
created
   Additional Editorial updates

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3996/files
  - new: https://git.openjdk.java.net/jdk/pull/3996/files/141bf720..9573ae11

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3996=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3996=06-07

  Stats: 1040 lines in 7 files changed: 533 ins; 397 del; 110 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3996.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3996/head:pull/3996

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

Re: RFR: 8265768 [aarch64] Use glibc libm impl for dlog, dlog10, dexp iff 2.29 or greater on AArch64.

[gregcawthorne]

On Thu, 15 Apr 2021 08:33:47 GMT, gregcawthorne 
 wrote:

> Glibc 2.29 onwards provides optimised versions of log,log10,exp.
> These functions have an accuracy of 0.9ulp or better in glibc
> 2.29.
> 
> Therefore this patch adds code to parse, store and check
> the runtime glibcs version in os_linux.cpp/hpp.
> This is then used to select the glibcs implementation of
> log, log10, exp at runtime for c1 and c2, iff we have
> glibc 2.29 or greater.
> 
> This will ensure OpenJDK can benefit from future improvements
> to glibc.
> 
> Glibc adheres to the ieee754 standard, unless stated otherwise
> in its spec.
> 
> As there are no stated exceptions in the current glibc spec
> for dlog, dlog10 and dexp, we can assume they currently follow
> ieee754 (which testing confirms). As such, future version of
> glibc are unlikely to lose this compliance with ieee754 in
> future.
> 
> W.r.t performance this patch sees ~15-30% performance improvements for
> log and log10, with ~50-80% performance improvements for exp for the
> common input ranged (which output real numbers). However for the NaN
> and inf output ranges we see a slow down of up to a factor of 2 for
> some functions and architectures.
> 
> Due to this being the uncommon case we assert that this is a
> worthwhile tradeoff.

greg.cawtho...@arm.com

Should work

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Chris Hegarty]

On Tue, 25 May 2021 14:53:44 GMT, Patrick Concannon  
wrote:

>> src/java.base/share/classes/java/io/StreamTokenizer.java line 795:
>> 
>>> 793:  * case statements
>>> 794:  */
>>> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>> 
>> Maybe (since its easier to grok the yield rather than the assignment of ret 
>> in branches):
>> 
>> String ret = switch (ttype) {
>> case TT_EOF -> "EOF";
>> case TT_EOL -> "EOL";
>> case TT_WORD-> sval;
>> case TT_NUMBER  -> "n=" + nval;
>> case TT_NOTHING -> "NOTHING";
>> default -> {
>> /*
>>  * ttype is the first character of either a quoted string or
>>  * is an ordinary character. ttype can definitely not be less
>>  * than 0, since those are reserved values used in the 
>> previous
>>  * case statements
>>  */
>> if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>> yield sval;
>> }
>> char s[] = new char[3];
>> s[0] = s[2] = ''';
>> s[1] = (char) ttype;
>> yield new String(s);
>> }
>> };
>> return "Token[" + ret + "], line " + LINENO;
>
> Code updated as suggested. See adc8af4

The snippet above both uses yield in the default case, and also removes the 
assignments from the other arms. adc8af4 overlooks the redundant assignments in 
the non-default cases.

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Chris Hegarty]

On Tue, 25 May 2021 14:57:22 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.io`, 
>> `java.math`, and `java.text` packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon 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:
> 
>  - 8267670: Updated code to use yield
>  - Merge remote-tracking branch 'origin/master' into JDK-8267670
>  - 8267670: Update java.io, java.math, and java.text to use switch expressions

src/java.base/share/classes/java/io/ObjectStreamClass.java line 2172:

> 2170: switch (typeCodes[i]) {
> 2171: case 'L', '[' -> vals[offsets[i]] = 
> unsafe.getReference(obj, readKeys[i]);
> 2172: default   -> throw new InternalError();

suggest:

vals[offsets[i]] = switch (typeCodes[i]) {
case 'L', '[' -> unsafe.getReference(obj, readKeys[i]);
default   -> throw new InternalError();

src/java.base/share/classes/java/io/StreamTokenizer.java line 787:

> 785: case TT_WORD-> ret = sval;
> 786: case TT_NUMBER  -> ret = "n=" + nval;
> 787: case TT_NOTHING -> ret = "NOTHING";

This is not correct, the assignments to ret in these case arms is redundant.

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Patrick Concannon]

On Tue, 25 May 2021 12:43:00 GMT, Naoto Sato  wrote:

>> Patrick Concannon 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:
>> 
>>  - 8267670: Updated code to use yield
>>  - Merge remote-tracking branch 'origin/master' into JDK-8267670
>>  - 8267670: Update java.io, java.math, and java.text to use switch 
>> expressions
>
> src/java.base/share/classes/java/text/BreakIterator.java line 569:
> 
>> 567: case SENTENCE_INDEX  -> 
>> breakIteratorProvider.getSentenceInstance(locale);
>> 568: default  -> null;
>> 569: };
> 
> No need to use the local variable `iterator` here. Simply return with the 
> switch expression.

Hi Naoto, thanks for spotting this. Code updated as suggested. See adc8af4

> src/java.base/share/classes/java/text/NumberFormat.java line 982:
> 
>> 980: case COMPACTSTYLE  -> 
>> provider.getCompactNumberInstance(locale, formatStyle);
>> 981: default-> null;
>> 982: };
> 
> Same as `BreakIterator`. No need for `numberFormat`.

Code updated as suggested. See adc8af4

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Patrick Concannon]

On Tue, 25 May 2021 12:31:38 GMT, Chris Hegarty  wrote:

>> Patrick Concannon 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:
>> 
>>  - 8267670: Updated code to use yield
>>  - Merge remote-tracking branch 'origin/master' into JDK-8267670
>>  - 8267670: Update java.io, java.math, and java.text to use switch 
>> expressions
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 1877:
> 
>> 1875: descriptor.checkInitialized();
>> 1876: }
>> 1877: default -> throw new 
>> StreamCorruptedException(
> 
> What would you think of assigning descriptor with the value returned from 
> evaluating the switch?
> 
> Either:
> 
> 1) 
> 
> ObjectStreamClass descriptor = switch (tc) {
> case TC_NULL-> (ObjectStreamClass) readNull();
> case TC_PROXYCLASSDESC  -> readProxyDesc(unshared);
> case TC_CLASSDESC   -> readNonProxyDesc(unshared);
> case TC_REFERENCE   -> readAndCheckHandle(unshared);
> default -> throw new StreamCorruptedException(String.format("invalid 
> type code: %02X", tc));
> };
> return descriptor;
> }
> 
> , where the body of TC_REFERENCE is enclosed in readAndCheckHandle,   OR
> 
> 2) Simply 
> 
>   case TC_REFERENCE   -> {
> var d = (ObjectStreamClass)readHandle(unshared);
> // Should only reference initialized class descriptors
> d.checkInitialized();
> yield d; }

Code updated as suggested. See adc8af4

> src/java.base/share/classes/java/io/StreamTokenizer.java line 795:
> 
>> 793:  * case statements
>> 794:  */
>> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
> 
> Maybe (since its easier to grok the yield rather than the assignment of ret 
> in branches):
> 
> String ret = switch (ttype) {
> case TT_EOF -> "EOF";
> case TT_EOL -> "EOL";
> case TT_WORD-> sval;
> case TT_NUMBER  -> "n=" + nval;
> case TT_NOTHING -> "NOTHING";
> default -> {
> /*
>  * ttype is the first character of either a quoted string or
>  * is an ordinary character. ttype can definitely not be less
>  * than 0, since those are reserved values used in the 
> previous
>  * case statements
>  */
> if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
> yield sval;
> }
> char s[] = new char[3];
> s[0] = s[2] = ''';
> s[1] = (char) ttype;
> yield new String(s);
> }
> };
> return "Token[" + ret + "], line " + LINENO;

Code updated as suggested. See adc8af4

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Patrick Concannon]

On Tue, 25 May 2021 13:16:00 GMT, Daniel Fuchs  wrote:

>> Patrick Concannon 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:
>> 
>>  - 8267670: Updated code to use yield
>>  - Merge remote-tracking branch 'origin/master' into JDK-8267670
>>  - 8267670: Update java.io, java.math, and java.text to use switch 
>> expressions
>
> src/java.base/share/classes/java/io/ObjectStreamField.java line 123:
> 
>> 121: case 'D'  -> type = Double.TYPE;
>> 122: case 'L', '[' -> type = Object.class;
>> 123: default   -> throw new 
>> IllegalArgumentException("illegal signature");
> 
> Why not assign type here?
> 
> 
> type = switch(signature.charAt(0)) {
> case 'Z'  -> Boolean.TYPE;
> 

Thanks for your suggestion. I've done that now. See adc8af4

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]

[Patrick Concannon]

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

Patrick Concannon 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:

 - 8267670: Updated code to use yield
 - Merge remote-tracking branch 'origin/master' into JDK-8267670
 - 8267670: Update java.io, java.math, and java.text to use switch expressions

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4182/files
  - new: https://git.openjdk.java.net/jdk/pull/4182/files/b98e47db..adc8af41

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

  Stats: 143 lines in 16 files changed: 28 ins; 56 del; 59 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4182.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4182/head:pull/4182

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

Re: RFR: 8265768 [aarch64] Use glibc libm impl for dlog, dlog10, dexp iff 2.29 or greater on AArch64.

[Andrew Haley]

Greg, what's your email address? Everything I try bounces.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Evgeny Mandrikov]

On Tue, 25 May 2021 14:13:55 GMT, Rémi Forax  wrote:

> 7: iconst_0 < this zero

@forax as far as I understood this will be a value of
parameter `startIndex` in `java.lang.runtime. SwitchBootstraps.doSwitch`  
Please correct me @lahodaj if I'm wrong.

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Jan Lahoda]

On Tue, 25 May 2021 14:13:55 GMT, Rémi Forax  wrote:

> > Thanks Evgeny, I'll take a look.
> > @forax, do you mean why there is "0" in:
> > 11: invokedynamic #13, 0
> > ?
> 
> Not this one, the one on the stack.
> 
> 7: iconst_0 < this zero
> 8: istore_3
> 9: aload_2
> 10: iload_3
> 11: invokedynamic #13, 0 // InvokeDynamic
> #0:typeSwitch:(Ljava/lang/Object;I)I
> 
> Why the descriptor is (Ljava/lang/Object;I)I instead of (Ljava/lang/Object;)I,
> what is the semantics associated to that integer ?

The reason for this integer (which is not a constant in the case of this 
switch) is to restart the matching in case guards fail to "match". Considering 
the example here:

class Example {
void example(Object o) {
switch (o) {
case String s && s.length() == 0 ->
System.out.println("1st case");
case String s && s.length() == 1 ->  // line 6
System.out.println("2nd case");  // line 7
case String s -> // line 8
System.out.println("3rd case");  // line 9
default ->   // line 10
System.out.println("default case");  // line 11
}
}
}


If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`, 
returning `0`. Then the guard will be evaluated `s.length() == 0`. If the 
length is not zero, the local variable 3 will be reassigned to `1`(bytecode 
index 58, 59) and the whole switch is restarted - just this time, the matching 
in the indy will start at index `1`, not `0`, i.e. `indy[...](o, 1)`. And so 
on. I believe there is a text explaining the meaning of the parameter in the 
javadoc of the bootstrap, and in TransPatterns in javac.

> 
> > The "0" is an artifact of how invokedynamic is represented in the classfile 
> > (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap 
> > shows the value of the first zero byte. That is probably not needed 
> > anymore, but there is nothing special in this patch, I think - all 
> > invokedynamic calls look like this, AFAIK.
> 
> I know that a little to well, i'm one of the guys behind the design of indy :)

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Rémi Forax]

On Tue, 25 May 2021 12:20:02 GMT, Jan Lahoda  wrote:

> Thanks Evgeny, I'll take a look.
> 
> @forax, do you mean why there is "0" in:
> 11: invokedynamic #13, 0
> ?

Not this one, the one on the stack.

7: iconst_0   < this zero
8: istore_3
9: aload_2
10: iload_3
11: invokedynamic #13, 0 // InvokeDynamic
#0:typeSwitch:(Ljava/lang/Object;I)I

Why the descriptor is (Ljava/lang/Object;I)I instead of (Ljava/lang/Object;)I,
what is the semantics associated to that integer ?

> The "0" is an artifact of how invokedynamic is represented in the classfile 
> (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap 
> shows the value of the first zero byte. That is probably not needed anymore, 
> but there is nothing special in this patch, I think - all invokedynamic calls 
> look like this, AFAIK.

I know that a little to well, i'm one of the guys behind the design of indy :)

-

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

Re: RFR: 8263561: Re-examine uses of LinkedList [v4]

[Сергей Цыпанов]

> The usage of `LinkedList` is senseless and can be replaced with either 
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
> 
> jdk:tier1 and jdk:tier2 are both ok

Сергей Цыпанов 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 five additional 
commits since the last revision:

 - Merge branch 'master' into purge-linked-list
 - 8263561: Use sized constructor where reasonable
 - 8263561: Use interface List instead of particular type where possible
 - 8263561: Rename requestList -> requests
 - 8263561: Re-examine uses of LinkedList

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2744/files
  - new: https://git.openjdk.java.net/jdk/pull/2744/files/40910fae..89160b3e

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

  Stats: 763526 lines in 11131 files changed: 166175 ins; 560683 del; 36668 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2744.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2744/head:pull/2744

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions

[Daniel Fuchs]

On Tue, 25 May 2021 09:37:58 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/io/ObjectStreamField.java line 123:

> 121: case 'D'  -> type = Double.TYPE;
> 122: case 'L', '[' -> type = Object.class;
> 123: default   -> throw new IllegalArgumentException("illegal 
> signature");

Why not assign type here?


type = switch(signature.charAt(0)) {
case 'Z'  -> Boolean.TYPE;


-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions

[Chris Hegarty]

On Tue, 25 May 2021 09:37:58 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/io/StreamTokenizer.java line 795:

> 793:  * case statements
> 794:  */
> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {

Maybe (since its easier to grok the yield rather than the assignment of ret in 
branches):

String ret = switch (ttype) {
case TT_EOF -> "EOF";
case TT_EOL -> "EOL";
case TT_WORD-> sval;
case TT_NUMBER  -> "n=" + nval;
case TT_NOTHING -> "NOTHING";
default -> {
/*
 * ttype is the first character of either a quoted string or
 * is an ordinary character. ttype can definitely not be less
 * than 0, since those are reserved values used in the previous
 * case statements
 */
if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
yield sval;
}
char s[] = new char[3];
s[0] = s[2] = ''';
s[1] = (char) ttype;
yield new String(s);
}
};
return "Token[" + ret + "], line " + LINENO;

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions

[Naoto Sato]

On Tue, 25 May 2021 09:37:58 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/text/BreakIterator.java line 569:

> 567: case SENTENCE_INDEX  -> 
> breakIteratorProvider.getSentenceInstance(locale);
> 568: default  -> null;
> 569: };

No need to use the local variable `iterator` here. Simply return with the 
switch expression.

src/java.base/share/classes/java/text/NumberFormat.java line 982:

> 980: case COMPACTSTYLE  -> 
> provider.getCompactNumberInstance(locale, formatStyle);
> 981: default-> null;
> 982: };

Same as `BreakIterator`. No need for `numberFormat`.

-

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

Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions

[Chris Hegarty]

On Tue, 25 May 2021 09:37:58 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/io/ObjectInputStream.java line 1877:

> 1875: descriptor.checkInitialized();
> 1876: }
> 1877: default -> throw new 
> StreamCorruptedException(

What would you think of assigning descriptor with the value returned from 
evaluating the switch?

Either:

1) 

ObjectStreamClass descriptor = switch (tc) {
case TC_NULL-> (ObjectStreamClass) readNull();
case TC_PROXYCLASSDESC  -> readProxyDesc(unshared);
case TC_CLASSDESC   -> readNonProxyDesc(unshared);
case TC_REFERENCE   -> readAndCheckHandle(unshared);
default -> throw new StreamCorruptedException(String.format("invalid 
type code: %02X", tc));
};
return descriptor;
}

, where the body of TC_REFERENCE is enclosed in readAndCheckHandle,   OR

2) Simply 

  case TC_REFERENCE   -> {
var d = (ObjectStreamClass)readHandle(unshared);
// Should only reference initialized class descriptors
d.checkInitialized();
yield d; }

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Jan Lahoda]

On Wed, 19 May 2021 08:00:12 GMT, Jan Lahoda  wrote:

>> This is a preview of a patch implementing JEP 406: Pattern Matching for 
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>> 
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>> 
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common 
>> superinterface for expressions and patterns, `CaseLabelTree`, which 
>> expressions and patterns implement, and a list of case labels is returned 
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree` 
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent 
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be 
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary 
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed 
>> primitives, as there is no value that is not part of the input domain so 
>> that could be used to represent `case null`. Requires a bit shuffling with 
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It 
>> produces code that delegates to a new bootstrap method, that will classify 
>> the input value to the switch and return the case number, to which the 
>> switch then jumps. To support guards, the switches (and the bootstrap 
>> method) are restartable. The bootstrap method as such is written very simply 
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String 
>> s`/`case null: case String s:`/`case String s: case null:`, handling of 
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>> 
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixing various error-related bugs.

Thanks Evgeny, I'll take a look.

@forax, do you mean why there is "0" in:
11: invokedynamic #13, 0
?

The "0" is an artifact of how invokedynamic is represented in the classfile 
(invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap 
shows the value of the first zero byte. That is probably not needed anymore, 
but there is nothing special in this patch, I think - all invokedynamic calls 
look like this, AFAIK.

-

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

Re: RFR: 8267403: tools/jpackage/share/FileAssociationsTest.java#id0 failed with "Error: Bundler "Mac PKG Package" (pkg) failed to produce a package"

[Andy Herrick]

On Fri, 21 May 2021 04:34:22 GMT, Alexander Matveev  
wrote:

> Looks like another issue similar to hdiutil (JDK-8249395) when process is 
> gone, but we still waiting for it. I was not able to reproduce this issue by 
> running test or pkgbuild separately and conclusion was made based on logs. 
> Fixed in same way as hdiutil issue. Also, I added logging PID for external 
> commands to simplify log analysis. Log will have multiple hdiutil and/or 
> pkgbuild processes, since we running multiple tests in parallel and matching 
> external process to test failure requires looking at command line for 
> particular process, so PID should simplify this.

Please update the bug report to contain the evaluation you have here in the PR

-

Marked as reviewed by herrick (Reviewer).

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

Re: RFR: 8263561: Re-examine uses of LinkedList [v3]

[Сергей Цыпанов]

> The usage of `LinkedList` is senseless and can be replaced with either 
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
> 
> jdk:tier1 and jdk:tier2 are both ok

Сергей Цыпанов has updated the pull request incrementally with one additional 
commit since the last revision:

  8263561: Use sized constructor where reasonable

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2744/files
  - new: https://git.openjdk.java.net/jdk/pull/2744/files/158006c6..40910fae

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

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

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

RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions

[Patrick Concannon]

Hi,

Could someone please review my code for updating the code in the `java.io`, 
`java.math`, and `java.text` packages to make use of the switch expressions?

Kind regards,
Patrick

-

Commit messages:
 - 8267670: Update java.io, java.math, and java.text to use switch expressions

Changes: https://git.openjdk.java.net/jdk/pull/4182/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4182=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267670
  Stats: 328 lines in 11 files changed: 1 ins; 187 del; 140 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4182.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4182/head:pull/4182

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

Integrated: 8267612; Declare package-private VarHandle.AccessMode/AccessType counts

[Claes Redestad]

On Mon, 24 May 2021 11:26:51 GMT, Claes Redestad  wrote:

> Slightly reduce VarHandle startup overhead by introducing package-private 
> COUNT constants for two enums

This pull request has now been integrated.

Changeset: 66b190e1
Author:Claes Redestad 
URL:   
https://git.openjdk.java.net/jdk/commit/66b190e1e7d06f3fc59917b5346e94a128e928cd
Stats: 18 lines in 4 files changed: 8 ins; 2 del; 8 mod

8267612: Declare package-private VarHandle.AccessMode/AccessType counts

Reviewed-by: mchung

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Tagir F . Valeev]

> Inspired by PR#4088. Most of the changes are done automatically using 
> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
> including indentations, moving comments, extracting common cast out of switch 
> expression branches, etc.
> 
> I also noticed that there are some switches having one branch only in 
> JapaneseImperialCalendar.java. Probably it would be better to replace them 
> with `if` statement?

Tagir F. Valeev has updated the pull request incrementally with one additional 
commit since the last revision:

  More vertical alignment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4161/files
  - new: https://git.openjdk.java.net/jdk/pull/4161/files/82f40605..f4ad5f14

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

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

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Remi Forax]

- Mail original -
> De: "Evgeny Mandrikov" 
> À: "build-dev" , "compiler-dev" 
> , "core-libs-dev"
> , "javadoc-dev" 
> Envoyé: Mardi 25 Mai 2021 11:32:03
> Objet: Re: RFR: 8262891: Compiler implementation for Pattern Matching for 
> switch (Preview) [v4]

> On Mon, 17 May 2021 19:01:01 GMT, Jan Lahoda  wrote:
> 
>>> Good work. There's a lot to take in here. I think overall, it holds up well 
>>> - I
>>> like how case labels have been extended to accommodate for patterns. In the
>>> front-end, I think there are some questions over the split between Attr and
>>> Flow - maybe it is unavoidable, but I'm not sure why some control-flow 
>>> analysis
>>> happens in Attr instead of Flow and I left some questions to make sure I
>>> understand what's happening.
>>> 
>>> In the backend it's mostly good work - but overall the structure of the 
>>> code,
>>> both in Lower and in TransPattern is getting very difficult to follow, given
>>> there are many different kind of switches each with its own little twist
>>> attached to it. It would be nice, I think (maybe in a separate cleanup?) if 
>>> the
>>> code paths serving the different switch kinds could be made more separate, 
>>> so
>>> that, when reading the code we can worry only about one possible code shape.
>>> That would make the code easier to document as well.
>>
>> @mcimadamore, @forax, thanks a lot for comments! I tried to apply the 
>> requested
>> changes in recent commits
>> (https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b
>> https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99
>> ).
>> 
>> I've also tried to update the implementation for the latest spec changes 
>> here:
>> https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
>> 
>> The spec changes are: total patterns are nullable; pattern matching ("new")
>> statement switches must be complete (as switch expressions).
>> 
>> Any further feedback is welcome!
> 
> Hi @lahodaj   ,
> 
> I tried `javac` built from this PR and for the following `Example.java`
> 
> 
> class Example {
>void example(Object o) {
>switch (o) {
>case String s && s.length() == 0 ->
>System.out.println("1st case");
>case String s && s.length() == 1 ->  // line 6
>System.out.println("2nd case");  // line 7
>case String s -> // line 8
>System.out.println("3rd case");  // line 9
>default ->   // line 10
>System.out.println("default case");  // line 11
>}
>}
> }
> 
> 
> execution of
> 
> 
> javac --enable-preview --release 17 Example.java
> javap -v -p Example.class
> 
> 
> produces
> 
> 
>  void example(java.lang.Object);
>descriptor: (Ljava/lang/Object;)V
>flags: (0x)
>Code:
>  stack=2, locals=7, args_size=2
> 0: aload_1
> 1: dup
> 2: invokestatic  #7  // Method
> 
> java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
> 5: pop
> 6: astore_2
> 7: iconst_0
> 8: istore_3
> 9: aload_2
>10: iload_3
>11: invokedynamic #13,  0 // InvokeDynamic
>#0:typeSwitch:(Ljava/lang/Object;I)I
>16: tableswitch   { // 0 to 2
>   0: 44
>   1: 74
>   2: 105
> default: 122
>}
>44: aload_2
>45: checkcast #17 // class java/lang/String
>48: astore4
>50: aload 4
>52: invokevirtual #19 // Method 
> java/lang/String.length:()I
>55: ifeq  63
>58: iconst_1
>59: istore_3
>60: goto  9
>63: getstatic #23 // Field
>java/lang/System.out:Ljava/io/PrintStream;
>66: ldc   #29 // String 1st case
>68: invokevirtual #31 // Method
>java/io/PrintStream.println:(Ljava/lang/String;)V
>71: goto  133
>74: aload_2
>75: checkcast #17 // class java/lang/String
>78: astore5
>80: aload 5
>82: invokevirtual #19 // Method 
> java/lang/String.length:()I
>85: iconst_1
>86: if_icmpeq 94
>89: iconst_2
>90: istore_3
>91: goto  9
>94: getstatic #23 // Field
>java/lang/System.out:Ljava/io/PrintStream;
>97: ldc   #37 // String 2nd case
>99: invokevirtual #31 // Method
>java/io/PrintStream.println:(Ljava/lang/String;)V
>   102: goto  133
>  

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v7]

[Chris Hegarty]

On Mon, 24 May 2021 21:57:50 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move merge and rejectUndecidedClass methods to OIF.Config
>   As default methods on OIF, their implementations were not concrete and not 
> trustable

src/java.base/share/classes/java/io/ObjectInputFilter.java line 73:

> 71:  * var filter = 
> ObjectInputFilter.Config.createFilter("example.*;java.base/*;!*")
> 72:  * ObjectInputFilter.Config.setSerialFilter(filter);
> 73:  * }

It's good to have a straightforward example, but it has an implicit assumption 
- that the built-in filter factory is in operation ( and will not change ). I 
wonder if there is a way to update the example (without too much fuss), or 
otherwise add a textual qualification. Though, I'm not sure what this would 
look like.

src/java.base/share/classes/java/io/ObjectInputFilter.java line 400:

> 398:  * {@link BinaryOperator {@literal 
> BinaryOperator}} interface, provide its implementation and
> 399:  * be accessible via the {@linkplain 
> ClassLoader#getSystemClassLoader() application class loader}.
> 400:  * The filter factory configured using the system or security 
> property during initialization

What is the expected behaviour if the factory property is to set to a non-class 
or non-accessible class? The current implementation does (it probably should be 
more graceful) :

$ java -Djdk.serialFilterFactory=allow T
Exception in thread "main" java.lang.ExceptionInInitializerError
at 
java.base/java.io.ObjectInputFilter$Config.(ObjectInputFilter.java:537)
at 
java.base/java.io.ObjectInputStream.(ObjectInputStream.java:394)
at T.main(T.java:5)
Caused by: java.lang.ClassNotFoundException: allow
at 
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:636)
at 
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:466)
at 
java.base/java.io.ObjectInputFilter$Config.(ObjectInputFilter.java:519)
... 2 more

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v3]

[Tagir F . Valeev]

On Tue, 25 May 2021 10:31:33 GMT, Patrick Concannon  
wrote:

>> Tagir F. Valeev has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Indent some lines to make GitHub diff happier
>
> src/java.base/share/classes/java/util/GregorianCalendar.java line 1730:
> 
>> 1728: int value = -1;
>> 1729: switch (field) {
>> 1730: case MONTH -> {
> 
> HI @amaembo, thanks for taking a look at this.
> 
> I think you should be careful here, and ask is introducing the enchanced 
> switch adding any value? While I think the enchanced switch can be valuable 
> when it makes the code more readable, it shouldn't be introduced just for the 
> sake of using it.

@pconcannon thank you for review!

In this particular place, it makes the code more better in the following points:
- It removes `break;` operators at the end of each branch, which add nothing 
but a visual noise
- It makes the whole switch shorter by 22 lines
- Using `->` syntax clearly states that no branch in this switch has a 
fall-through behavior, so the code reader should not check every branch to 
ensure this. Just see very first `->` and you already know that no fallthrough 
is here.
- In case if new branches will appear in the future, the new-style switch will 
protect future maintainers from accidental fall-through, an error that often 
happens with old-style switches.

> src/java.base/share/classes/java/util/PropertyPermission.java line 337:
> 
>> 335:  */
>> 336: static String getActions(int mask) {
>> 337: return switch (mask & (READ | WRITE)) {
> 
> Just a suggestion, but it might be more readable if you align the lambda 
> operators

Thank you for pointing to this. Actually, I just noticed that there was some 
kind of vertical alignment before my change. I added vertical alignment for 
single-line switch rules.

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v4]

[Tagir F . Valeev]

> Inspired by PR#4088. Most of the changes are done automatically using 
> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
> including indentations, moving comments, extracting common cast out of switch 
> expression branches, etc.
> 
> I also noticed that there are some switches having one branch only in 
> JapaneseImperialCalendar.java. Probably it would be better to replace them 
> with `if` statement?

Tagir F. Valeev has updated the pull request incrementally with one additional 
commit since the last revision:

  Vertical alignment for single-line switch rules

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4161/files
  - new: https://git.openjdk.java.net/jdk/pull/4161/files/07a998bc..82f40605

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

  Stats: 54 lines in 10 files changed: 2 ins; 0 del; 52 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4161.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4161/head:pull/4161

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v3]

[Patrick Concannon]

On Tue, 25 May 2021 06:01:49 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Indent some lines to make GitHub diff happier

src/java.base/share/classes/java/util/GregorianCalendar.java line 1730:

> 1728: int value = -1;
> 1729: switch (field) {
> 1730: case MONTH -> {

HI @amaembo, thanks for taking a look at this.

I think you should be careful here, and ask is introducing the enchanced switch 
adding any value? While I think the enchanced switch can be valuable when it 
makes the code more readable, it shouldn't be introduced just for the sake of 
using it.

src/java.base/share/classes/java/util/PropertyPermission.java line 337:

> 335:  */
> 336: static String getActions(int mask) {
> 337: return switch (mask & (READ | WRITE)) {

Just a suggestion, but it might be more readable if you align the lambda 
operators

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v7]

[Chris Hegarty]

On Mon, 24 May 2021 21:57:50 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move merge and rejectUndecidedClass methods to OIF.Config
>   As default methods on OIF, their implementations were not concrete and not 
> trustable

The conf/security/java.security file will need to be updated as part of this 
change. It does not have an entry for the factory property, and also its 
description of jdk.serialFilter will be no longer accurate - since filter set 
by jdk.serialFilter may no longer have any impact on OIS, if a filter factory 
is specified as either a system property or security property.

-

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

Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]

[Maxim Kartashev]

> Character strings within JVM are produced and consumed in several formats. 
> Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() 
> or dlopen()) consume strings also in UTF8. On Windows, however, the situation 
> is far less simple: some new(er) APIs expect UTF16 (wide-character strings), 
> some older APIs can only work with strings in a "platform" format, where not 
> all UTF8 characters can be represented; which ones can depends on the current 
> "code page".
> 
> This commit switches the Windows version of native library loading code to 
> using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use 
> of various string formats in the surrounding code. 
> 
> Namely, exception messages are made to consume strings explicitly in the UTF8 
> format, while logging functions (that end up using legacy Windows API) are 
> made to consume "platform" strings in most cases. One exception is 
> `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, 
> which can, of course, be fixed, but was considered not worth the additional 
> code (NB: this isn't a new bug).
> 
> The test runs in a separate JVM in order to make NIO happy about non-ASCII 
> characters in the file name; tests are executed with LC_ALL=C and that 
> doesn't let NIO work with non-ASCII file names even on Linux or MacOS.
> 
> Tested by running `test/hotspot/jtreg:tier1` on Linux and 
> `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (`   
> jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran 
> on those platforms as well.
> 
> Results from Linux:
> 
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>jtreg:test/hotspot/jtreg:tier1 1784  1784 0 0  
>  
> ==
> TEST SUCCESS
> 
> 
> Building target 'run-test-only' in configuration 'linux-x86_64-server-release'
> Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', 
> will run:
> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode
> 
> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode'
> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
> Test results: passed: 1
> 
> 
> Results from Windows 10:
> 
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR
>jtreg:test/hotspot/jtreg/runtime746   746 0 0
> ==
> TEST SUCCESS
> Finished building target 'run-test-only' in configuration 
> 'windows-x86_64-server-fastdebug'
> 
> 
> Building target 'run-test-only' in configuration 
> 'windows-x86_64-server-fastdebug'
> Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run:
> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode
> 
> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode'
> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
> Test results: passed: 1

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

  8195129: System.load() fails to load from unicode paths

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4169/files
  - new: https://git.openjdk.java.net/jdk/pull/4169/files/265f7907..fe661dff

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

  Stats: 55 lines in 3 files changed: 24 ins; 1 del; 30 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4169.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169

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

Re: RFR: 8265768 [aarch64] Use glibc libm impl for dlog, dlog10, dexp iff 2.29 or greater on AArch64.

[Andrew Dinn]

On Thu, 15 Apr 2021 08:33:47 GMT, gregcawthorne 
 wrote:

> Glibc 2.29 onwards provides optimised versions of log,log10,exp.
> These functions have an accuracy of 0.9ulp or better in glibc
> 2.29.
> 
> Therefore this patch adds code to parse, store and check
> the runtime glibcs version in os_linux.cpp/hpp.
> This is then used to select the glibcs implementation of
> log, log10, exp at runtime for c1 and c2, iff we have
> glibc 2.29 or greater.
> 
> This will ensure OpenJDK can benefit from future improvements
> to glibc.
> 
> Glibc adheres to the ieee754 standard, unless stated otherwise
> in its spec.
> 
> As there are no stated exceptions in the current glibc spec
> for dlog, dlog10 and dexp, we can assume they currently follow
> ieee754 (which testing confirms). As such, future version of
> glibc are unlikely to lose this compliance with ieee754 in
> future.
> 
> W.r.t performance this patch sees ~15-30% performance improvements for
> log and log10, with ~50-80% performance improvements for exp for the
> common input ranged (which output real numbers). However for the NaN
> and inf output ranges we see a slow down of up to a factor of 2 for
> some functions and architectures.
> 
> Due to this being the uncommon case we assert that this is a
> worthwhile tradeoff.

> [ One thing: Java uses the term "semi-monotonic" to
> mean "whenever the mathematical function is non-decreasing, so is
> the floating-point approximation, likewise, whenever the
> mathematical function is non-increasing, so is the floating-point
> approximation." I don't really understand what distinction means. ]

I believe this is to allow for the fact that the function is continuous and the 
floating-point approximation is discrete.

Let F be the actual function and f the floating point approximation.  Assume we 
have two successive floating point values x, x'  and, without loss of 
generality, F(x) <= F(x'). What are the circumstances under which we require 
f(x) =< f(x')? Semi-monotonicity says that is only needed when F is 
non-decreasing on the interval [x, x']. Expressed more precisely, the condition 
that F is non-decreasing is

  for all y such that x =< y =< x' : F(x) <= F(y) <= F(x').

In other words:

  if the graph only ever stays level or increases across the interval [x, x'] 
then we must have f(x) =< f(x')

  If the graph wiggles *up* and *down* across the interval [x, x'] we can allow 
f(x) > f(x').

-

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

Re: RFR: 8267612; Declare package-private VarHandle.AccessMode/AccessType counts [v2]

[Claes Redestad]

On Mon, 24 May 2021 22:49:05 GMT, Claes Redestad  wrote:

>> Slightly reduce VarHandle startup overhead by introducing package-private 
>> COUNT constants for two enums
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Mandy review

Thanks Mandy!

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Evgeny Mandrikov]

On Mon, 17 May 2021 19:01:01 GMT, Jan Lahoda  wrote:

>> Good work. There's a lot to take in here. I think overall, it holds up well 
>> - I like how case labels have been extended to accommodate for patterns. In 
>> the front-end, I think there are some questions over the split between Attr 
>> and Flow - maybe it is unavoidable, but I'm not sure why some control-flow 
>> analysis happens in Attr instead of Flow and I left some questions to make 
>> sure I understand what's happening.
>> 
>> In the backend it's mostly good work - but overall the structure of the 
>> code, both in Lower and in TransPattern is getting very difficult to follow, 
>> given there are many different kind of switches each with its own little 
>> twist attached to it. It would be nice, I think (maybe in a separate 
>> cleanup?) if the code paths serving the different switch kinds could be made 
>> more separate, so that, when reading the code we can worry only about one 
>> possible code shape. That would make the code easier to document as well.
>
> @mcimadamore, @forax, thanks a lot for comments! I tried to apply the 
> requested changes in recent commits 
> (https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b
>  
> https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99
>  ).
> 
> I've also tried to update the implementation for the latest spec changes here:
> https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
> 
> The spec changes are: total patterns are nullable; pattern matching ("new") 
> statement switches must be complete (as switch expressions).
> 
> Any further feedback is welcome!

Hi @lahodaj   ,

I tried `javac` built from this PR and for the following `Example.java`


class Example {
void example(Object o) {
switch (o) {
case String s && s.length() == 0 ->
System.out.println("1st case");
case String s && s.length() == 1 ->  // line 6
System.out.println("2nd case");  // line 7
case String s -> // line 8
System.out.println("3rd case");  // line 9
default ->   // line 10
System.out.println("default case");  // line 11
}
}
}


execution of


javac --enable-preview --release 17 Example.java
javap -v -p Example.class


produces


  void example(java.lang.Object);
descriptor: (Ljava/lang/Object;)V
flags: (0x)
Code:
  stack=2, locals=7, args_size=2
 0: aload_1
 1: dup
 2: invokestatic  #7  // Method 
java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
 5: pop
 6: astore_2
 7: iconst_0
 8: istore_3
 9: aload_2
10: iload_3
11: invokedynamic #13,  0 // InvokeDynamic 
#0:typeSwitch:(Ljava/lang/Object;I)I
16: tableswitch   { // 0 to 2
   0: 44
   1: 74
   2: 105
 default: 122
}
44: aload_2
45: checkcast #17 // class java/lang/String
48: astore4
50: aload 4
52: invokevirtual #19 // Method 
java/lang/String.length:()I
55: ifeq  63
58: iconst_1
59: istore_3
60: goto  9
63: getstatic #23 // Field 
java/lang/System.out:Ljava/io/PrintStream;
66: ldc   #29 // String 1st case
68: invokevirtual #31 // Method 
java/io/PrintStream.println:(Ljava/lang/String;)V
71: goto  133
74: aload_2
75: checkcast #17 // class java/lang/String
78: astore5
80: aload 5
82: invokevirtual #19 // Method 
java/lang/String.length:()I
85: iconst_1
86: if_icmpeq 94
89: iconst_2
90: istore_3
91: goto  9
94: getstatic #23 // Field 
java/lang/System.out:Ljava/io/PrintStream;
97: ldc   #37 // String 2nd case
99: invokevirtual #31 // Method 
java/io/PrintStream.println:(Ljava/lang/String;)V
   102: goto  133
   105: aload_2
   106: checkcast #17 // class java/lang/String
   109: astore6
   111: getstatic #23 // Field 
java/lang/System.out:Ljava/io/PrintStream;
   114: ldc   #39 // String 3rd case
   116: invokevirtual #31 // Method 
java/io/PrintStream.println:(Ljava/lang/String;)V
   119: goto  133
   122: getstatic #23 // 

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Chris Hegarty]

On Mon, 24 May 2021 15:09:26 GMT, Roger Riggs  wrote:

> i) is too limiting. It should be possible for an application to check whether 
> a filter factory has been provided on the command line (by calling 
> getSerialFilterFactory) and if not setting the factory itself. It may also 
> want to install its own filter factory that delegates to the builtin factory 
> without needed to re-implement the builtin behavior.

How is this supposed to work in practice?  getSerialFilterFactory always 
returns a non-null factory, so how does one know whether or not the returned 
factory is the built-in factory, a factory set by the command line (or security 
property) ? (without resorting to implementation assumptions)

-

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

Integrated: 8267110: Update java.util to use instanceof pattern variable

[Patrick Concannon]

On Tue, 18 May 2021 10:37:21 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.util` 
> package to make use of the `instanceof` pattern variable?
> 
> Kind regards,
> Patrick

This pull request has now been integrated.

Changeset: a52c4ede
Author:Patrick Concannon 
URL:   
https://git.openjdk.java.net/jdk/commit/a52c4ede2f043b7d4a234c7d06f91871312e9654
Stats: 267 lines in 29 files changed: 1 ins; 125 del; 141 mod

8267110: Update java.util to use instanceof pattern variable

Reviewed-by: lancea, naoto

-

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

Re: RFR: 8266310: deadlock while loading the JNI code [v5]

[Aleksei Voitylov]

On 25/05/2021 04:44, David Holmes wrote:
> On 25/05/2021 7:53 am, Aleksei Voitylov wrote:
>> On Mon, 24 May 2021 06:24:15 GMT, David Holmes 
>> wrote:
>>
 Aleksei Voitylov has updated the pull request incrementally with
 one additional commit since the last revision:

    fix whitespace
>>>
>>> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java
>>> line 511:
>>>
 509: if (currentLock.getCounter() == 1) {
 510: // unlock and release the object if no other
 threads are queued
 511: currentLock.unlock();
>>>
>>> This isn't atomic so I don't see how it can work as desired. Overall
>>> I don't understand what the semantics of this "counted lock" are
>>> intended to be.
>>
>> Hi David,
>>
>> The locking strategy is as follows:
>>
>> CountedLock is a subclass of ReentrantLock that allows exact counting
>> of threads that intend to acquire the lock object. Each time a thread
>> calls acquireNativeLibraryLock() with a certain name, either a new
>> CountedLock object is allocated and assigned 1 as the counter, or an
>> existing CountedLock is incremented prior to invocation of the lock()
>> method on it. The increment operation to the lock object is performed
>> in the context of execution of compute() method, which is executed
>> atomically. This allows to correctly dispose the CountedLock object
>> when the last thread in the queue leaves releaseNativeLibraryLock().
>> The entire remapping function passed to computeIfPresent() method is
>> executed atomically.
>
> So the counting is trying to be an in-use count so that it can be
> deleted when not needed? 
Yes. The ReentrantLock does not provide an exact count of threads in the
queue, so we had to invent something here. ReentrantLock only has the
lock.state that's exact which is the number of holds by the same thread
which is not what we need to make sure the object can be disposed.
> I'm still not clear on exactly what this counting is doing (partly
> because I have trouble reading the lambda expressions that relate to
> the lock).
The counter is incremented each time a thread calls
acquireNativeLibraryLock(), regardless of that it is the same or a
different thread.
>
>> Could you be more specific on what is not performed atomically?
>
> The code I referenced. The counter is not protected by the lock that
> it counts. 
Yes, but it is protected by a barrier created by synchronized() block in
the computeIfPresent() code. The entire lambda is executed inside that
synchronized() block. The ReentrantLock.unlock() is called here so that
the calling thread releases the per-library lock object, prior to
"forgetting" the reference to that object.
> In the code above we hold the lock and check if the counter == 1
> before unlocking. But the counter is incremented without the lock
> held, so as soon as we have called getCounter() the count could have
> changed.
The counter is incremented and decremented in the lambda expression
executed inside computeXXX() method which blocks on the hash map node
containing the lock object. That lambda is guaranteed to be executed
atomically for all threads referencing the same key.
>
> David
> -
>
>> -
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3976
>>

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]

[Alan Bateman]

On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs  wrote:

>> Methods are added to java.lang.Process to read and write characters and 
>> lines from and to a spawned Process.
>> The Charset used to encode and decode characters to bytes can be specified 
>> or use the
>> operating system native encoding as is available from the "native.encoding" 
>> system property.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added throws for NPE, delegated zero-arg methods that use native.encoding to
>   the 1-arg method with a charset, added tests for Redirect cases where the 
> streams are null-input or null-output streams.

The updated javadoc addresses most of my points. The clarification to 
inputReader/errorReader about malformed input looks good but we will need the 
equivalent in outputWriter for the unmappable character case.
I assume the "not null" can be dropped from the description of the charset 
parameter as NPE is now specified.

-

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

Re: RFR: 8263561: Re-examine uses of LinkedList [v2]

[Сергей Цыпанов]

> The usage of `LinkedList` is senseless and can be replaced with either 
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
> 
> jdk:tier1 and jdk:tier2 are both ok

Сергей Цыпанов 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 three new commits 
since the last revision:

 - 8263561: Use interface List instead of particular type where possible
 - 8263561: Rename requestList -> requests
 - 8263561: Re-examine uses of LinkedList

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2744/files
  - new: https://git.openjdk.java.net/jdk/pull/2744/files/73029fe1..158006c6

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

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

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

Re: RFR: 8195129: System.load() fails to load from unicode paths

[Alan Bateman]

On Mon, 24 May 2021 16:43:09 GMT, Maxim Kartashev 
 wrote:

> Character strings within JVM are produced and consumed in several formats. 
> Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() 
> or dlopen()) consume strings also in UTF8. On Windows, however, the situation 
> is far less simple: some new(er) APIs expect UTF16 (wide-character strings), 
> some older APIs can only work with strings in a "platform" format, where not 
> all UTF8 characters can be represented; which ones can depends on the current 
> "code page".
> 
> This commit switches the Windows version of native library loading code to 
> using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use 
> of various string formats in the surrounding code. 
> 
> Namely, exception messages are made to consume strings explicitly in the UTF8 
> format, while logging functions (that end up using legacy Windows API) are 
> made to consume "platform" strings in most cases. One exception is 
> `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, 
> which can, of course, be fixed, but was considered not worth the additional 
> code (NB: this isn't a new bug).
> 
> The test runs in a separate JVM in order to make NIO happy about non-ASCII 
> characters in the file name; tests are executed with LC_ALL=C and that 
> doesn't let NIO work with non-ASCII file names even on Linux or MacOS.
> 
> Tested by running `test/hotspot/jtreg:tier1` on Linux and 
> `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (`   
> jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran 
> on those platforms as well.
> 
> Results from Linux:
> 
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>jtreg:test/hotspot/jtreg:tier1 1784  1784 0 0  
>  
> ==
> TEST SUCCESS
> 
> 
> Building target 'run-test-only' in configuration 'linux-x86_64-server-release'
> Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', 
> will run:
> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode
> 
> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode'
> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
> Test results: passed: 1
> 
> 
> Results from Windows 10:
> 
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR
>jtreg:test/hotspot/jtreg/runtime746   746 0 0
> ==
> TEST SUCCESS
> Finished building target 'run-test-only' in configuration 
> 'windows-x86_64-server-fastdebug'
> 
> 
> Building target 'run-test-only' in configuration 
> 'windows-x86_64-server-fastdebug'
> Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run:
> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode
> 
> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode'
> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
> Test results: passed: 1

test/hotspot/jtreg/runtime/jni/loadLibraryUnicode/LoadLibraryUnicode.java line 
6:

> 4:  * Licensed under the Apache License, Version 2.0 (the "License");
> 5:  * you may not use this file except in compliance with the License.
> 6:  * You may obtain a copy of the License at

The test sources have Apache license, I thought we always used GPL for tests.

-

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]

[Alan Bateman]

On Mon, 24 May 2021 01:21:09 GMT, Roger Riggs  wrote:

> On the question of process termination behavior, I'm not sure what can be 
> said that could be specification.
> The implementations vary between Mac, Linux, and Windows; with the common 
> thread
> to avoid losing process output. But this may have to be included in the 
> unspecified implementation behavior
> or just an API note. Suggestions?

I think the javadoc could set expectations that the behavior when the process 
has terminated, and streams have not been redirected, is unspecified. Reading 
from the process output/error streams may read some or no bytes/characters.

-

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

Re: RFR: 8224243: Make AccessibleObject a sealed class [v3]

[Alan Bateman]

On Mon, 24 May 2021 21:54:09 GMT, Joe Darcy  wrote:

>> Conceptually, AccessbileObject is a sealed class with a protected 
>> constructor stating
>> 
>> Constructor: only used by the Java Virtual Machine.
>> 
>> With the language now supporting sealed classes, the AccessbileObject should 
>> be marked as sealed.
>> 
>> Executable and Field are the subclasses of AccessbileObject in the JDK; as 
>> Executable has subclasses, it is marked as non-sealed.
>> 
>> Please also review the corresponding CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8224243
>
> 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 five additional commits since 
> the last revision:
> 
>  - Minor fixes.
>  - Change to UnsupportedOperationException.
>  - Merge branch 'master' into 8224243
>  - Update in response to review feedback.
>  - 8224243: Make AccessibleObject a sealed class

The updated proposal looks good although the JBS issue should probably be 
renamed as the proposal is no longer to seal AccessibleObject.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v3]

[Tagir F . Valeev]

On Tue, 25 May 2021 03:48:41 GMT, Tagir F. Valeev  wrote:

>> src/java.base/share/classes/java/util/regex/CharPredicates.java line 217:
>> 
>>> 215: case "WORD" -> WORD();
>>> 216: default -> null;
>>> 217: };
>> 
>> This file has lots of changes which are difficult to review. Maybe it should 
>> be split out of this PR.
>
> *sigh* GitHub diff tool is really poor and outdated. Here's how it looks in 
> IntelliJ IDEA diff view:
> ![image](https://user-images.githubusercontent.com/5114450/119436474-6b455b80-bd46-11eb-8865-8b7f30826a8d.png)

I played with indentations and found a way to make GitHub diff happier. Now, 
lines like `? UPPERCASE().union(LOWERCASE(), TITLECASE())` are probably a 
little bit too far to the right but it's still acceptable and diff looks much 
easier to review now.

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v3]

[Tagir F . Valeev]

> Inspired by PR#4088. Most of the changes are done automatically using 
> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
> including indentations, moving comments, extracting common cast out of switch 
> expression branches, etc.
> 
> I also noticed that there are some switches having one branch only in 
> JapaneseImperialCalendar.java. Probably it would be better to replace them 
> with `if` statement?

Tagir F. Valeev has updated the pull request incrementally with one additional 
commit since the last revision:

  Indent some lines to make GitHub diff happier

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4161/files
  - new: https://git.openjdk.java.net/jdk/pull/4161/files/2e966d0f..07a998bc

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

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

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