Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v6]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: restoring jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES, it is needed by the build - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/2744f615..304fd76a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3526&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3526&range=04-05 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: [External] : Re: ReversibleCollection proposal
On 4/30/21 9:25 PM, Stuart Marks wrote:
So I think we're stuck with void-returning add{First,Last} methods.
Have you thought of perhaps not adding them at all?
Collection.add() for:
List(s) - behaves the same as addLast()
LinkedHashSet - behaves the same as addLast()
Deque - behaves the same as addLast()
So for all ReversibleCollection(s) where it works, addLast() would be
equivalent to add()
addFirst(x) can be written as: reversed().add(x) if reversed() is
specified to return a reversed view over the underlying
ReversibleCollection.
Peter
Re: ReversibleCollection proposal
- Mail original - > De: "Stuart Marks" > À: "Remi Forax" > Cc: "core-libs-dev" > Envoyé: Samedi 1 Mai 2021 00:42:10 > Objet: Re: ReversibleCollection proposal >> You did not really answer to the real question, why should i use >> ReversibleCollection instead of a Collection as parameter. >> You said that it's a better type because you can not send a HashSet, but as i >> said, sending a HashSet of 0 or 1 element is perfectly valid. >> For me, we are not far from, it's typechecking for the sake of typechecking. > > I thought I did answer it, but it might have been in response to a different > message > on a different part of the thread. But I'll make the case in a more explicit > fashion > here. > > First, a couple background points related to this: > > - ReversibleCollection is not intended to, and indeed cannot represent all > ordered > collections. Queue is ordered and is not a ReversibleCollection, and there are > likely other collections out there that are ordered but that implement > Collection > directly. Also, they might or might not be reversible. > > - I expect that few, if any Java SE APIs will be adjusted to use > ReversibleCollection as a parameter type. Any APIs that use Collection but > require > an ordered type cannot be changed because of compatibility reasons. > > Despite both of these points, I believe ReversibleCollection can be > successful, > and > it can be useful in APIs as a parameter type. (The proposal itself uses > ReversibleCollection and ReversibleSet as method return types.) > > Consider the case of a large application or other system, one that's large > enough to > have lots of internal APIs, but that is built as a single unit, so > release-to-release compatibility isn't an issue. Suppose there is some method > > processItemsInOrder(List items) > > that has to process items in the order in which they occur, because processing > of > later items might depend the processing of earlier ones. The maintainer of > this > API > chose to accept a List as a parameter, because it's a common interface and > it's > clearly an ordered collection. > > Now consider a client that gets items from different places, keeping them in > order, > but removing duplicates. It might do something like this: > > var items = new LinkedHashSet(); > items.addAll(getItemsFromSomeplace()); > items.addAll(getItemsFromAnotherPlace()); > items.addAll(getItemsFromSomeplaceElse()); > processItemsInOrder(new ArrayList<>(items)); > > It turns out the copying of the items into an ArrayList is a performance > bottleneck, > so the maintainer of the client code asks the maintainer of the items > processing > code to change the API to accept Collection instead. > > The items processing maintainer demurs, recalling that the API *did* accept > Collection in the past, and a bug where somebody accidentally passed a HashSet > resulted in a customer escalation because of item processing irregularities. > In > the > aftermath of that escalation, the API was changed to List. The client > maintainer > reluctantly pursues alternatives for generating a deduplicated List. > > But wait! Those Java guys added a ReversibleCollection interface in Java N. It > has > the desired property of being ordered, and conveniently it's a supertype of > both > List and LinkedHashSet. The items processing maintainer adjusts the API to > consume > ReversibleCollection, and the client maintainer removes the temporary > ArrayList, > and > everybody is happy. I suppose the performance issue comes from the fact that traversing a LinkedHahSet is slow because it's a linked list ? You can replace a LinkedHashSet by a List + Set, the List keeps the values in order, the Set avoids duplicates. Using a Stream, it becomes Stream.of(getItemsFromSomeplace(), getItemsFromAnotherPlace(), getItemsFromSomeplaceElse()) .flatMap(List::stream) .distinct() // use a Set internally .collect(toList()); I think there are maybe some scenarios where ReversibleCollection can be useful, but they are rare, to the point where when there is a good scenario for it people will not recognize it because ReversibleCollection will not be part of their muscle memory. There is a real value to add methods like descendingSet/descendingList()/getFirst/getLast on existing collections, but we don't need a new interface (or two) for that. > > s'marks Rémi
Integrated: JDK-8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
On Fri, 30 Apr 2021 20:19:25 GMT, Andy Herrick wrote: > JDK-8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with > fastdebug > > just disabling this test when vm.debug This pull request has now been integrated. Changeset: 5c083e85 Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/5c083e8560ce9cc78615e3149a558206724cff53 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug Reviewed-by: asemenyuk, almatvee - PR: https://git.openjdk.java.net/jdk/pull/3827
RFR: 8266193: BasicJMapTest should include testHistoParallel methods
The testHistoParallel* method are not included in the BasicJMapTest's main() method. They should be included. - Commit messages: - 8266193: BasicJMapTest should include testHistoParallel methods Changes: https://git.openjdk.java.net/jdk/pull/3810/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3810&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266193 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3810.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3810/head:pull/3810 PR: https://git.openjdk.java.net/jdk/pull/3810
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Wed, 28 Apr 2021 06:44:58 GMT, Ioi Lam wrote: >> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 557: >> >>> 555: // non-latin1, and deduplicating if we find a match. For >>> deduplication we >>> 556: // only care if the arrays consist of the same sequence of bytes. >>> 557: const jchar* chars = static_cast(value->base(T_CHAR)); >> >> The encoding of the shared strings always match CompactStrings. Otherwise >> the CDS archive will fail to map. E.g., >> >> >> $ java -XX:-CompactStrings -Xshare:dump >> $ java -XX:+CompactStrings -Xlog:cds -version >> ... >> [0.032s][info][cds] UseSharedSpaces: The shared archive file's >> CompactStrings >> setting (disabled) does not equal the current >> CompactStrings setting (enabled). >> [0.032s][info][cds] UseSharedSpaces: Unable to map shared spaces >> ... >> >> >> So you can avoid this `if` block when `CompactStrings == true`. The >> `!java_lang_String::is_latin1(found)` check below can be changed to an >> assert. >> >> Also, I think we need an explicit test case where you'd return `true` at >> line 564. I can write a new test case and email to you. I think it will >> involve dumping an archive with `-XX:-CompactStrings`. > > Oh, I realized that my suggestion above is wrong. When > `CompactStrings==true`, you can still have a string whose coder is UTF16: > > https://github.com/openjdk/jdk/blob/75a2354dc276e107d64516d20fc72bc7ef3d5f86/src/java.base/share/classes/java/lang/String.java#L343-L351 Correct. - PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Sun, 25 Apr 2021 17:32:51 GMT, Zhengyu Gu 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.
>
> src/hotspot/share/classfile/stringTable.cpp line 355:
>
>> 353: // Notify deduplication support that the string is being interned. A
>> string
>> 354: // must never be deduplicated after it has been interned. Doing so
>> interferes
>> 355: // with compiler optimizations don on e.g. interned string literals.
>
> typo: don -> done
Fixed locally.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Wed, 28 Apr 2021 00:29:23 GMT, Ioi Lam wrote:
>> src/hotspot/share/classfile/stringTable.cpp line 784:
>>
>>> 782: SharedStringIterator iter(f);
>>> 783: _shared_table.iterate(&iter);
>>> 784: }
>>
>> So with this change (somewhere below) do you no longer deduplicate strings
>> from the shared string table? ie
>>
>> // The CDS archive does not include the string deduplication table. Only the
>> string
>> // table is saved in the archive. The shared strings from CDS archive need
>> to be
>> // added to the string deduplication table before deduplication occurs. That
>> is
>> // done in the beginning of the StringDedupThread (see
>> StringDedupThread::do_deduplication()).
>> void StringDedupThread::deduplicate_shared_strings(StringDedupStat* stat) {
>> StringDedupSharedClosure sharedStringDedup(stat);
>> StringTable::shared_oops_do(&sharedStringDedup);
>> }
>> Asking @iklam to have a look then.
>
> If I understand correctly, we no longer need to add the entire set of shared
> strings into the dedup table.
>
>
> +// As a space optimization, when shared StringTable entries exist the shared
> +// part of the StringTable is also used as a source for byte arrays. This
> +// permits deduplication of strings against those shared entries without
> +// recording them in this table too.
> +class StringDedup::Table : AllStatic {
>
> ...
>
> +void StringDedup::Table::deduplicate(oop java_string) {
> + assert(java_lang_String::is_instance(java_string), "precondition");
> + _cur_stat.inc_inspected();
> + if ((StringTable::shared_entry_count() > 0) &&
> + try_deduplicate_shared(java_string)) {
> +return; // Done if deduplicated against shared
> StringTable.
Ioi is correct; we deduplicate "directly" against the shared string table
rather than adding the shared strings to the deduplication table.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Tue, 27 Apr 2021 22:30:04 GMT, Coleen Phillimore 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.
>
> src/hotspot/share/classfile/javaClasses.inline.hpp line 77:
>
>> 75:
>> 76: uint8_t* java_lang_String::flags_addr(oop java_string) {
>> 77: assert(_initialized, "Mut be initialized");
>
> typo: must
Fixed locally.
> src/hotspot/share/runtime/globals.hpp line 1994:
>
>> 1992:
>> \
>> 1993: product(uint64_t, StringDeduplicationHashSeed, 0, DIAGNOSTIC,
>> \
>> 1994: "Seed for the table hashing function; 0 requests computed
>> seed") \
>
> Should these two really be develop() options?
StringDeduplicationResizeALot is used by a test that we want to run against
release builds too. If StringDeduplicationHashSeed isn't a diagnostic option
(so available in release builds), I'd be more inclined to just remove it than
to make it a develop option.
> src/hotspot/share/runtime/mutexLocker.cpp line 239:
>
>> 237: def(StringDedupQueue_lock , PaddedMonitor, leaf,true,
>> _safepoint_check_never);
>> 238: def(StringDedupTable_lock , PaddedMutex , leaf + 1,true,
>> _safepoint_check_never);
>> 239: }
>
> Why weren't these duplicate definitions? This is disturbing.
These are assignments, not definitions. Only one of the assignments was being
performed because only one of G1 or Shenandoah will be selected. (Not that it
matters anymore after this change.)
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Integrated: 8265356: need code example for getting canonical constructor of a Record
On Sat, 17 Apr 2021 08:55:59 GMT, Tagir F. Valeev wrote:
> I decided to show a complete static method in the example, so it could be
> copied to user utility class as is. Not sure if it's reasonable to add
> `assert cls.isRecord();` there. Also I don't know whether there's a
> limitation on max characters in the sample code. Probable a line break in
> `static \nConstructor getCanonicalConstructor(Class
> cls)` is unnecessary.
>
> ---
> Aside from this PR, I've found a couple of things to clean up in
> `java.lang.Class`:
> 1. There's erroneous JavaDoc link in `getSimpleName()` JavaDoc (introduced by
> @jddarcy in #3038). It should be `#isArray()` instead of `isArray()`.
> 2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have
> unused type parameters ``.
> 3. Probably too much but AnnotationData can be nicely converted to a record!
> Not sure, probably nobody wants to have `java.lang.Record` initialized too
> early or increasing the footprint of such a basic class in the metaspace, so
> I don't insist on this.
>
>
> private record AnnotationData(
> Map, Annotation> annotations,
> Map, Annotation> declaredAnnotations,
> // Value of classRedefinedCount when we created this AnnotationData
> instance
> int redefinedCount) {
> }
>
>
> Please tell me if it's ok to fix 1 and 2 along with this PR.
This pull request has now been integrated.
Changeset: 3e667cc4
Author:Tagir F. Valeev
URL:
https://git.openjdk.java.net/jdk/commit/3e667cc40521dfb6d07dda07c2f33e37086ee64b
Stats: 24 lines in 2 files changed: 17 ins; 0 del; 7 mod
8265356: need code example for getting canonical constructor of a Record
Reviewed-by: smarks
-
PR: https://git.openjdk.java.net/jdk/pull/3556
Re: RFR: 8265356: need code example for getting canonical constructor of a Record [v2]
On Sat, 24 Apr 2021 01:32:45 GMT, Tagir F. Valeev wrote:
>> I decided to show a complete static method in the example, so it could be
>> copied to user utility class as is. Not sure if it's reasonable to add
>> `assert cls.isRecord();` there. Also I don't know whether there's a
>> limitation on max characters in the sample code. Probable a line break in
>> `static \nConstructor getCanonicalConstructor(Class
>> cls)` is unnecessary.
>>
>> ---
>> Aside from this PR, I've found a couple of things to clean up in
>> `java.lang.Class`:
>> 1. There's erroneous JavaDoc link in `getSimpleName()` JavaDoc (introduced
>> by @jddarcy in #3038). It should be `#isArray()` instead of `isArray()`.
>> 2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have
>> unused type parameters ``.
>> 3. Probably too much but AnnotationData can be nicely converted to a record!
>> Not sure, probably nobody wants to have `java.lang.Record` initialized too
>> early or increasing the footprint of such a basic class in the metaspace, so
>> I don't insist on this.
>>
>>
>> private record AnnotationData(
>> Map, Annotation> annotations,
>> Map, Annotation> declaredAnnotations,
>> // Value of classRedefinedCount when we created this AnnotationData
>> instance
>> int redefinedCount) {
>> }
>>
>>
>> Please tell me if it's ok to fix 1 and 2 along with this PR.
>
> Tagir F. Valeev has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Trailing space removed
> - Add a reference from java.lang.Record to related Class methods
> - Fix cosmetic issues
Thank you! I also changed 'This method' to 'The following method', as it looks
like this wording is more commonly used in OpenJDK specs.
-
PR: https://git.openjdk.java.net/jdk/pull/3556
Re: RFR: 8265356: need code example for getting canonical constructor of a Record [v3]
> I decided to show a complete static method in the example, so it could be
> copied to user utility class as is. Not sure if it's reasonable to add
> `assert cls.isRecord();` there. Also I don't know whether there's a
> limitation on max characters in the sample code. Probable a line break in
> `static \nConstructor getCanonicalConstructor(Class
> cls)` is unnecessary.
>
> ---
> Aside from this PR, I've found a couple of things to clean up in
> `java.lang.Class`:
> 1. There's erroneous JavaDoc link in `getSimpleName()` JavaDoc (introduced by
> @jddarcy in #3038). It should be `#isArray()` instead of `isArray()`.
> 2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have
> unused type parameters ``.
> 3. Probably too much but AnnotationData can be nicely converted to a record!
> Not sure, probably nobody wants to have `java.lang.Record` initialized too
> early or increasing the footprint of such a basic class in the metaspace, so
> I don't insist on this.
>
>
> private record AnnotationData(
> Map, Annotation> annotations,
> Map, Annotation> declaredAnnotations,
> // Value of classRedefinedCount when we created this AnnotationData
> instance
> int redefinedCount) {
> }
>
>
> Please tell me if it's ok to fix 1 and 2 along with this PR.
Tagir F. Valeev has updated the pull request incrementally with one additional
commit since the last revision:
Use @apiNote; this method -> the following method
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3556/files
- new: https://git.openjdk.java.net/jdk/pull/3556/files/1133ab7d..86cac681
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3556&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3556&range=01-02
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/3556.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3556/head:pull/3556
PR: https://git.openjdk.java.net/jdk/pull/3556
