RFR: 8266291: (jrtfs) Calling Files.exists may break the JRT filesystem

[Athijegannathan Sundararajan]

Problematic paths that start with /modules/modules prefix are handled properly

-

Commit messages:
 - fixed typos in comments
 - 8266291: (jrtfs) Calling Files.exists may break the JRT filesystem

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

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

Re: RFR: 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2 [v4]

[Ioi Lam]

> This bug was discovered during the development of 
> [JDK-6824466](https://bugs.openjdk.java.net/browse/JDK-6824466): when CDS is 
> enabled, if `BootLoader::loadClassOrNull` is called before `initPhase2`, it 
> would trigger the initialization of the archived module graph in the wrong 
> order. Because of unanticipated nesting of `` methods, 
> `BootLoader::SERVICES_CATALOG` becomes empty, causing future `ServiceLoader` 
> operations to fail.
> 
> The fix has 2 parts:
> 
> - `BootLoader::loadClassOrNull` no longer calls `ClassLoaders::bootLoader()`. 
> This avoids triggering the archived module graph initialization. Instead, it 
> makes a direct call to `Classloader::findBootstrapClassOrNull()`. We don't 
> actually need a `ClassLoader` instance for this call, so I changed 
> `Classloader::findBootstrapClassOrNull()` to be a static method.
> - The object returned by `BootLoader::getServicesCatalog()` is now maintained 
> inside `jdk.internal.loader.ClassLoaders`.  Although not strictly required 
> for the fix, this simplifies the initialization of the archived module graph. 
> It also makes the logic consistent for the 3 built-in loaders 
> (boot/platform/app).
> 
> Testing: tiers1-4 in progress. I also verified that the bug reported by Mandy 
> is no longer reproducible after I applied this patch on her branch.

Ioi Lam has updated the pull request with a new target base due to a merge or a 
rebase. The incremental webrev excludes the unrelated changes brought in by the 
merge/rebase. The pull request contains four additional commits since the last 
revision:

 - Merge branch 'master' into 
8265605-bootloader-loadClassOrNull-before-init-phase2
 - cleaned up ClassLoaders.setArchivedServicesCatalog
 - @AlanBateman comments
 - 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3992/files
  - new: https://git.openjdk.java.net/jdk/pull/3992/files/4cd981c0..a3dc9427

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

  Stats: 9450 lines in 343 files changed: 2601 ins; 4164 del; 2685 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3992.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3992/head:pull/3992

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

Re: RFR: 8254598: StringDedupTable should use OopStorage [v4]

[Kim Barrett]

On Fri, 14 May 2021 04:31:59 GMT, Kim Barrett  wrote:

>> Please review this change to the String Deduplication facility.  It is being
>> changed to use OopStorage to hold weak references to relevant objects,
>> rather than bespoke data structures requiring dedicated processing phases by
>> supporting GCs.
>> 
>> (The Shenandoah update was contributed by Zhengyu Gu.)
>> 
>> This change significantly simplifies the interface between a given GC and
>> the String Deduplication facility, which should make it much easier for
>> other GCs to opt in.  However, this change does not alter the set of GCs
>> providing support; currently only G1 and Shenandoah support string
>> deduplication.  Adding support by other GCs will be in followup RFEs.
>> 
>> Reviewing via the diffs might not be all that useful for some parts, as
>> several files have been essentially completely replaced, and a number of
>> files have been added or eliminated.  The full webrev might be a better
>> place to look.
>> 
>> The major changes are in gc/shared/stringdedup/* and in the supporting
>> collectors, but there are also some smaller changes in other places, most
>> notably classfile/{stringTable,javaClasses}.
>> 
>> This change is additionally labeled for review by core-libs, although there
>> are no source changes there.  This change injects a byte field of bits into
>> java.lang.String, using one of the previously unused padding bytes in that
>> class.  (There were two unused bytes, now only one.)
>> 
>> Testing:
>> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>> 
>> Locally (linux-x64) ran all of the existing tests that use string
>> deduplication with both G1 and Shenandoah.  Note that
>> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
>> currently fails, for reasons I haven't figured out but suspect are test
>> related.
>> 
>> - gc/stringdedup/   -- these used to be in gc/g1
>> - runtime/cds/SharedStringsDedup.java
>> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
>> - runtime/cds/appcds/sharedStrings/*
>> 
>> shenandoah-only:
>> - gc/shenandoah/jvmti/TestHeapDump.java
>> - gc/shenandoah/TestStringDedup.java
>> - gc/shenandoah/TestStringDedupStress.java
>> 
>> Performance tested baseline, baseline + stringdedup, new with stringdedup,
>> finding no significant differences.
>
> Kim Barrett has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into new_dedup2
>  - misc cleanups
>  - refactor Requests::add
>  - fix typo
>  - more comment improvements
>  - improve naming and comments around injected String flags
>  - fix some typos in comments
>  - New string deduplication

The "merge from master" commit (ccb9951) doesn't build with Shenandoah.  I've 
asked Zhengyu to take a look.

-

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

Re: RFR: 8254598: StringDedupTable should use OopStorage [v4]

[Kim Barrett]

> Please review this change to the String Deduplication facility.  It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
> 
> (The Shenandoah update was contributed by Zhengyu Gu.)
> 
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in.  However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication.  Adding support by other GCs will be in followup RFEs.
> 
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated.  The full webrev might be a better
> place to look.
> 
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
> 
> This change is additionally labeled for review by core-libs, although there
> are no source changes there.  This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class.  (There were two unused bytes, now only one.)
> 
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
> 
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah.  Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
> 
> - gc/stringdedup/   -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
> 
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
> 
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.

Kim Barrett has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains eight commits:

 - Merge branch 'master' into new_dedup2
 - misc cleanups
 - refactor Requests::add
 - fix typo
 - more comment improvements
 - improve naming and comments around injected String flags
 - fix some typos in comments
 - New string deduplication

-

Changes: https://git.openjdk.java.net/jdk/pull/3662/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3662=03
  Stats: 6191 lines in 104 files changed: 2369 ins; 3204 del; 618 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662

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

RFR: 8249395: (macos) jpackage tests timeout on MacPro5_1 systems

[Alexander Matveev]

Looks like it is similar to JDK-8236282, except for "hdiutil create". Based on 
call stack from failed test we launched "hdiutil create" and were waiting for 
it to terminate while reading output from it. However, based on process dump 
from machine which reproduced this issue, hdiutil no longer runs and existed 
long time ago. Fixed in same way as JDK-8236282 by writing output to file and 
then reading it back when process terminates.

-

Commit messages:
 - 8249395: (macos) jpackage tests timeout on MacPro5_1 systems

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

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

Re: RFR: 8266310: deadlock while loading the JNI code

[David Holmes]

On 14/05/2021 7:20 am, Mandy Chung wrote:



On 5/13/21 6:05 AM, David Holmes wrote:
Not every problem has a solution :) If JNI_OnLoad has to execute 
atomically with respect to loading a library then there will always be 
a deadlock potential. The only complete solution would not hold a lock 
while JNI_OnLoad is executed, but that completely changes the 
semantics of native library loading. 


I have been giving some thought on this issue.  I agree with David that 
we should look into a solution not holding a lock while JNI_OnLoad is 
executed.  It might be possible to put all threads that trying to load 
the same native library that is being loaded to wait and only one thread 
is loading it and executing JNI_OnLoad (e.g. add a state in 
NativeLibrary to indicate it is being loaded, already loaded, being 
unloaded).  I haven't had the cycle to think through it in details though.


Any mechanism that blocks threads from accessing the library while 
another thread is performing JNI_OnLoad, suffers from the same deadlock 
problem.


David


Mandy

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v3]

[Mandy Chung]

On Thu, 13 May 2021 23:07:07 GMT, Mark Reinhold  wrote:

>> Please review this implementation of JEP 403 
>> (https://openjdk.java.net/jeps/403).
>> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 
>> on 
>> {linux,macos,windows}-x64 and {linux,macos}-aarch64.
>
> Mark Reinhold has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove now-unnecessary uses and mentions of the --illegal-access option

Looks good.

-

Marked as reviewed by mchung (Reviewer).

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mark Reinhold]

On Thu, 13 May 2021 18:16:23 GMT, Mandy Chung  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove stray mentions of the jdk.module.illegalAccess property
>
> There are a few tests with `--illegal-access=deny` for example
> 
> test/jdk/java/lang/ModuleTests/BasicModuleTest.java
> test/jdk/java/lang/instrument/RedefineModuleTest.java
> test/jdk/java/lang/invoke/CallerSensitiveAccess.java
> test/jdk/java/lang/reflect/AccessibleObject/ - a few tests
> test/jdk/jdk/modules/open/Basic.java
> test/jdk/tools/launcher/modules/addexports/manifest/AddExportsAndOpensInManifest.java
> 
> 
> It would be good to remove these references to `--illegal-access` option in 
> this patch too.

@mlchung Thanks for pointing out those stray uses of `--illegal-access`. I’ve 
removed them, along with the mention of `--illegal-access` in 
`launcher.properties`.

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v3]

[Mark Reinhold]

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

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

  Remove now-unnecessary uses and mentions of the --illegal-access option

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4015/files
  - new: https://git.openjdk.java.net/jdk/pull/4015/files/719ff4ee..1c14998e

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

  Stats: 26 lines in 10 files changed: 0 ins; 8 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4015.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4015/head:pull/4015

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

Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]

[Kim Barrett]

On Wed, 12 May 2021 11:55:08 GMT, Thomas Schatzl  wrote:

>> Kim Barrett has updated the pull request incrementally with three additional 
>> commits since the last revision:
>> 
>>  - more comment improvements
>>  - improve naming and comments around injected String flags
>>  - fix some typos in comments
>
> src/hotspot/share/gc/shared/stringdedup/stringDedup.hpp line 82:
> 
>> 80: // Doing so would counteract C2 optimizations on string literals.  But an
>> 81: // interned string might later become a deduplication candidate through 
>> the
>> 82: // normal GC discovery mechanism.  To prevent such modificatoins, the
> 
> s/modificatoins/modifications

Fixed.

-

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

Re: RFR: 8254598: StringDedupTable should use OopStorage [v3]

[Kim Barrett]

> Please review this change to the String Deduplication facility.  It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
> 
> (The Shenandoah update was contributed by Zhengyu Gu.)
> 
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in.  However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication.  Adding support by other GCs will be in followup RFEs.
> 
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated.  The full webrev might be a better
> place to look.
> 
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
> 
> This change is additionally labeled for review by core-libs, although there
> are no source changes there.  This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class.  (There were two unused bytes, now only one.)
> 
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
> 
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah.  Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
> 
> - gc/stringdedup/   -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
> 
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
> 
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.

Kim Barrett has updated the pull request incrementally with three additional 
commits since the last revision:

 - misc cleanups
 - refactor Requests::add
 - fix typo

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3662/files
  - new: https://git.openjdk.java.net/jdk/pull/3662/files/656e2426..30e7b8e9

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

  Stats: 69 lines in 6 files changed: 26 ins; 26 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662

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

Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]

[Kim Barrett]

On Wed, 12 May 2021 21:13:54 GMT, Albert Mingkun Yang  wrote:

>> Kim Barrett has updated the pull request incrementally with three additional 
>> commits since the last revision:
>> 
>>  - more comment improvements
>>  - improve naming and comments around injected String flags
>>  - fix some typos in comments
>
> Just some minor comments.

Following up on an off-line discussion with @albertnetymk , I've done a little 
refactoring of Requests::add.  I also made a few other small cleanups, noticed 
while dealing with @albertnetymk comments.  I still haven't dealt with the 
accumulated merge conflicts.  I'll be doing that next.

> src/hotspot/share/gc/shared/stringdedup/stringDedup.cpp line 171:
> 
>> 169:   // Store the string in the next pre-allocated storage entry.
>> 170:   oop* ref = _buffer[--_index];
>> 171:   _buffer[_index] = nullptr;
> 
> It's not really needed to clear the slot with null, right?

You are right; there used to be some asserts, but no longer. Removed.

> src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 51:
> 
>> 49: }
>> 50: 
>> 51: StringDedup::Processor::~Processor() {}
> 
> Why the empty destructor? Could we just not have it?

Changed to use `= default`.

> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 92:
> 
>> 90:   GrowableArrayCHeap _values;
>> 91: 
>> 92:   void adjust_capacity(int new_length);
> 
> Nit: diff arg name in declaration and implementation, `new_length` vs 
> `new_capacity`.

Changed to consistently use `new_capacity`

> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 295:
> 
>> 293: 
>> 294: protected:
>> 295:   CleanupState() {}
> 
> Is it possible to use `= default` here? Just like the destructor.

Changed to use `= default`.

> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:
> 
>> 312:   size_t _bucket_index;
>> 313:   size_t _shrink_index;
>> 314:   bool _grow_only;
> 
> Seems unused.

Removed unused `_grow_only`, left over from earlier work.

> src/hotspot/share/memory/universe.cpp line 1153:
> 
>> 1151: ResolvedMethodTable::verify();
>> 1152:   }
>> 1153:   if (should_verify_subset(Verify_StringDedup) && 
>> StringDedup::is_enabled()) {
> 
> Maybe drop the `is_enabled()` check in the `if` to keep the consistency with 
> other `if`s?

Done.  StringDedup::verify already does an is_enabled check.  Also fixed the 
description comment, which incorrectly said `is_enabled()` was a precondition.

-

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

Re: [External] : Re: ReversibleCollection proposal

[forax]

- Mail original -
> De: "Anthony Vanelverdinghe" 
> À: "Remi Forax" 
> Cc: "Stuart Marks" , "core-libs-dev" 
> 
> Envoyé: Jeudi 13 Mai 2021 21:18:22
> Objet: Re: [External] : Re:  ReversibleCollection proposal

> Hi Rémi
> 
> The discussion "new types? or new default methods?" is indeed a valid one. In
> fact, I think this choice has already been made once in favor of a default
> method, when adding `List::sort` was chosen over adding `SortedList` (though I
> imagine that choice was a no-brainer).

yes, very true, there is no SortedList even if it would be useful to know if a 
list is already sorted, by example, we can add binarySearch() on it with no 
problem of people using it on an unsorted list.

> 
> In this case I prefer adding new types though, so I wanted to share my take on
> this.
> 
> In the following diagram (see [1] in case it got mangled), I've tried to 
> arrange
> all relevant Collection types by their defining characteristics:
> 
>|- Collection
>|---|
>|unorderedordered   sorted   
>distinct
>|distinct & ordereddistinct & sorted |
>|Set   
>  |
>| Queue = (get|remove)First + addLast   PriorityQueue
>| |
>| Stack = (add|get|remove)Last
>| |
>| Deque = Queue + Stack + addFirst
>| LinkedHashSet SortedSet |
>| List = Deque + (add|get|remove)IndexedList::sort
>| NavigableSet  |
>||
> 
> As I see it, there are a couple of issues with this:
> * conceptually, every List is a Deque, but List doesn't extend Deque. If it
> would, all uses of List (e.g. as a parameter type in APIs) where the indexed
> access isn't required could be replaced with Deque instead. Or e.g. when you
> need to take a stack as argument: currently Deque is the recommended parameter
> type, but that means you can't just pass a List to the method as-is. With RC,
> you'd be able to use that as parameter type and pass both Deque and List.

You can use the same argument to have a common super type between Map and 
Collection, it would be very useful.

Until you think what methods are available on that type, size() and isEmpty(), 
meh, this is useless.

RC has the same issue, it's stuck in between Collection and List, so the cases 
where you don't want a Collection because it has not enough methods you want to 
use but at the same time you don't want a List because it has too many methods 
are vanishingly small.

The problem is that adding RC is not source compatible, so you are asking to 
add something in the JDK which serve a corner case but at the same time forces 
people to change their code even if they don't use RC.
It's a loose / loose situation, we will have to maintain a collection forever 
in the JDK that nobody uses but everybody will pay for it.

[...]

> 
> On Wednesday, May 12, 2021 13:22 CEST, fo...@univ-mlv.fr wrote:
> 
>> First, i think we have overlooked ReversibleMap, if you have a LinkedHashMap,
>> the keySet should be a ReversibleSet.
> 
> I'm not sure what you meant, but the PR already defines 
> `LinkedHashMap::keySet`
> as `public ReversibleSet keySet()`.

LinkedHashMap is perhaps the only implementation which has a keySet which is 
reversible, but there are a lot of other collection implementations, Apache 
collection, Guava, Eclipse, clojure-core, etc that may have such kind of 
implementation too. The same way you want RC between Queue and List, you want 
ReversibleMap between those Map implementations.

> 
>> Again, we have seen that introducing those interfaces
>> - is not source backward compatible thus it will be painful for some of our
>> users,
>>   I believe NavigableSet/NavigableMap did not have that issue because only 
>> one
>>   existing implementation per interface was retrofitted to implement those
>>   interfaces, TreeSet for NavigableSet and TreeMap for NavigableMap.
>>   ConcurrentSkipListSet/ConcurrentSkipListMap were added at the same time, so
>>   there was no existing code doing a lub (lowest upper bound) between 
>> TreeSet and
>>   ConcurrentSkipListSet.
>>   Here, they are a lot of implementations that will implement be retrofitted 
>> to
>>   ReversibleCollection, ReversibleSet and ReversibleMap.
> 
> As far as I understand, both options might cause source incompatibilities, 
> but I
> assume you estimate the actual impact of adding default methods to be (much)
> smaller?

Adding a default method may creates source incompatibilities but this can be 
solved by having a name with no collision, that why Iterator.forEachRemaining 
is such a long name by example.

Re: RFR: 8266310: deadlock while loading the JNI code

[Mandy Chung]

On 5/13/21 6:05 AM, David Holmes wrote:
Not every problem has a solution :) If JNI_OnLoad has to execute 
atomically with respect to loading a library then there will always be 
a deadlock potential. The only complete solution would not hold a lock 
while JNI_OnLoad is executed, but that completely changes the 
semantics of native library loading. 


I have been giving some thought on this issue.  I agree with David that 
we should look into a solution not holding a lock while JNI_OnLoad is 
executed.  It might be possible to put all threads that trying to load 
the same native library that is being loaded to wait and only one thread 
is loading it and executing JNI_OnLoad (e.g. add a state in 
NativeLibrary to indicate it is being loaded, already loaded, being 
unloaded).  I haven't had the cycle to think through it in details though.


Mandy

Re: Proposal for new interface: TimeSource

[Stephen Colebourne]

On Wed, 12 May 2021 at 18:41, Roger Riggs  wrote:
> Will you be posting a PR for the implementation?
> It is frequently helpful to evaluate the CSR and the implementation
> concurrently.

PR: https://github.com/openjdk/jdk/pull/4016
Issue: https://bugs.openjdk.java.net/browse/JDK-8266846
CSR: https://bugs.openjdk.java.net/browse/JDK-8266847

The PR takes a middle ground approach to the implementation.

It is not practical to remove the existing package-scoped Clock
implementation classes (SystemClock, TickClock, FixedClock,
OffsetClock) despite the fact that these would be better expressed as
classes that only implement `InstantSource`. However, given that
"system" is the 99%+ use case, I do believe it is worth adding a
dedicated `SystemInstantSource` class, as per the PR.

To achieve this I moved the actual logic using
`VM.getNanoAdjustment()` into a static method which can then be called
directly from three places - Instant.now(), SystemClock and
SystemInstantSource. Previously, every instance of SystemClock
performed the VM/offset calculations separately. The new logic
performs them once based on a single shared static variable. I have no
reason to believe this changes the memory model or performance, but I
must flag it up for reviewers.

When initially discussing the proposal, I planned to add a new static
method `Clock.of(InstantSource, ZoneId)`. When implementing the change
I found that the method was adding no value as the instance method
`InstantSource.withZone(ZoneId)` achieves the same outcome, so I
omitted it.

The Mac test failure appears to be unconnected to the change.

Thanks for any and all reviews!
Stephen

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v4]

[Mandy Chung]

On Thu, 13 May 2021 20:38:20 GMT, Lance Andersen  wrote:

>> HI all,
>> 
>> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
>> 
>> The fix adds a no-arg constructor which TestNG requires.
>> 
>> The change allows the test to run with the current jtreg as well as the 
>> upcoming jtreg
>> 
>> 
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adjust imports and spacing

Marked as reviewed by mchung (Reviewer).

-

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v4]

[Lance Andersen]

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

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

  Adjust imports and spacing

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4009/files
  - new: https://git.openjdk.java.net/jdk/pull/4009/files/a36e9f51..2e8f6c97

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

  Stats: 33 lines in 1 file changed: 16 ins; 8 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4009.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass

[Mandy Chung]

Maybe you were thinking 
`jdk.internal.reflect.ReflectUtil::isVMAnonymousClass` that is now 
removed by this patch [1].


[1] 
https://github.com/openjdk/jdk/pull/3974/files#diff-1af3026a3b4942af3ebe6a4df02f8952fb9d51bf93336a2f7f93ce175d047574


On 5/13/21 12:03 PM, Brian Goetz wrote:
Thanks for checking.  I thought I remembered something like this 
somewhere, but it may be that you already fixed such tests when you 
did hidden classes?  In any case, seems like we're all good now.


Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:
I did a search on java.base and the tests on `String::indexOf` and 
`String::contains` of a slash and don't spot any such test.  The JDK 
has no use of VM anonymous class.   If the test is trying to 
determine if it's lambda proxy class, it should be converted to call 
`Class::isHidden` but testing of the class name containing a slash is 
still valid (I haven't found any of such test though).


Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name 
to see if it contains a slash, especially tests, but which don’t say 
“anonymous”.  Did you do a search for these idioms too, which are now dead 
tests?

Sent from my iPad


On May 11, 2021, at 8:59 AM, Harold Seigel  wrote:

Please review this large change to remove Unsafe::defineAnonymousClass().  The 
change removes dAC relevant code and changes a lot of tests.  Many of the 
changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
the tests were modified to use hidden classes, others were deleted because 
either similar hidden classes tests already exist or they tested dAC specific 
functionality, such as host classes.

This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and 
Mach5 tiers 3-7 on Linux x64.

Thanks, Harold

-

Commit messages:
- 8243287: Removal of Unsafe::defineAnonymousClass

Changes:https://git.openjdk.java.net/jdk/pull/3974/files
Webrev:https://webrevs.openjdk.java.net/?repo=jdk=3974=00
  Issue:https://bugs.openjdk.java.net/browse/JDK-8243287
  Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod
  Patch:https://git.openjdk.java.net/jdk/pull/3974.diff
  Fetch: git fetchhttps://git.openjdk.java.net/jdk  pull/3974/head:pull/3974

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v3]

[Lance Andersen]

On Thu, 13 May 2021 18:53:25 GMT, Mandy Chung  wrote:

>> Lance Andersen has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Update copyright typo
>
> test/jdk/tools/jmod/hashes/HashesTest.java line 90:
> 
>> 88: }
>> 89: this.mods = dest.resolve("mods");
>> 90: Path srcDir = dest.resolve("src");
> 
> you can just get rid of this local variable and do this in line 92:
> 
>  this.builder = new ModuleInfoMaker(dest.resolve("src"));

Yes I can do that and realized that after I pushed the update

> test/jdk/tools/jmod/hashes/HashesTest.java line 387:
> 
>> 385: if (hashes != null) {
>> 386: hashes.names().stream().sorted().forEach(n ->
>> 387: System.out.format("  %s %s%n", n, 
>> toHex(hashes.hashFor(n)))
> 
> Nit: the extra whitespaces in line 384 and 387 may be added by IDE.  Can you 
> revert it.

Intellij did that.  I can tweak

-

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v2]

[Lance Andersen]

On Thu, 13 May 2021 18:26:52 GMT, Lance Andersen  wrote:

>> HI all,
>> 
>> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
>> 
>> The fix adds a no-arg constructor which TestNG requires.
>> 
>> The change allows the test to run with the current jtreg as well as the 
>> upcoming jtreg
>> 
>> 
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updates based on feedback and additional cleanup

I can look to revert the imports, that was an optimization via Intellij

-

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

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

[Rémi Forax]

On Tue, 4 May 2021 16:41:44 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

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 127:

> 125: Stream.of(labels).forEach(SwitchBootstraps::verifyLabel);
> 126: 
> 127: return new TypeSwitchCallSite(invocationType, labels);

See why below

  MethodHandle target = MethodHandles.insertArguments(DO_SWITCH, 2, labels);
  return new ConstantCallsite(target);

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 134:

> 132: throw new IllegalArgumentException("null label found");
> 133: }
> 134: if (label.getClass() != Class.class &&

store `label.getClass` in a local variable,
it's too bad that you can no use pattern matching here :)

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 141:

> 139: }
> 140: 
> 141: static class TypeSwitchCallSite extends ConstantCallSite {

That's weird, having a callsite extending MutableCallSite is expected but 
having a callsite extending constant callsite is useless because you can not 
change it after being constructed.

As an interesting trivia, the VM does not store the CallSite returned by the 
BSM, but only the target inside it.
So there is no point of keeping a ConstantCallSite around.  

So `doSwitch()` should be static and takes the array of Object[] as parameter, 
will array will be injected with an insertArguments().

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 157:

> 155: Class targetClass = target.getClass();
> 156: for (int i = startIndex; i < labels.length; i++) {
> 157: if (labels[i] instanceof Class) {

label[i] should be stored is a local variable and
using `instanceof Class c` (like the other instanceof below) will make the 
code more readable

-

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

Re: ReversibleCollection proposal

[Anthony Vanelverdinghe]

Hi Stuart

Will EnumSet also be updated to implement ReversibleSet? Or will it be updated 
to implement NavigableSet [1] independently of this enhancement?

I'd also like to propose adding `ReversibleSet::of` factory methods. This is 
something I miss having on SortedSet occasionally, but ReversibleSet would 
actually be a better fit for such methods.

Kind regards, Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-6278287

On Friday, April 16, 2021 19:40 CEST, Stuart Marks  
wrote:

> This is a proposal to add a ReversibleCollection interface to the Collections
> Framework. I'm looking for comments on overall design before I work on 
> detailed
> specifications and tests. Please send such comments as replies on this email 
> thread.
>
> Here's a link to a draft PR that contains the code diffs. It's prototype 
> quality,
> but it should be good enough to build and try out:
>
>  https://github.com/openjdk/jdk/pull/3533
>
> And here's a link to a class diagram showing the proposed additions:
>
>
> https://cr.openjdk.java.net/~smarks/ReversibleCollection/ReversibleCollectionDiagram.pdf
>
> Thanks,
>
> s'marks
>
>
> # Ordering and Reversibility
>
>
> A long-standing concept that's been missing from collections is that of the
> positioning, sequencing, or arrangement of elements as a structural property 
> of a
> collection. (This is sometimes called the "iteration order" of a collection.) 
> For
> example, a HashSet is not ordered, but a List is. This concept is mostly not
> manifested in the collections API.
>
> Iterating a collection produces elements one after another in *some* 
> sequence. The
> concept of "ordered" determines whether this sequence is defined or whether 
> it's a
> coincidence of implementation. What does "having an order" mean? It implies 
> that
> there is a first element and that each element has a successor. Since 
> collections
> have a finite number of elements, it further implies that there is a last 
> element
> that has no successor. However, it is difficult to discern whether a 
> collection has
> a defined order. HashSet generally iterates its elements in the same undefined
> order, and you can't actually tell that it's not a defined order.
>
> Streams do have a notion of ordering ("encounter order") and this is 
> preserved,
> where appropriate, through the stream pipeline. It's possible to detect this 
> by
> testing whether its spliterator has the ORDERED characteristic. Any 
> collection with
> a defined order will have a spliterator with this characteristic. However, 
> this is
> quite a roundabout way to determine whether a collection has a defined order.
> Furthermore, knowing this doesn't enable any additional operations. It only 
> provides
> constraints on the stream's implementations (keeping the elements in order) 
> and
> provides stronger semantics for certain operations. For example, findFirst() 
> on an
> unordered stream is the same as findAny(), but actually finds the first 
> element if
> the stream is ordered.
>
> The concept of ordering is thus present in the system but is surfaced only in 
> a
> fairly indirect way. We can strengthen abstraction of ordering by making a few
> observations and considering their implications.
>
> Given that there is a first element and a last element, the sequence of 
> elements has
> two ends. It's reasonable to consider operations (add, get, remove) on either 
> end.
> Indeed, the Deque interface has a full complement of operations at each end. 
> This is
> an oft-requested feature on various other collections.
>
> Each element except for the last has a successor, implying that each element 
> except
> for the first has a predecessor. Thus it's reasonable to consider iterating 
> the
> elements from first to last or from last to first (that is, in forward or 
> reverse
> order). Indeed, the concept of iterating in reverse order appears already in 
> bits
> and pieces in particular places around the collections:
>
>   - List has indexOf() and lastIndexOf()
>   - Deque has removeFirstOccurrence() and removeLastOccurrence()
>   - List has a ListIterator with hasPrevious/previous methods
>   - Deque and NavigableSet have descendingIterator methods
>
> Given an ordered collection, though, there's no general way to iterate it in 
> reverse
> order. Reversed iteration isn't the most common operation, but there are some
> frequent use cases, such as operating on elements in most-recently-added 
> order.
> Questions and bug reports about this have come up repeatedly over the years.
>
> Unfortunately, iterating in reverse order is much harder than iterating in 
> forward
> order. There are a variety of ways to iterate in forward order. For example, 
> given a
> List, one can do any of the following:
>
>  for (var e : list) { ... }
>  list.forEach(...)
>  list.stream()
>  list.toArray()
>
> However, to iterate a list in reverse order, one must use an explicit loop 
> over
> ListIterator:
>
>  for (var it = 

Re: [External] : Re: ReversibleCollection proposal

[Anthony Vanelverdinghe]

Hi Rémi

The discussion "new types? or new default methods?" is indeed a valid one. In 
fact, I think this choice has already been made once in favor of a default 
method, when adding `List::sort` was chosen over adding `SortedList` (though I 
imagine that choice was a no-brainer).

In this case I prefer adding new types though, so I wanted to share my take on 
this.

In the following diagram (see [1] in case it got mangled), I've tried to 
arrange all relevant Collection types by their defining characteristics:

|- Collection 
---|
|unorderedordered   sorted   
distinctdistinct & ordereddistinct & sorted |
|Set
 |
| Queue = (get|remove)First + addLast   PriorityQueue   
 |
| Stack = (add|get|remove)Last  
 |
| Deque = Queue + Stack + addFirst  
 LinkedHashSet SortedSet |
| List = Deque + (add|get|remove)IndexedList::sort  
   NavigableSet  |
||

As I see it, there are a couple of issues with this:
* conceptually, every List is a Deque, but List doesn't extend Deque. If it 
would, all uses of List (e.g. as a parameter type in APIs) where the indexed 
access isn't required could be replaced with Deque instead. Or e.g. when you 
need to take a stack as argument: currently Deque is the recommended parameter 
type, but that means you can't just pass a List to the method as-is. With RC, 
you'd be able to use that as parameter type and pass both Deque and List.
* LinkedHashSet and SortedSet lack a common ancestor which asserts "this is an 
ordered set". So when defining an API, I'm forced to choose between SortedSet, 
which is more specific than I want, or List, which is more generic than I want 
(usually I go with SortedSet, because enforcing something through Javadoc isn't 
very reliable (cf. Collectors::toList: even though the spec clearly says the 
result might not be modifiable, people tend to simply assume it is)).

Now with RC/RS these issues would be solved, where RC is ~ Deque + reversed, 
and RS ~ Deque + distinct + reversed. In terms of the diagram, they group 
together a bunch of closely-related Collection types (see [1] if needed):

|- Collection 
---|
|unorderedordered   sorted   
distinctdistinct & ordereddistinct & sorted |
|Set
 |
| Queue = (get|remove)First + addLast   PriorityQueue   
 |
| Stack = (add|get|remove)Last  
 |
||- ReversibleCollection ---|   
|- ReversibleSet ---||
||Deque = Queue + Stack + addFirst  |   
|LinkedHashSet SortedSet||
||List = Deque + (add|get|remove)IndexedList::sort  |   
|  NavigableSet ||
||--|   
|---||
||

On Wednesday, May 12, 2021 13:22 CEST, fo...@univ-mlv.fr wrote:

> First, i think we have overlooked ReversibleMap, if you have a LinkedHashMap, 
> the keySet should be a ReversibleSet.

I'm not sure what you meant, but the PR already defines `LinkedHashMap::keySet` 
as `public ReversibleSet keySet()`.

> Again, we have seen that introducing those interfaces
> - is not source backward compatible thus it will be painful for some of our 
> users,
>   I believe NavigableSet/NavigableMap did not have that issue because only 
> one existing implementation per interface was retrofitted to implement those 
> interfaces, TreeSet for NavigableSet and TreeMap for NavigableMap.
>   ConcurrentSkipListSet/ConcurrentSkipListMap were added at the same time, so 
> there was no existing code doing a lub (lowest upper bound) between TreeSet 
> and ConcurrentSkipListSet.
>   Here, they are a lot of implementations that will implement be retrofitted 
> to 

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass

[Brian Goetz]

Thanks for checking.  I thought I remembered something like this 
somewhere, but it may be that you already fixed such tests when you did 
hidden classes?  In any case, seems like we're all good now.


Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:
I did a search on java.base and the tests on `String::indexOf` and 
`String::contains` of a slash and don't spot any such test.  The JDK 
has no use of VM anonymous class. If the test is trying to determine 
if it's lambda proxy class, it should be converted to call 
`Class::isHidden` but testing of the class name containing a slash is 
still valid (I haven't found any of such test though).


Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name 
to see if it contains a slash, especially tests, but which don’t say 
“anonymous”.  Did you do a search for these idioms too, which are now dead 
tests?

Sent from my iPad


On May 11, 2021, at 8:59 AM, Harold Seigel  wrote:

Please review this large change to remove Unsafe::defineAnonymousClass().  The 
change removes dAC relevant code and changes a lot of tests.  Many of the 
changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
the tests were modified to use hidden classes, others were deleted because 
either similar hidden classes tests already exist or they tested dAC specific 
functionality, such as host classes.

This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and 
Mach5 tiers 3-7 on Linux x64.

Thanks, Harold

-

Commit messages:
- 8243287: Removal of Unsafe::defineAnonymousClass

Changes:https://git.openjdk.java.net/jdk/pull/3974/files
Webrev:https://webrevs.openjdk.java.net/?repo=jdk=3974=00
  Issue:https://bugs.openjdk.java.net/browse/JDK-8243287
  Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod
  Patch:https://git.openjdk.java.net/jdk/pull/3974.diff
  Fetch: git fetchhttps://git.openjdk.java.net/jdk  pull/3974/head:pull/3974

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v3]

[Mandy Chung]

On Thu, 13 May 2021 18:37:21 GMT, Lance Andersen  wrote:

>> HI all,
>> 
>> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
>> 
>> The fix adds a no-arg constructor which TestNG requires.
>> 
>> The change allows the test to run with the current jtreg as well as the 
>> upcoming jtreg
>> 
>> 
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update copyright typo

Looks like the imports and whitespace are changed by IDE??   can you please 
revert these unintentional change.

Otherwise, the change looks good.

test/jdk/tools/jmod/hashes/HashesTest.java line 90:

> 88: }
> 89: this.mods = dest.resolve("mods");
> 90: Path srcDir = dest.resolve("src");

you can just get rid of this local variable and do this in line 92:

 this.builder = new ModuleInfoMaker(dest.resolve("src"));

test/jdk/tools/jmod/hashes/HashesTest.java line 387:

> 385: if (hashes != null) {
> 386: hashes.names().stream().sorted().forEach(n ->
> 387: System.out.format("  %s %s%n", n, 
> toHex(hashes.hashFor(n)))

Nit: the extra whitespaces in line 384 and 387 may be added by IDE.  Can you 
revert it.

-

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass

[Mandy Chung]

I did a search on java.base and the tests on `String::indexOf` and 
`String::contains` of a slash and don't spot any such test.  The JDK has 
no use of VM anonymous class.   If the test is trying to determine if 
it's lambda proxy class, it should be converted to call 
`Class::isHidden` but testing of the class name containing a slash is 
still valid (I haven't found any of such test though).


Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name 
to see if it contains a slash, especially tests, but which don’t say 
“anonymous”.  Did you do a search for these idioms too, which are now dead 
tests?

Sent from my iPad


On May 11, 2021, at 8:59 AM, Harold Seigel  wrote:

Please review this large change to remove Unsafe::defineAnonymousClass().  The 
change removes dAC relevant code and changes a lot of tests.  Many of the 
changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
the tests were modified to use hidden classes, others were deleted because 
either similar hidden classes tests already exist or they tested dAC specific 
functionality, such as host classes.

This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and 
Mach5 tiers 3-7 on Linux x64.

Thanks, Harold

-

Commit messages:
- 8243287: Removal of Unsafe::defineAnonymousClass

Changes: https://git.openjdk.java.net/jdk/pull/3974/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3974=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8243287
  Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3974.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v3]

[Lance Andersen]

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

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

  Update copyright typo

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4009/files
  - new: https://git.openjdk.java.net/jdk/pull/4009/files/201b9a2b..a36e9f51

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

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

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Lance Andersen]

On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen  wrote:

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

The latest commit takes into account the feedback received

> I also think it's good to fix this properly. Each test wants to run in an 
> unique directory. Another solution is to make the fields non-final and add a 
> new `@BeforeMethod` method to generate the unique pathname for each test. I 
> think this seems a clean way (I sent you a patch to checkout).

The latest commit leverages @BeforeMethod vs an inner class and also includes 
some additional minor cleanup

-

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v2]

[Lance Andersen]

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

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

  Updates based on feedback and additional cleanup

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4009/files
  - new: https://git.openjdk.java.net/jdk/pull/4009/files/12c7c4b1..201b9a2b

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

  Stats: 182 lines in 1 file changed: 7 ins; 41 del; 134 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4009.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009

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

Re: JEP draft: Scope Locals

[Pedro Lamarão]

Em qui., 13 de mai. de 2021 às 13:36, Alan Snyder 
escreveu:


> >> Is this a common practice? Doesn’t it point to a deficiency in the
> library API?
> >
> > Not necessarily. For example, say you want to give access to a particular
> > resource (a log stream, say) to trusted callees. Nobody AFAIK passes a
> log
> > stream as an explicit argument through business logic because that's just
> > too much clutter.
> >
>
> That sounds interesting, but I’m not following how scope locals would be
> used to solve this problem.
>

It appears to me that, in this context, scoped locals is inserting itself
inside a hierarchy of "overrides".
We already have mechanisms which provide "context" capable of carrying
"overrides", such as process environment variables and JVM system
properties.
Those are too global for certain uses such as "overriding" a single method
call.
So, in a great hierarchy of "overrides", we would have, from the largest to
the smallest: process environment variables > JVM system properties > *scoped
locals* > method call parameters.
Things like these are recurrent in the design of "I/O cancelability", where
people are always discovering the need for things like "timeout scopes".
It could be argued that "cancelable" APIs must have some "cancellation
token " parameter.
But there are cases where some domain interface, which was never designed
with this in mind, must be implemented over network protocols.
This is the case when you must implement a KeyStore or a Cipher with
remotely managed private keys.
Since you cannot alter KeyStore to pass a "cancellation token" to single
methods, you must insert the token as a sort of "override" thing in some
parallel storage.

-- 
Pedro Lamarão
https://www.prodist.com.br
Securing Critical Systems
Tel: +55 11 4380-6585

Antes de imprimir esta mensagem e seus anexos, certifique-se que seja
realmente necessário.
Proteger o meio ambiente é nosso dever.
Before printing this e-mail or attachments, be sure it is necessary.
It is in our hands to protect the environment.

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mandy Chung]

On Thu, 13 May 2021 18:11:30 GMT, Mark Reinhold  wrote:

>> Please review this implementation of JEP 403 
>> (https://openjdk.java.net/jeps/403).
>> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 
>> on 
>> {linux,macos,windows}-x64 and {linux,macos}-aarch64.
>
> Mark Reinhold has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove stray mentions of the jdk.module.illegalAccess property

There are a few tests with `--illegal-access=deny` for example

test/jdk/java/lang/ModuleTests/BasicModuleTest.java
test/jdk/java/lang/instrument/RedefineModuleTest.java
test/jdk/java/lang/invoke/CallerSensitiveAccess.java
test/jdk/java/lang/reflect/AccessibleObject/ - a few tests
test/jdk/jdk/modules/open/Basic.java
test/jdk/tools/launcher/modules/addexports/manifest/AddExportsAndOpensInManifest.java


It would be good to remove these references to `--illegal-access` option in 
this patch too.

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mark Reinhold]

On Thu, 13 May 2021 17:37:42 GMT, Harold Seigel  wrote:

>> Mark Reinhold has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove stray mentions of the jdk.module.illegalAccess property
>
> src/hotspot/share/runtime/arguments.cpp line 2434:
> 
>> 2432: // -agentlib and -agentpath
>> 2433: } else if (match_option(option, "-agentlib:", ) ||
>> 2434:   (is_absolute_path = match_option(option, "-agentpath:", 
>> ))) {
> 
> I think jdk.module.illegalAccess can also be removed from lines 1332 and 2064 
> of arguments.cpp.
> Thanks, Harold

Good point — fixed.  Thanks.

-

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v2]

[Mark Reinhold]

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

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

  Remove stray mentions of the jdk.module.illegalAccess property

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4015/files
  - new: https://git.openjdk.java.net/jdk/pull/4015/files/4e2cbf93..719ff4ee

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

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

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

Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

[Harold Seigel]

On Thu, 13 May 2021 17:14:36 GMT, Mark Reinhold  wrote:

> Please review this implementation of JEP 403 
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.

src/hotspot/share/runtime/arguments.cpp line 2434:

> 2432: // -agentlib and -agentpath
> 2433: } else if (match_option(option, "-agentlib:", ) ||
> 2434:   (is_absolute_path = match_option(option, "-agentpath:", 
> ))) {

I think jdk.module.illegalAccess can also be removed from lines 1332 and 2064 
of arguments.cpp.
Thanks, Harold

-

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

RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

[Mark Reinhold]

Please review this implementation of JEP 403 
(https://openjdk.java.net/jeps/403).
Alan Bateman is the original author of almost all of it.  Passes tiers 1-3 on 
{linux,macos,windows}-x64 and {linux,macos}-aarch64.

-

Commit messages:
 - 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals

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

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Mandy Chung]

On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen  wrote:

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

I also think it's good to fix this properly.Each test wants to run in an 
unique directory.Another solution is to make the fields non-final and add a 
new `@BeforeMethod` method to generate the unique pathname for each test.   I 
think this seems a clean way (I sent you a patch to checkout).

-

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

Integrated: 8266162: Remove JPackage duplicate tests

[Alexey Semenyuk]

On Wed, 12 May 2021 20:34:08 GMT, Alexey Semenyuk  wrote:

> 8266162: Remove JPackage duplicate tests

This pull request has now been integrated.

Changeset: f3c6cda4
Author:Alexey Semenyuk 
URL:   
https://git.openjdk.java.net/jdk/commit/f3c6cda47631cc123dbcddbfb627dc05cf7bc13b
Stats: 39 lines in 2 files changed: 39 ins; 0 del; 0 mod

8266162: Remove JPackage duplicate tests

Reviewed-by: almatvee, herrick

-

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

Integrated: 8258795: Update IANA Language Subtag Registry to Version 2021-05-11

[Naoto Sato]

On Wed, 12 May 2021 16:28:54 GMT, Naoto Sato  wrote:

> Please review the changes to the subject issue. This is to incorporate the 
> latest language subtag registry definition into the JDK.

This pull request has now been integrated.

Changeset: a259ab4a
Author:Naoto Sato 
URL:   
https://git.openjdk.java.net/jdk/commit/a259ab4a8d163ff924ba56c5da5395cec0d8c350
Stats: 340 lines in 2 files changed: 327 ins; 0 del; 13 mod

8258795: Update IANA Language Subtag Registry to Version 2021-05-11

Reviewed-by: joehw

-

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

Re: RFR: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert. [v2]

[Mitsuru Kariya]

> Fix `SerialBlob.setBytes(long pos, byte[] bytes, int offset, int length)` in 
> the following cases:
> 
> 1. `pos - 1 + bytes.length - offset > this.length() && pos - 1 + length <= 
> this.length()`
>The original implementation throws `ArrayIndexOutOfBoundsException` but 
> this case should end successfully.
>(test31)
> 
> 2. `pos - 1 + length > this.length()`
>The original implementation throws `ArrayIndexOutOfBoundsException` but 
> this case should end successfully. *1
>(test32)
> 
> 3. `pos == this.length() + 1`
>The original implementation throws `SerialException` but this case should 
> end successfully. *2
>(test33)
> 
> 4. `length < 0`
>The original implementation throws `ArrayIndexOutOfBoundsException` but 
> this case should throw `SerialException`.
>(test34)
> 
> Additionally, fix `SerialClob.setString(long pos, String str, int offset, int 
> length)` in the following cases:
> 
> 1. `offset > str.length()`
>The original implementaion throws `StringIndexOutOfBoundsException` but 
> this case should throw `SerialException`.
>(test39)
> 
> 2. `pos - 1 + str.length() - offset > this.length() && pos - 1 + length <= 
> this.length()`
>The original implementation throws `ArrayIndexOutOfBoundsException` but 
> this case should end successfully.
>(test32)
> 
> 3. `pos - 1 + length > this.length()`
>The original implementaion throws `SerialException` but this case should 
> end successfully. *3
>(test40)
> 
> 4. `pos == this.length() + 1`
>The original implementaion throws `SerialException` but this case should 
> end successfully. *4
>(test41)
> 
> 5. `length < 0`
>The original implementation throws `StringIndexOutOfBoundsException` but 
> this case should throw `SerialException`.
>(test42)
> 
> 
> The javadoc has also been modified according to the above.
> 
> *1 The documentation of `Blob.setBytes()` says, "If the end of the Blob value 
> is reached while writing the array of bytes, then the length of the Blob 
> value will be increased to accommodate the extra bytes."
> 
> *2 The documentation of `Blob.setBytes()` says, "If the value specified for 
> pos is greater than the length+1 of the BLOB value then the behavior is 
> undefined."
>So, it should work correctly when pos == length+1 of the BLOB value.
> 
> *3 The documentation of `Clob.setString()` says, "If the end of the Clob 
> value is eached while writing the given string, then the length of the Clob 
> value will be increased to accommodate the extra characters."
> 
> *4 The documentation of `Clob.setString()` says, "If the value specified for 
> pos is greater than the length+1 of the CLOB value then the behavior is 
> undefined."
>So, it should work correctly when pos == length+1 of the CLOB value.

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

  Add check: ensure length >= 0

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4001/files
  - new: https://git.openjdk.java.net/jdk/pull/4001/files/8849de96..6a0cc1ad

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

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

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

Re: JEP draft: Scope Locals

[Alan Snyder]

> On May 13, 2021, at 2:03 AM, Andrew Haley  wrote:
> 
> On 5/12/21 8:12 PM, Alan Snyder wrote:
>> From the motivation section:
>> 
>>> So you want to invoke a method |X| in a library that later calls back into 
>>> your code. In your callback you need some context, perhaps a transaction ID 
>>> or some |File| instances. However, |X| provides no way to pass a reference 
>>> through their code into your callback. What are you to do?
>> 
>> When I read this, my first thought was… pass a callback that is bound to the 
>> context.
>> 
>> I suppose the example you have in mind has a fixed callback.
> 
> Fixed? It's a callback, of some form.
> 

What I meant was that the callback has a larger scope than the method call. 
Otherwise, you could bind the context to the callback object.


>> Is this a common practice? Doesn’t it point to a deficiency in the library 
>> API?
> 
> Not necessarily. For example, say you want to give access to a particular
> resource (a log stream, say) to trusted callees. Nobody AFAIK passes a log
> stream as an explicit argument through business logic because that's just
> too much clutter.
> 

That sounds interesting, but I’m not following how scope locals would be used 
to solve this problem.


>> Is this feature just a workaround for poorly designed libraries, or are 
>> there other examples?
> 
> It's not really about poor design as much as separation of concerns.
> Intermediate libraries have no way to know what might need to be passed
> to callees, and in many cases it's better isolation if they don't get to
> find out. The latter is especially true of passing security permissions.
> 
> Also, there is evolution of libraries: with scope locals you don't need
> to change library interfaces to add useful capabilities like logging.
> 
> -- 
> 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: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Lance Andersen]

On Thu, 13 May 2021 11:24:34 GMT, Chris Hegarty  wrote:

> The non-static state in this test class is initialized for each of the static 
> testXXX scenarios. An alternative could be to move said state (four fields) 
> into a static inner class, and have each of the testXXX scenarios create an 
> instance of that class with the test-specific path. That would also allow the 
> addition of the no-args public constructor to HashesTest, and the testXXX 
> methods to be made non-static.

I had originally thought about introducing an inner class but decided that 
given TestNG needs access to  the default constructor, I chose that route vs. 
doing more of an update.  I can revisit this though

-

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

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Lance Andersen]

On Thu, 13 May 2021 11:11:23 GMT, Alan Bateman  wrote:

>> HI all,
>> 
>> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
>> 
>> The fix adds a no-arg constructor which TestNG requires.
>> 
>> The change allows the test to run with the current jtreg as well as the 
>> upcoming jtreg
>> 
>> 
>> Best
>> Lance
>
> test/jdk/tools/jmod/hashes/HashesTest.java line 94:
> 
>> 92: lib= null;
>> 93: builder=null;
>> 94: }
> 
> This looks like a workaround. Can you instead see if the fields can be 
> changed to non-final and place the constructor with a method that has the 
> `@BeforeClass` annotation?

I can look to update the test.  As I mentioned in my reply to Chris, I had 
thought about introducing an inner class bug decided to go the route of the 
default/no-arg constructor as it required fewer changes.

-

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

Re: Fuzzing the Java core libs

[Fabian Meumertzheim]

On Thu, May 13, 2021 at 1:22 PM Alan Bateman 
wrote:

> The workflow is shown on the Vulnerability Group page [1]. There isn't a
> repo that you can test commits on before the publication date.
>
> -Alan
>
> [1] https://openjdk.java.net/groups/vulnerability/
>

Based on the information on that page, there should be no conflict between
the OpenJDK and the OSS-Fuzz policies regarding disclosures (
https://google.github.io/oss-fuzz/getting-started/bug-disclosure-guidelines/
).

Is there anyone who would volunteer to receive the finding reports? Every
report comes with a stack trace and the exact input that reproduces the
finding with the fuzzer, i.e., is immediately actionable.

Examples of such reports for fixed bugs can be found at
https://bugs.chromium.org/p/oss-fuzz/issues/list?q=proj%3A%22json-sanitizer%22%20OR%20proj%3A%22fastjson2%22%20OR%20proj%3A%22jackson-core%22%20OR%20proj%3A%22jackson-dataformats-binary%22%20or%20proj%3A%22apache-commons%22=1

Integrated: JDK-8266552 Technical corrections to java/util/random/package-info.java

[Jim Laskey]

On Wed, 5 May 2021 12:41:50 GMT, Jim Laskey  wrote:

> The author (Guy Steele) of https://bugs.openjdk.java.net/browse/JDK-8193209 
> with others have post-integration reviewed commentary (javadoc) and has 
> submitted technical corrections.

This pull request has now been integrated.

Changeset: b4371e9b
Author:Jim Laskey 
URL:   
https://git.openjdk.java.net/jdk/commit/b4371e9bcaa1c8aa394b5eca409c5afc669cc146
Stats: 139 lines in 11 files changed: 10 ins; 20 del; 109 mod

8266552: Technical corrections to java/util/random/package-info.java

Reviewed-by: darcy

-

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

Re: RFR: 8266310: deadlock while loading the JNI code

[David Holmes]

Hi Aleksei,

On 13/05/2021 9:54 pm, Aleksei Voitylov wrote:

Hi David,

On 12/05/2021 10:56, David Holmes wrote:

Hi Aleksei,

On 11/05/2021 11:19 pm, 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 ClassLoader per-name locking map has a number of problems so I'm
not sure using it as a model is a good idea. In particular your new
map only grows, with entries never being removed AFAICS.

OK. I'm working on a version of a patch which addresses this concern.


This is a difficult deadlock to solve. Even using a per-entry lock it
isn't obvious to me that you can't still get a deadlock.


I believe the patch solves the deadlock when different libraries are in
play. Yes, it's still possible to get a deadlock when a library with a
JNI_Onload instantiates a class that requires the same library to be
loaded. Is this a valid use case to begin with for future use and why
such a recursion should be allowed? If not, I'd like to file a CSR for
JNI specification with the intent to forbid such behavior. It's obvious
there are no current users of such behavior because it deadlocks.


So in the current case we have:

Thread 1:
 - loads native lib and acquires load-library lock
 - JNI_Onload tries to initialize class X but can't get the class init lock

Thread 2:
 - initializes class X and has the class init lock
 -  tries to load a native library and can't get the 
load-library lock


If the library lock is finer granularity then we can reduce the risk of 
deadlock but not remove it.


I don't think we can try to forbid anything through the JNI 
specification in such a case. This is really just one of a number of 
class initialization deadlocks that are possible. At most the spec could 
caution against such things, but there is no way to detect it and so 
"forbid" it. And you can construct similar deadlocks without class 
initialization being involved.




My own thoughts on this problem were that we should not be calling
JNI_OnLoad with any lock held. But that risks use of a library in a
separate thread before the JNI+OnLoad has completed. As I said this is
a difficult deadlock to solve - and may not have a complete solution.


Thanks, this is a very appealing idea. While it's true dlopen on Linux
and Mac OS are explicitly marked thread safe (and LoadLibrary on Windows
can probably be assumed so as well, though is not marked so in Microsoft
documentation), all implementations use reference counting techniques,
and the number of calls to open has to be equal to the number of calls
to close. I don't know how to achieve that without locks.


Not every problem has a solution :) If JNI_OnLoad has to execute 
atomically with respect to loading a library then there will always be a 
deadlock potential. The only complete solution would not hold a lock 
while JNI_OnLoad is executed, but that completely changes the semantics 
of native library loading.


Cheers,
David


-Aleksei



David
-


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.

-

Commit messages:
   - JDK-8266310: deadlock while loading the JNI code
   - JDK-8266310: deadlock while loading the JNI code

Changes: https://git.openjdk.java.net/jdk/pull/3976/files
   Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3976=00
    Issue: https://bugs.openjdk.java.net/browse/JDK-8266310
    Stats: 475 lines in 6 files changed: 456 ins; 0 del; 19 mod
    Patch: https://git.openjdk.java.net/jdk/pull/3976.diff
    Fetch: git fetch https://git.openjdk.java.net/jdk
pull/3976/head:pull/3976

PR: 

Re: RFR: 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0() [v2]

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

> Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 and 
> https://github.com/openjdk/jdk/pull/2212 it appears, that in `j.l.Class` 
> expressions like
> 
> String str = baseName.replace('.', '/') + '/' + name;
> 
> are not compiled into invokedynamic-based code, but into one using 
> `StringBuilder`.
> 
> This happens due to some bootstraping issues. Currently the bytecode for the 
> last (most often used) branch of `Class.descriptorString()` looks like
> 
> public sb()Ljava/lang/String;
>L0
> LINENUMBER 21 L0
> NEW java/lang/StringBuilder
> DUP
> INVOKESPECIAL java/lang/StringBuilder. ()V
> ASTORE 1
>L1
> LINENUMBER 23 L1
> ALOAD 1
> LDC "a"
> INVOKEVIRTUAL java/lang/StringBuilder.append 
> (Ljava/lang/String;)Ljava/lang/StringBuilder;
> POP
>L2
> LINENUMBER 24 L2
> ALOAD 1
> LDC "b"
> INVOKEVIRTUAL java/lang/StringBuilder.append 
> (Ljava/lang/String;)Ljava/lang/StringBuilder;
> POP
>L3
> LINENUMBER 26 L3
> ALOAD 1
> INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
> ARETURN
> 
> Here the `StringBuilder` is created with default constructor and then expands 
> if necessary while appending. 
> 
> This can be improved by manually allocating `StringBuilder` of exact size. 
> The benchmark demonstrates measurable improvement:
> 
> @State(Scope.Benchmark)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ClassDescriptorStringBenchmark {
> 
> private final Class clazzWithShortDescriptor = Object.class;
> private final Class clazzWithLongDescriptor = getClass();
> 
> @Benchmark
> public String descriptorString_short() {
> return clazzWithShortDescriptor.descriptorString();
> }
> 
> @Benchmark
> public String descriptorString_long() {
> return clazzWithLongDescriptor.descriptorString();
> }
> }
> 
> 
> 
> original
> -Xint
>Mode Score Error   
> Units
> descriptorString_long  avgt  6326.478 ± 107.251   
> ns/op
> descriptorString_short avgt  5220.729 ± 103.545   
> ns/op
> descriptorString_long:·gc.alloc.rate.norm  avgt   528.089 ±   0.021
> B/op
> descriptorString_short:·gc.alloc.rate.norm avgt   232.036 ±   0.015
> B/op
> 
> -XX:TieredStopAtLevel=1
>Mode  ScoreError   
> Units
> descriptorString_long  avgt230.223 ±  1.254   
> ns/op
> descriptorString_short avgt164.255 ±  0.755   
> ns/op
> descriptorString_long:·gc.alloc.rate.norm  avgt528.046 ±  0.002
> B/op
> descriptorString_short:·gc.alloc.rate.norm avgt232.022 ±  0.001
> B/op
> 
> full
>Mode  Score Error   
> Units
> descriptorString_long  avgt 74.835 ±   0.262   
> ns/op
> descriptorString_short avgt 43.822 ±   0.788   
> ns/op
> descriptorString_long:·gc.alloc.rate.norm  avgt504.010 ±   0.001
> B/op
> descriptorString_short:·gc.alloc.rate.norm avgt208.004 ±   0.001
> B/op
> 
> 
> patched
> -Xint
>Mode  Score Error   
> Units
> descriptorString_long  avgt   4485.994 ±  60.173   
> ns/op
> descriptorString_short avgt   3949.965 ± 278.143   
> ns/op
> descriptorString_long:·gc.alloc.rate.norm  avgt336.051 ±   0.004
> B/op
> descriptorString_short:·gc.alloc.rate.norm avgt184.027 ±   0.010
> B/op
> 
> -XX:TieredStopAtLevel=1
>ModeScoreError   
> Units
> descriptorString_long  avgt  185.774 ±  1.100   
> ns/op
> descriptorString_short avgt  135.338 ±  1.066   
> ns/op
> descriptorString_long:·gc.alloc.rate.norm  avgt  336.030 ±  0.001
> B/op
> descriptorString_short:·gc.alloc.rate.norm avgt  184.019 ±  0.001
> B/op
> 
> full
>Mode  Score Error   
> Units
> descriptorString_long  avgt 42.864 ±   0.160   
> ns/op
> descriptorString_short avgt 27.255 ±   0.381   
> ns/op
> descriptorString_long:·gc.alloc.rate.norm  avgt224.005 ±   0.001
> B/op
> descriptorString_short:·gc.alloc.rate.norm avgt120.002 ±   0.001
> B/op
> 
> Same can be done also for Class.isHidden() branch in Class.descriptorString() 
> and for Class.getCanonicalName0()

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

  8266622: Remove pointless String.substring() call

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]

[Harold Seigel]

On Thu, 13 May 2021 12:31:44 GMT, Harold Seigel  wrote:

>> Please review this large change to remove Unsafe::defineAnonymousClass().  
>> The change removes dAC relevant code and changes a lot of tests.  Many of 
>> the changed tests need renaming.  I hope to do this in a follow up RFE.  
>> Some of the tests were modified to use hidden classes, others were deleted 
>> because either similar hidden classes tests already exist or they tested dAC 
>> specific functionality, such as host classes.
>> 
>> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
>> and Mach5 tiers 3-7 on Linux x64.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix Weak hidden comment

Thanks Ioi, Alan, Mandy, and David for reviewing this big change!

-

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

Integrated: 8243287: Removal of Unsafe::defineAnonymousClass

[Harold Seigel]

On Tue, 11 May 2021 12:50:31 GMT, Harold Seigel  wrote:

> Please review this large change to remove Unsafe::defineAnonymousClass().  
> The change removes dAC relevant code and changes a lot of tests.  Many of the 
> changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
> the tests were modified to use hidden classes, others were deleted because 
> either similar hidden classes tests already exist or they tested dAC specific 
> functionality, such as host classes.
> 
> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
> and Mach5 tiers 3-7 on Linux x64.
> 
> Thanks, Harold

This pull request has now been integrated.

Changeset: e14b0268
Author:Harold Seigel 
URL:   
https://git.openjdk.java.net/jdk/commit/e14b0268411bba8eb01bf6c477cc8743a53ffd1c
Stats: 3754 lines in 122 files changed: 75 ins; 3426 del; 253 mod

8243287: Removal of Unsafe::defineAnonymousClass

Reviewed-by: iklam, mchung, alanb, dholmes

-

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]

[Harold Seigel]

On Thu, 13 May 2021 07:19:03 GMT, David Holmes  wrote:

>> Harold Seigel has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix Weak hidden comment
>
> src/hotspot/share/oops/constantPool.hpp line 493:
> 
>> 491:   // object into a CONSTANT_String entry of an unsafe anonymous class.
>> 492:   // Methods internally created for method handles may also
>> 493:   // use pseudo-strings to link themselves to related metaobjects.
> 
> Is this comment wrong? Are psuedo-strings not used by anything now?

Thanks for looking at this.  I discussed pseudo strings with Coleen and we 
didn't find any use of them besides unsafe.DefineAnonymousClass.

-

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

Re: RFR: 8266162: Remove JPackage duplicate tests

[Andy Herrick]

On Wed, 12 May 2021 20:34:08 GMT, Alexey Semenyuk  wrote:

> 8266162: Remove JPackage duplicate tests

Marked as reviewed by herrick (Reviewer).

-

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]

[Harold Seigel]

On Wed, 12 May 2021 22:30:30 GMT, Mandy Chung  wrote:

>> Harold Seigel has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   test changes and small fixes
>
> src/hotspot/share/classfile/classLoaderData.cpp line 299:
> 
>> 297: }
>> 298: 
>> 299: // Weak hidden classes have their own ClassLoaderData that is marked to 
>> keep alive
> 
> `s/Weak/Non-strong/`

fixed.  thanks for finding it.

> test/hotspot/jtreg/runtime/HiddenClasses/StressHiddenClasses.java line 39:
> 
>> 37: import jdk.test.lib.compiler.InMemoryJavaCompiler;
>> 38: 
>> 39: public class StressHiddenClasses {
> 
> Since `StressClassLoadingTest` is revised to test hidden class, this test is 
> a subset of it.
> I think this can be removed as JDK-8265694 suggests.

Thanks Mandy. I will remove the test as the fix for JDK-8265694 after this 
change is pushed.

-

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]

[Harold Seigel]

> Please review this large change to remove Unsafe::defineAnonymousClass().  
> The change removes dAC relevant code and changes a lot of tests.  Many of the 
> changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
> the tests were modified to use hidden classes, others were deleted because 
> either similar hidden classes tests already exist or they tested dAC specific 
> functionality, such as host classes.
> 
> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
> and Mach5 tiers 3-7 on Linux x64.
> 
> Thanks, Harold

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

  fix Weak hidden comment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3974/files
  - new: https://git.openjdk.java.net/jdk/pull/3974/files/5246dd76..38675761

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

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

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

Re: RFR: 8266310: deadlock while loading the JNI code

[Aleksei Voitylov]

Hi David,

On 12/05/2021 10:56, David Holmes wrote:
> Hi Aleksei,
>
> On 11/05/2021 11:19 pm, 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 ClassLoader per-name locking map has a number of problems so I'm
> not sure using it as a model is a good idea. In particular your new
> map only grows, with entries never being removed AFAICS.
OK. I'm working on a version of a patch which addresses this concern.
>
> This is a difficult deadlock to solve. Even using a per-entry lock it
> isn't obvious to me that you can't still get a deadlock.

I believe the patch solves the deadlock when different libraries are in
play. Yes, it's still possible to get a deadlock when a library with a
JNI_Onload instantiates a class that requires the same library to be
loaded. Is this a valid use case to begin with for future use and why
such a recursion should be allowed? If not, I'd like to file a CSR for
JNI specification with the intent to forbid such behavior. It's obvious
there are no current users of such behavior because it deadlocks.

>
> My own thoughts on this problem were that we should not be calling
> JNI_OnLoad with any lock held. But that risks use of a library in a
> separate thread before the JNI+OnLoad has completed. As I said this is
> a difficult deadlock to solve - and may not have a complete solution.

Thanks, this is a very appealing idea. While it's true dlopen on Linux
and Mac OS are explicitly marked thread safe (and LoadLibrary on Windows
can probably be assumed so as well, though is not marked so in Microsoft
documentation), all implementations use reference counting techniques,
and the number of calls to open has to be equal to the number of calls
to close. I don't know how to achieve that without locks.

-Aleksei

>
> David
> -
>
>> 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.
>>
>> -
>>
>> Commit messages:
>>   - JDK-8266310: deadlock while loading the JNI code
>>   - JDK-8266310: deadlock while loading the JNI code
>>
>> Changes: https://git.openjdk.java.net/jdk/pull/3976/files
>>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3976=00
>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8266310
>>    Stats: 475 lines in 6 files changed: 456 ins; 0 del; 19 mod
>>    Patch: https://git.openjdk.java.net/jdk/pull/3976.diff
>>    Fetch: git fetch https://git.openjdk.java.net/jdk
>> pull/3976/head:pull/3976
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3976
>>

Re: RFR: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available

[Michael Kroll]

Okay, that makes sense.
Thanks for clarifying.

Greetings,
Michael


Am 13. Mai 2021 00:45:13 MESZ schrieb Claes Redestad 
:
>Hi,
>
>I gave this some thought the other day when looking at one of Sergey's
>patches.
>
>I'm no expert on javac but I think translating to String::concat is
>likely to be a bit harder than it seems. It's not a static method, so
>the left hand side must be non-null. This is something I believe javac
>can't prove in the general case. Corner cases such as when the left
>hand side is a String literal, of course, but that would be of limited
>utility.
>
>There are more convoluted solutions that might work if one squints (add
>public static concat methods and translate calls into those?), but the
>question we need to be asking ourselves is really if it is worth our
>time - and the added complexity? The answer is likely no.
>
>It might help some if you're targeting java 8 or using javac as a
>frontend for non-JVM targets (lol!), but going forward I think it would
>mostly help java.base and a few other JDK modules where the
>invokedynamic translation can't be used due bootstrapping issues. And
>in
>most of those cases it won't really matter for performance anyhow.
>
>That's why I think using String::concat on a case-by-case basis in
>those
>few OpenJDK modules where indified string concat is not applicable is
>enough. When we see that it helps. And then only sparingly.
>
>Hope this clarifies how I reason about this.
>
>/Claes
>
>
>On 2021-05-12 22:49, Michael Kroll wrote:
>> Hello,
>> 
>> just being curious here:
>> Before we start to change every usage of String+String into
>String.concat, shouldn't the compiler be so smart to do that for us?
>> Currently it compiles to invokedynamic if available and to using
>StringBuilder otherwise. Now why doesn't it compile to String.concat
>instead of StringBuilder for the case when invokedynamic is not
>available as target?
>> 
>> greets,
>> Michael
>> 
>> 
>> 
>> 
>> Am 12. Mai 2021 15:04:21 MESZ schrieb "Сергей Цыпанов"
>:
>>> Hello, from discussion in https://github.com/openjdk/jdk/pull/3464
>I've
>>> found out, that in a few of JDK core classes, e.g. in `j.l.Class`
>>> expressions like `baseName.replace('.', '/') + '/' + name` are not
>>> compiled into `invokedynamic`-based code, but into one using
>>> `StringBuilder`. This happens due to some bootstraping issues.
>>>
>>> However the code like
>>>
>>> private String getSimpleName0() {
>>> if (isArray()) {
>>> return getComponentType().getSimpleName() + "[]";
>>> }
>>> //...
>>> }
>>>
>>> can be improved via replacement of `+` with `String.concat()` call.
>>>
>>> I've used this benchmark to measure impact:
>>>
>>> @State(Scope.Thread)
>>> @BenchmarkMode(Mode.AverageTime)
>>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>>> public class ClassMethodsBenchmark {
>>>   private final Class arrayClass =
>Object[].class;
>>>   private Method descriptorString;
>>>
>>>   @Setup
>>>   public void setUp() throws NoSuchMethodException {
>>> //fore some reason compiler doesn't allow me to call
>>> Class.descriptorString() directly
>>>   descriptorString =
>Class.class.getDeclaredMethod("descriptorString");
>>>   }
>>>
>>>   @Benchmark
>>>   public Object descriptorString() throws Exception {
>>> return descriptorString.invoke(arrayClass);
>>>   }
>>>
>>>   @Benchmark
>>>   public Object typeName() {
>>> return arrayClass.getTypeName();
>>>   }
>>> }
>>>
>>> and got those results
>>>
>>>   Mode  Cnt Score Error  
>Units
>>> descriptorString   avgt   60   
>91.480
>>> ±   1.839   ns/op
>>> descriptorString:·gc.alloc.rate.norm   avgt   60  
>404.008
>>> ±   4.033B/op
>>> descriptorString:·gc.churn.G1_Eden_Space   avgt   60 
>2791.866
>>> ± 181.589  MB/sec
>>> descriptorString:·gc.churn.G1_Eden_Space.norm  avgt   60  
>401.702
>>> ±  26.047B/op
>>> descriptorString:·gc.churn.G1_Survivor_Space   avgt   60
>0.003
>>> ±   0.001  MB/sec
>>> descriptorString:·gc.churn.G1_Survivor_Space.norm  avgt   60≈
>10⁻³
>>> B/op
>>> descriptorString:·gc.count avgt   60  
>205.000
>>>   counts
>>> descriptorString:·gc.time  avgt   60  
>216.000
>>>   ms
>>>
>>> patched
>>>   Mode  Cnt Score Error  
>Units
>>> descriptorString   avgt   60   
>65.016
>>> ±   3.446   ns/op
>>> descriptorString:·gc.alloc.rateavgt   60 
>2844.240
>>> ± 115.719  MB/sec
>>> descriptorString:·gc.alloc.rate.norm   avgt   60  
>288.006
>>> ±   0.001B/op
>>> descriptorString:·gc.churn.G1_Eden_Space   avgt   60 
>2832.996
>>> ± 206.939  MB/sec
>>> descriptorString:·gc.churn.G1_Eden_Space.norm  avgt   60  
>286.955
>>> ±  17.853B/op
>>> 

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Chris Hegarty]

On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen  wrote:

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

The non-static state in this test class is initialized for each of the static 
testXXX scenarios. An alternative could be to move said state (four fields) 
into a static inner class, and have each of the testXXX scenarios create an 
instance of that class with the test-specific path. That would also allow the 
addition of the no-args public constructor to HashesTest, and the testXXX 
methods to be made non-static.

-

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

Re: Fuzzing the Java core libs

[Alan Bateman]

On 13/05/2021 10:37, Fabian Meumertzheim wrote:
OSS-Fuzz files bugs in a Monorail instance (of 
https://bugs.chromium.org 
 
fame), which can be used to discuss the issues, but of course doesn't 
have to be. Authentication to that Monorail instance as well as to 
oss-fuzz.com 
, 
which provides additional information on findings and fuzzer 
performance, is tied to Google accounts.


All findings (security or not) remain confidential for 90 days (+ a 
possibility for a grace period) or until OSS-Fuzz determines that the 
finding no longer reproduces against the source repository (i.e., a 
fix has been committed).


Does the OpenJDK security workflow rely on commits with purposefully 
innocuous messages to the main branch for embargoed security issues? 
If that's the case, we should ensure that the OSS-Fuzz policies don't 
conflict with the OoenJDK security policies before performing the 
integration.


The workflow is shown on the Vulnerability Group page [1]. There isn't a 
repo that you can test commits on before the publication date.


-Alan

[1] https://openjdk.java.net/groups/vulnerability/

Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Alan Bateman]

On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen  wrote:

> HI all,
> 
> Please review the fix to HashesTest.java to support running on TestNG 7.4.  
> 
> The fix adds a no-arg constructor which TestNG requires.
> 
> The change allows the test to run with the current jtreg as well as the 
> upcoming jtreg
> 
> 
> Best
> Lance

Changes requested by alanb (Reviewer).

test/jdk/tools/jmod/hashes/HashesTest.java line 94:

> 92: lib= null;
> 93: builder=null;
> 94: }

This looks like a workaround. Can you instead see if the fields can be changed 
to non-final and place the constructor with a method that has the 
`@BeforeClass` annotation?

-

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

Re: RFR: 8266310: deadlock while loading the JNI code

[Chris Hegarty]

On Tue, 11 May 2021 13:10:30 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.

Hi Aleksei, 

As you may know, I looked into a similar issue recently and put together a 
reproducer [1] (which is probably similar to what you have). The reproducer, 
run at the time against Oracle 11u, demonstrates the issue on the mainline too, 
but the deadlock is slightly different.   The reason I mention it here is that 
the reproducer encounters the issue whether there is an attempt to load the 
same class or another class ( from the same jar file ).  In fact, the issue is 
even more general, the problem is with trying to load a class, not already 
loaded, from a jar further down on the class path ( the class may not even 
exist, just that it causes the loader to walk the class path up to the jar 
being verified ).

I filed an issue for this, which may need to be closed as a duplicate depending 
on the outcome of this PR [2].

[1] https://github.com/ChrisHegarty/deadlock
[2] https://bugs.openjdk.java.net/browse/JDK-8266350

-

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

RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

[Lance Andersen]

HI all,

Please review the fix to HashesTest.java to support running on TestNG 7.4.  

The fix adds a no-arg constructor which TestNG requires.

The change allows the test to run with the current jtreg as well as the 
upcoming jtreg


Best
Lance

-

Commit messages:
 - Remove extrawhitespace
 - Update HashesTest for TestNG 7.4

Changes: https://git.openjdk.java.net/jdk/pull/4009/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4009=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266461
  Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4009.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v19]

[Maurizio Cimadamore]

> This PR contains the API and implementation changes for JEP-412 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/412

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

  Rename is_entry_blob to is_optimized_entry_blob
  Rename as_entry_blob to as_optimized_entry_blob
  Fix misc typos and indentation issues

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3699/files
  - new: https://git.openjdk.java.net/jdk/pull/3699/files/3f99cccf..352c287f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3699=18
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3699=17-18

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

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v19]

[Vladimir Ivanov]

On Thu, 13 May 2021 10:52:35 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Rename is_entry_blob to is_optimized_entry_blob
>   Rename as_entry_blob to as_optimized_entry_blob
>   Fix misc typos and indentation issues

Marked as reviewed by vlivanov (Reviewer).

-

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v18]

[Maurizio Cimadamore]

> This PR contains the API and implementation changes for JEP-412 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/412

Maurizio Cimadamore has updated the pull request incrementally with two 
additional commits since the last revision:

 - Take care of remaining references to "Panama"
 - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
   * Replace EntryBlob with OptimizedEntryBlob

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3699/files
  - new: https://git.openjdk.java.net/jdk/pull/3699/files/03662920..3f99cccf

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3699=17
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3699=16-17

  Stats: 45 lines in 15 files changed: 1 ins; 1 del; 43 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3699.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3699/head:pull/3699

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

Re: Fuzzing the Java core libs

[Fabian Meumertzheim]

OSS-Fuzz files bugs in a Monorail instance (of https://bugs.chromium.org
fame), which can be used to discuss the issues, but of course doesn't have
to be. Authentication to that Monorail instance as well as to oss-fuzz.com,
which provides additional information on findings and fuzzer performance,
is tied to Google accounts.

All findings (security or not) remain confidential for 90 days (+ a
possibility for a grace period) or until OSS-Fuzz determines that the
finding no longer reproduces against the source repository (i.e., a fix has
been committed).

Does the OpenJDK security workflow rely on commits with purposefully
innocuous messages to the main branch for embargoed security issues? If
that's the case, we should ensure that the OSS-Fuzz policies don't conflict
with the OoenJDK security policies before performing the integration.

On Thu, May 13, 2021, 10:51 Alan Bateman  wrote:

> On 13/05/2021 09:14, Fabian Meumertzheim wrote:
> > I'm one of the maintainers of Jazzer (
> > https://github.com/CodeIntelligenceTesting/jazzer), a new open-source
> > fuzzer for the JVM platform. Jazzer has recently been integrated into
> > Google's OSS-Fuzz (https://google.github.io/oss-fuzz/) to allow for free
> > continuous fuzzing of important open-source Java projects. Jazzer has
> > already found over a hundred bugs and eight security issues in libraries
> > such as Apache Commons, PDFBox and the OWASP json-sanitizer.
> >
> > Jazzer finds unexpected exceptions and infinite loops by default, but can
> > also be used to check domain-specific properties such as
> > decrypt(encrypt(data)) == data. Since it tracks the coverage it achieves
> > using instrumentation applied by a Java agent, it can synthesize
> > interesting test data from scratch.
> >
> > If there is interest from your side, I could set up the Java core
> libraries
> > themselves for fuzzing in OSS-Fuzz. Especially the parts that are
> > frequently applied to untrusted input, such as java.security.* and
> > javax.imageio.*, would benefit from fuzz tests. I have prepared basic
> fuzz
> > tests for some of the classes in these packages at
> >
> https://github.com/CodeIntelligenceTesting/oss-fuzz/tree/openjdk/projects/openjdk
> ,
> > which has already resulted in the first bug report (JDK-8267086).
> >
> > All I would need from you is:
> >
> > * a list of email addresses to which the fuzzer findings should be sent
> > (ideally associated with Google accounts for authentication to full
> reports
> > on oss-fuzz.com),
> > * ideas for additional fuzz tests, in particular those where there are
> > interesting properties to verify.
> >
> > The technical questions about setting up the OpenJDK in OSS-Fuzz have
> > already been resolved (see also
> > https://github.com/google/oss-fuzz/issues/5757).
> >
> > If you need more information on OSS-Fuzz or fuzzing in general, I am
> happy
> > to help.
> I have one ask. As you mention, sometimes fuzzing will report issues
> issues that may be security issues. It often requires experts in
> particular areas to look at an issue and decide if it a functional or
> security issue. If there is any question mark over an issue then the
> assumption is that it is a security issue until determined otherwise. In
> that context it may not be possible to engage on the mailing lists here
> about these issues. Oracle engineers are strictly prohibited from
> engaging in any discussions on mailing lists about potential
> vulnerability issues. I suspect many other contributors are somewhat
> restricted too but I think everyone is very responsible and knows not to
> discuss sensitive issues that may need patching. So all I ask is that if
> the fuzzer finds issues that may be security issues that they should be
> reported to the vunl-report list [1] and not discussed on the mailing
> list. I'm not suggesting to sign up the vunl-report list for
> notifications of course, but whoever is triaging these issues should
> know how to report issues.
>
> -Alan
>
> [1] https://openjdk.java.net/groups/vulnerability/report
>

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v17]

[Maurizio Cimadamore]

> This PR contains the API and implementation changes for JEP-412 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/412

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

  Address CSR comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3699/files
  - new: https://git.openjdk.java.net/jdk/pull/3699/files/6701654c..03662920

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3699=16
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3699=15-16

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

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

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/12/21 8:12 PM, Alan Snyder wrote:
> From the motivation section:
> 
>> So you want to invoke a method |X| in a library that later calls back into 
>> your code. In your callback you need some context, perhaps a transaction ID 
>> or some |File| instances. However, |X| provides no way to pass a reference 
>> through their code into your callback. What are you to do?
> 
> When I read this, my first thought was… pass a callback that is bound to the 
> context.
> 
> I suppose the example you have in mind has a fixed callback.

Fixed? It's a callback, of some form.

> Is this a common practice? Doesn’t it point to a deficiency in the library 
> API?

Not necessarily. For example, say you want to give access to a particular
resource (a log stream, say) to trusted callees. Nobody AFAIK passes a log
stream as an explicit argument through business logic because that's just
too much clutter.

> Is this feature just a workaround for poorly designed libraries, or are there 
> other examples?

It's not really about poor design as much as separation of concerns.
Intermediate libraries have no way to know what might need to be passed
to callees, and in many cases it's better isolation if they don't get to
find out. The latter is especially true of passing security permissions.

Also, there is evolution of libraries: with scope locals you don't need
to change library interfaces to add useful capabilities like logging.

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

Integrated: 8263382: java/util/logging/ParentLoggersTest.java failed with "checkLoggers: getLoggerNames() returned unexpected loggers"

[Daniel Fuchs]

On Wed, 12 May 2021 15:48:16 GMT, Daniel Fuchs  wrote:

> ParentLoggersTest.java has been (rarely) seen failing with "checkLoggers: 
> getLoggerNames() returned unexpected loggers"
> The suspicion is that there might be some possible interaction with other 
> tests running in the same VM. This test causes the LogManager to reparse its 
> configuration and should therefore run in its own VM.

This pull request has now been integrated.

Changeset: 08a5a5c6
Author:Daniel Fuchs 
URL:   
https://git.openjdk.java.net/jdk/commit/08a5a5c6d64db51700d058954d115aa89dbe73be
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8263382: java/util/logging/ParentLoggersTest.java failed with "checkLoggers: 
getLoggerNames() returned unexpected loggers"

Reviewed-by: bpb, mchung

-

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

Re: RFR: 8266310: deadlock while loading the JNI code

[Aleksei Voitylov]

On Tue, 11 May 2021 13:10:30 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.

Peter, that's right. It is still possible to have the same pair of objects 
being locked with per-library lock, namely when a library being loaded 
explicitly loads a class that requires the same library to be loaded. I think 
this type of design flaw cannot be solved by changing locking algorithm. It's 
clearly not the goal of this patch.

-

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

Re: Fuzzing the Java core libs

[Alan Bateman]

On 13/05/2021 09:14, Fabian Meumertzheim wrote:

I'm one of the maintainers of Jazzer (
https://github.com/CodeIntelligenceTesting/jazzer), a new open-source
fuzzer for the JVM platform. Jazzer has recently been integrated into
Google's OSS-Fuzz (https://google.github.io/oss-fuzz/) to allow for free
continuous fuzzing of important open-source Java projects. Jazzer has
already found over a hundred bugs and eight security issues in libraries
such as Apache Commons, PDFBox and the OWASP json-sanitizer.

Jazzer finds unexpected exceptions and infinite loops by default, but can
also be used to check domain-specific properties such as
decrypt(encrypt(data)) == data. Since it tracks the coverage it achieves
using instrumentation applied by a Java agent, it can synthesize
interesting test data from scratch.

If there is interest from your side, I could set up the Java core libraries
themselves for fuzzing in OSS-Fuzz. Especially the parts that are
frequently applied to untrusted input, such as java.security.* and
javax.imageio.*, would benefit from fuzz tests. I have prepared basic fuzz
tests for some of the classes in these packages at
https://github.com/CodeIntelligenceTesting/oss-fuzz/tree/openjdk/projects/openjdk,
which has already resulted in the first bug report (JDK-8267086).

All I would need from you is:

* a list of email addresses to which the fuzzer findings should be sent
(ideally associated with Google accounts for authentication to full reports
on oss-fuzz.com),
* ideas for additional fuzz tests, in particular those where there are
interesting properties to verify.

The technical questions about setting up the OpenJDK in OSS-Fuzz have
already been resolved (see also
https://github.com/google/oss-fuzz/issues/5757).

If you need more information on OSS-Fuzz or fuzzing in general, I am happy
to help.
I have one ask. As you mention, sometimes fuzzing will report issues 
issues that may be security issues. It often requires experts in 
particular areas to look at an issue and decide if it a functional or 
security issue. If there is any question mark over an issue then the 
assumption is that it is a security issue until determined otherwise. In 
that context it may not be possible to engage on the mailing lists here 
about these issues. Oracle engineers are strictly prohibited from 
engaging in any discussions on mailing lists about potential 
vulnerability issues. I suspect many other contributors are somewhat 
restricted too but I think everyone is very responsible and knows not to 
discuss sensitive issues that may need patching. So all I ask is that if 
the fuzzer finds issues that may be security issues that they should be 
reported to the vunl-report list [1] and not discussed on the mailing 
list. I'm not suggesting to sign up the vunl-report list for 
notifications of course, but whoever is triaging these issues should 
know how to report issues.


-Alan

[1] https://openjdk.java.net/groups/vulnerability/report

Fuzzing the Java core libs

[Fabian Meumertzheim]

I'm one of the maintainers of Jazzer (
https://github.com/CodeIntelligenceTesting/jazzer), a new open-source
fuzzer for the JVM platform. Jazzer has recently been integrated into
Google's OSS-Fuzz (https://google.github.io/oss-fuzz/) to allow for free
continuous fuzzing of important open-source Java projects. Jazzer has
already found over a hundred bugs and eight security issues in libraries
such as Apache Commons, PDFBox and the OWASP json-sanitizer.

Jazzer finds unexpected exceptions and infinite loops by default, but can
also be used to check domain-specific properties such as
decrypt(encrypt(data)) == data. Since it tracks the coverage it achieves
using instrumentation applied by a Java agent, it can synthesize
interesting test data from scratch.

If there is interest from your side, I could set up the Java core libraries
themselves for fuzzing in OSS-Fuzz. Especially the parts that are
frequently applied to untrusted input, such as java.security.* and
javax.imageio.*, would benefit from fuzz tests. I have prepared basic fuzz
tests for some of the classes in these packages at
https://github.com/CodeIntelligenceTesting/oss-fuzz/tree/openjdk/projects/openjdk,
which has already resulted in the first bug report (JDK-8267086).

All I would need from you is:

* a list of email addresses to which the fuzzer findings should be sent
(ideally associated with Google accounts for authentication to full reports
on oss-fuzz.com),
* ideas for additional fuzz tests, in particular those where there are
interesting properties to verify.

The technical questions about setting up the OpenJDK in OSS-Fuzz have
already been resolved (see also
https://github.com/google/oss-fuzz/issues/5757).

If you need more information on OSS-Fuzz or fuzzing in general, I am happy
to help.

Fabian (@fmeum on GitHub)

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]

[David Holmes]

On Wed, 12 May 2021 16:10:24 GMT, Harold Seigel  wrote:

>> Please review this large change to remove Unsafe::defineAnonymousClass().  
>> The change removes dAC relevant code and changes a lot of tests.  Many of 
>> the changed tests need renaming.  I hope to do this in a follow up RFE.  
>> Some of the tests were modified to use hidden classes, others were deleted 
>> because either similar hidden classes tests already exist or they tested dAC 
>> specific functionality, such as host classes.
>> 
>> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
>> and Mach5 tiers 3-7 on Linux x64.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   test changes and small fixes

Hi Harold,

Big change! :) This looks good. All the removals of references to 
unsafe_anonymous were easy enough  to follow.

I didn't realize that cp patching and psuedo-strings were only related to VM 
anonymous. It is good to see all that code go as well (but hard to see if you 
got it all :) ). But as per comment below are we sure psuedo-strings can't be 
used elsewhere?

Thanks,
David

src/hotspot/share/oops/constantPool.hpp line 493:

> 491:   // object into a CONSTANT_String entry of an unsafe anonymous class.
> 492:   // Methods internally created for method handles may also
> 493:   // use pseudo-strings to link themselves to related metaobjects.

Is this comment wrong? Are psuedo-strings not used by anything now?

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]

[Alan Bateman]

On Tue, 11 May 2021 14:11:22 GMT, Harold Seigel  wrote:

>> Can you check test/jdkjava/lang/Class/attributes/ClassAttributesTest.java? 
>> It may minimally need a comment to be updated. That's the only additional 
>> test that I could find in test/jdk.
>
> Hi Alan,
> Thanks for find this.  I removed the unneeded imports and @modules from 
> GetModulesTest.java and updated the comment in ClassAttributesTest.java.
> Thanks, Harold

Thanks for fixing up these tests, I didn't spot any more in the test/jdk tree.

-

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

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]

[Alan Bateman]

On Wed, 12 May 2021 16:10:24 GMT, Harold Seigel  wrote:

>> Please review this large change to remove Unsafe::defineAnonymousClass().  
>> The change removes dAC relevant code and changes a lot of tests.  Many of 
>> the changed tests need renaming.  I hope to do this in a follow up RFE.  
>> Some of the tests were modified to use hidden classes, others were deleted 
>> because either similar hidden classes tests already exist or they tested dAC 
>> specific functionality, such as host classes.
>> 
>> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
>> and Mach5 tiers 3-7 on Linux x64.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   test changes and small fixes

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2 [v3]

[Alan Bateman]

On Wed, 12 May 2021 21:03:27 GMT, Ioi Lam  wrote:

>> This bug was discovered during the development of 
>> [JDK-6824466](https://bugs.openjdk.java.net/browse/JDK-6824466): when CDS is 
>> enabled, if `BootLoader::loadClassOrNull` is called before `initPhase2`, it 
>> would trigger the initialization of the archived module graph in the wrong 
>> order. Because of unanticipated nesting of `` methods, 
>> `BootLoader::SERVICES_CATALOG` becomes empty, causing future `ServiceLoader` 
>> operations to fail.
>> 
>> The fix has 2 parts:
>> 
>> - `BootLoader::loadClassOrNull` no longer calls 
>> `ClassLoaders::bootLoader()`. This avoids triggering the archived module 
>> graph initialization. Instead, it makes a direct call to 
>> `Classloader::findBootstrapClassOrNull()`. We don't actually need a 
>> `ClassLoader` instance for this call, so I changed 
>> `Classloader::findBootstrapClassOrNull()` to be a static method.
>> - The object returned by `BootLoader::getServicesCatalog()` is now 
>> maintained inside `jdk.internal.loader.ClassLoaders`.  Although not strictly 
>> required for the fix, this simplifies the initialization of the archived 
>> module graph. It also makes the logic consistent for the 3 built-in loaders 
>> (boot/platform/app).
>> 
>> Testing: tiers1-4 in progress. I also verified that the bug reported by 
>> Mandy is no longer reproducible after I applied this patch on her branch.
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   cleaned up ClassLoaders.setArchivedServicesCatalog

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2 [v2]

[Alan Bateman]

On Wed, 12 May 2021 22:20:53 GMT, Mandy Chung  wrote:

>> How about the latest version (4cd981c)? I removed the `archivedClassLoaders` 
>> argument; this also makes the callers simpler.
>
> Looks good to me.

This makes it simpler and much nicer - thank you!

-

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

Re: RFR: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available

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

> Before we start to change every usage of String+String into String.concat, 
> shouldn't the compiler be so smart to do that for us?
> Currently it compiles to invokedynamic if available and to using 
> StringBuilder otherwise. Now why doesn't it compile to String.concat instead 
> of StringBuilder for the case when invokedynamic is not available as target?

I think the main reason is that

String str = "smth is " + null;

returns "smth is null" and with current implementation of String.concat()
the same expression throws NPE. See the code of String.concat():

public String concat(String str) {
if (str.isEmpty()) {
return this;
}
return StringConcatHelper.simpleConcat(this, str);
}

Regards,
Sergey

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v16]

[Vladimir Ivanov]

On Wed, 12 May 2021 15:07:37 GMT, Maurizio Cimadamore  
wrote:

>> src/hotspot/share/runtime/sharedRuntime.hpp line 465:
>> 
>>> 463:   static void restore_native_result(MacroAssembler *_masm, BasicType 
>>> ret_type, int frame_slots);
>>> 464: 
>>> 465:   static void   move32_64(MacroAssembler* masm, VMRegPair src, 
>>> VMRegPair dst);
>> 
>> Please, file an RFE to move these declarations to `MacroAssembler`.
>
> There's already an issue for that:
> https://bugs.openjdk.java.net/browse/JDK-8266257
> 
> I've upgraded the description to make it reflect the proposed move a bit more 
> precisely.

Got it. I was confused by the bug synopsis.

-

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