Re: [External] : Sequenced Collections
Wow, I missed that the Sequenced Collections JEP draft was posted! Of course, I strongly support this initiative and am happy that my proposal got some love and is moving forward. In general, I like the JEP in the way it is. I have only two slight concerns: 1. I'm not sure that having addition methods (addFirst, addLast, putFirst, putLast) is a good idea, as not every mutable implementation may support them. While this adds some API unification, it's quite a rare case when this could be necessary. I think most real world applications of Sequenced* types would be around querying, or maybe draining (so removal operations are ok). Probably it would be enough to add addFirst, addLast, putFirst, putLast directly to the compatible implementations/subinterfaces like List, LinkedHashSet, and LinkedHashMap removing them from the Sequenced* interfaces. In this case, SortedSet interface will not be polluted with operations that can never be implemented. Well my opinion is not very strong here. 2. SequencedCollection name is a little bit too long. I think every extra letter adds a hesitation for users to use the type, especially in APIs where it could be the most useful. I see the Naming section and must admit that I don't have better ideas. Well, maybe just Sequenced would work? Or too vague? Speaking of Remi's suggestion, I don't think it's a good idea. Maybe it could be if we designed the Collection API from scratch. But given the current state of Java collections, it's better to add new interfaces than to put the new semantics to the java.util.List and greatly increase the amount of non-random-accessed lists in the wild. There are tons of code that implicitly assume fast random access of every incoming list (using indexed iteration inside). The suggested approach could become a performance disaster. With best regards, Tagir Valeev. On Sat, Feb 12, 2022 at 2:26 AM Stuart Marks wrote: > Hi Rémi, > > I see that you're trying to reduce the number of interfaces introduced by > unifying > things around an existing interface, List. Yes, it's true that List is an > ordered > collection. However, your analysis conveniently omits other facts about > List that > make it unsuitable as a general "ordered collection" interface. > Specifically: > > 1) List supports element access by int index; and > > 2) List is externally ordered. That is, its ordering is determined by a > succession > of API calls, irrespective of element values. This is in contrast to > SortedSet et al > which are internally ordered, in that the ordering is determined by the > element values. > > The problem with indexed element access is that it creates a bunch of > hidden > performance pitfalls for any data structure where element access is other > than O(1). > So get(i) degrades to O(n), binarySearch degrades from O(log n) to O(n). > (This is in > the sequential implementation; the random access implementation degrades > to O(n log > n)). Apparently innocuous indexed for-loops degrade to quadratic. This is > one of the > reasons why LinkedList is a bad List implementation. > > If we refactor LinkedHashSet to implement List, we basically have created > another > situation just like LinkedList. That's a step in the wrong direction. > > Turning to internal ordering (SortedSet): it's fundamentally incompatible > with > List's external ordering. List has a lot of positional mutation operations > such as > add(i, obj); after this call, you expect obj to appear at position i. That > can't > work with a SortedSet. > > There is implicit positioning semantics in other methods that don't have > index > arguments. For example, replaceAll replaces each element of a List with > the result > of calling a function on that element. Crucially, the function result goes > into the > same location as the original element. That to cannot work with SortedSet. > > Well, we can try to deal with these issues somehow, like making certain > methods > throw UnsupportedOperationException, or by relaxing the semantics of the > methods so > that they no longer have the same element positioning semantics. Either of > these > approaches contorts the List interface to such an extent that it's no > longer a List. > > So, no, it's not useful or effective to try to make List be the common > "ordered > collection" interface. > > s'marks > > > > On 2/10/22 3:14 AM, Remi Forax wrote: > > I've read the draft of the JEP on sequenced collection, and i think the > proposed design can be improved. > >https://bugs.openjdk.java.net/browse/JDK-8280836 > > > > I agree with the motivation, there is a need for an API to consider the > element of a list, a sorted set and a linked hash set as an ordered > sequence of elements with a simple way to access/add/remove the first/last > element and also reverse the elements as view. > > > > I disagree about the conclusion that we need to introduce 4 new > interfaces for that matter. > > > > Here are the reasons > > 1/ Usually an ordered collection is
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Yes, I like the IllegalCallerException. - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: 8176706: Additional Date-Time Formats [v4]
On Fri, 11 Feb 2022 22:26:03 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressing review comments
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 5103:
>
>> 5101: * @param timeStyle the time style to use, may be null
>> 5102: */
>> 5103: LocalizedPrinterParser(FormatStyle dateStyle, FormatStyle
>> timeStyle) {
>
> Can the constructors be `private`.
> The combination of package protected and the style of caller doing the
> validation makes me a bit nervous.
> There should not be any callers outside of DateTimeFormatterBuilder.
Changed to `private` so that it can only be instantiated within
`DateTimeFormatterBuilder`. Same modifications are applied to other
`DateTimePrinterParser` implementations.
> src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java
> line 79:
>
>> 77: public String getJavaTimeDateTimePattern(String requestedTemplate,
>> String calType, Locale locale) {
>> 78: // default implementation throws exception
>> 79: throw new DateTimeException("Formatter is not available for the
>> requested template: " + requestedTemplate);
>
> "Formatting **pattern** is not available"...
Modified, as well as other minor corrections in the javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v5]
> Following the prior discussion [1], here is the PR for the subject > enhancement. CSR has also been updated according to the suggestion. > > [1] > https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-January/085175.html Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7340/files - new: https://git.openjdk.java.net/jdk/pull/7340/files/af3c0d62..399257d4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7340=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7340=03-04 Stats: 31 lines in 2 files changed: 3 ins; 0 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/7340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7340/head:pull/7340 PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 22:37:57 GMT, Mandy Chung wrote: > Thanks for taking on these null caller issue. > > To give more context to this issue, the spec of > `ClassLoader::isRegisteredAsParallelCapable` returns true if this class > loader is registered as parallel capable, otherwise false. The current spec > does not specify what if the caller class is not a class loader. The current > implementation throws NPE if caller is null. I initially proposed to return > false for simplicity. However, if the caller is not a subclass of > `ClassLoader`, the current implementation throws `ClassCastException`. Both > cases are invalid caller and they should be handled in the same way, either > return false or throw an exception. > > Having a second thought, since this API expects to be called by a class > loader, I think throwing `IllegalCallerException` to indicate this method is > called by an illegal caller. This will need a CSR due to the spec change. > > What do you think? Throwing IllegalCallerException sounds good to me. As a static method, from a language standpoint, the call could appear almost anywhere. But its intended usage is pretty narrow. I think it's worth notifying the developer of a pretty obvious error. Is this method mainly meant to be called from a classloader's static initializer? That might be worth mentioning in the JavaDoc (if we're doing a CSR anyway...). - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Build change looks good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7448
Integrated: JDK-8281462: Annotation toString output for enum not reusable for source input
On Thu, 10 Feb 2022 05:49:47 GMT, Joe Darcy wrote: > Two changes to the toString output for annotations to give better source > fidelity: > > 1) For enum constants, call their name method rather than their toString > method. An enum class can override the toString method to print something > other than the name. > > 2) Switch from using binary names (names with "$" for nested types) to > canonical names (names with "." with nested types) > > Various existing regression tests are updated to accommodate the changes. > > Please also review the CSR: > https://bugs.openjdk.java.net/browse/JDK-8281568 This pull request has now been integrated. Changeset: c3179a87 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/c3179a8760019b5954e344bf0d2775e1e1968f32 Stats: 80 lines in 8 files changed: 25 ins; 6 del; 49 mod 8281462: Annotation toString output for enum not reusable for source input Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v3]
On Fri, 11 Feb 2022 20:34:43 GMT, Joe Darcy wrote: >> Two changes to the toString output for annotations to give better source >> fidelity: >> >> 1) For enum constants, call their name method rather than their toString >> method. An enum class can override the toString method to print something >> other than the name. >> >> 2) Switch from using binary names (names with "$" for nested types) to >> canonical names (names with "." with nested types) >> >> Various existing regression tests are updated to accommodate the changes. >> >> Please also review the CSR: >> https://bugs.openjdk.java.net/browse/JDK-8281568 > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Respond to more review feedback. > - Merge branch 'master' into JDK-8281462 > - Respond to review feedback. > - JDK-8281462: Annotation toString output for enum not reusable for source > input Looks good to me. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: 8176706: Additional Date-Time Formats [v4]
On Fri, 11 Feb 2022 00:03:50 GMT, Naoto Sato wrote:
>> Following the prior discussion [1], here is the PR for the subject
>> enhancement. CSR has also been updated according to the suggestion.
>>
>> [1]
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-January/085175.html
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressing review comments
Looks good.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5103:
> 5101: * @param timeStyle the time style to use, may be null
> 5102: */
> 5103: LocalizedPrinterParser(FormatStyle dateStyle, FormatStyle
> timeStyle) {
Can the constructors be `private`.
The combination of package protected and the style of caller doing the
validation makes me a bit nervous.
There should not be any callers outside of DateTimeFormatterBuilder.
src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java
line 66:
> 64: * Returns the formatting pattern for the requested template,
> calendarType, and locale.
> 65: * Concrete implementation of this method will retrieve
> 66: * a java.time specific pattern from selected Locale Provider.
editorial: "from **the** selected Locale"...
src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java
line 79:
> 77: public String getJavaTimeDateTimePattern(String requestedTemplate,
> String calType, Locale locale) {
> 78: // default implementation throws exception
> 79: throw new DateTimeException("Formatter is not available for the
> requested template: " + requestedTemplate);
"Formatting **pattern** is not available"...
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Thanks for taking on these null caller issue. To give more context to this issue, the spec of `ClassLoader::isRegisteredAsParallelCapable` returns true if this class loader is registered as parallel capable, otherwise false. The current spec does not specify what if the caller class is not a class loader. The current implementation throws NPE if caller is null. I initially proposed to return false for simplicity. However, if the caller is not a subclass of `ClassLoader`, the current implementation throws `ClassCastException`. Both cases are invalid caller and they should be handled in the same way, either return false or throw an exception. Having a second thought, since this API expects to be called by a class loader, I think throwing `IllegalCallerException` to indicate this method is called by an illegal caller.This will need a CSR due to the spec change. What do you think? - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: 8279995: jpackage --add-launcher option should allow overriding description [v2]
On Fri, 11 Feb 2022 21:22:44 GMT, Alexander Matveev
wrote:
>> Added ability to override description for additional launchers via
>> "description" property.
>
> Alexander Matveev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8279995: jpackage --add-launcher option should allow overriding description
> [v2]
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/AdditionalLauncher.java line
90:
> 88: .findFirst().orElse(null);
> 89:
> 90: return entry == null ? null : entry.getValue();
This can be simply
`rawProperties.stream().findAny(item -> item.getKey().equals(key)).map(e ->
e.getValue()).orElse(null);`
Or slightly better:
public String getRawPropertyValue(String key, Supplier getDefault) {
return rawProperties.stream().findAny(item ->
item.getKey().equals(key)).map(e -> e.getValue()).orElseGet(getDefault);
}
Then we can create a function that will return the expected description of an
additional launcher:
private String getDesciption(JPackageCommand cmd) {
return getRawPropertyValue("description", () ->
cmd.getArgumentValue("--description", unused -> "Unknown"));
}
This will let you avoid `if (expectedDescription != null)` checks and
**always** verify launcher description.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/AdditionalLauncher.java line
275:
> 273:
> expectedDescription.equals(lines.get(i).trim());
> 274: }
> 275: }
This piece of code can be placed in `WindowsHelper.getExecutableDesciption(Path
pathToExeFile)` function to make it reusable in other tests if needed. This way
it can be used to verify the description of the main launcher.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/AdditionalLauncher.java line
277:
> 275: }
> 276: }
> 277: TKit.assertTrue(descriptionIsValid, "Invalid file
> description");
I'd use `TKit.assertEquals()` to compare the expected description with the
actual one. This will produce a meaningful error message in log output in case
they don't match.
-
PR: https://git.openjdk.java.net/jdk/pull/7399
Re: [External] : Sequenced Collections
- Original Message - > From: "Stuart Marks" > To: "Remi Forax" > Cc: "core-libs-dev" > Sent: Friday, February 11, 2022 8:25:19 PM > Subject: Re: [External] : Sequenced Collections > Hi Rémi, > > I see that you're trying to reduce the number of interfaces introduced by > unifying > things around an existing interface, List. Yes, it's true that List is an > ordered > collection. However, your analysis conveniently omits other facts about List > that > make it unsuitable as a general "ordered collection" interface. Specifically: > > 1) List supports element access by int index; and > > 2) List is externally ordered. That is, its ordering is determined by a > succession > of API calls, irrespective of element values. This is in contrast to SortedSet > et al > which are internally ordered, in that the ordering is determined by the > element > values. > > The problem with indexed element access is that it creates a bunch of hidden > performance pitfalls for any data structure where element access is other than > O(1). > So get(i) degrades to O(n), binarySearch degrades from O(log n) to O(n). (This > is in > the sequential implementation; the random access implementation degrades to > O(n > log > n)). Apparently innocuous indexed for-loops degrade to quadratic. This is one > of > the > reasons why LinkedList is a bad List implementation. > > If we refactor LinkedHashSet to implement List, we basically have created > another > situation just like LinkedList. That's a step in the wrong direction. Everybody is focused on LinkedList but that's not the problem, the problem is that, unlike every other languages, in Java, doing an indexed loop on a List is a bad idea. There are plenty of other implementations of AbstractSequentialList, other than LinkedList, that exhibits the same bad performance when used in an indexed loop. A simple search on github find a lot of classes that implements AbstractSequentialList https://github.com/search?l=Java=6=%22extends+AbstractSequentialList%22+-luni+-LinkedList=Code In fact, the problem of performance is not something inherent to the List API, it's true for any interface of the collection API, By example, AbstractSet.contains() is linear, CopyOnWriteArrayList/CopyOnWriteArraySet.add() is linear etc. The whole idea of using interfaces in the collection API is at the same time - great because you have more interropt - awful because you have usually no idea of the complexity of the method you call. BTW, if we want to be serious and solve the issue of indexed Loop on List, we should not have stop half way with the enhanced for loop, and also provides a way to get an index for an element when looping And obviously, there is also a need for a compiler warning to explain that doing an indexed loop on a List is bad. > > Turning to internal ordering (SortedSet): it's fundamentally incompatible with > List's external ordering. List has a lot of positional mutation operations > such > as > add(i, obj); after this call, you expect obj to appear at position i. That > can't > work with a SortedSet. I don't propose that SortedSet implements List, i propose to add a new method asList() to SortedSet that return a view of the sorted set as a List. Obviously, like the Set returned by Map.keySet() does not implement all the methods of Set, calling add() throws an UnsupportedOperationException, the method List.add(int,E) will throw an UnsupportedOperationException when called on the result of SortedSet.asList() > > There is implicit positioning semantics in other methods that don't have index > arguments. For example, replaceAll replaces each element of a List with the > result > of calling a function on that element. Crucially, the function result goes > into > the > same location as the original element. That to cannot work with SortedSet. Not with SortedSet, like above, replace() is an optional method, it will not be implemented by the List returned by SortedSet.asList(), but LinkedHashSet.asList() may return a List that implements replace(). > > Well, we can try to deal with these issues somehow, like making certain > methods > throw UnsupportedOperationException, or by relaxing the semantics of the > methods > so > that they no longer have the same element positioning semantics. Either of > these > approaches contorts the List interface to such an extent that it's no longer a > List. As said above, it's a list view, like by example the List returned by Arrays.asList() does not implement add(E)/add(int,E) or remove(int)/remove(E), the view of the List does not have to implement the methods that do a mutation using an index. > > So, no, it's not useful or effective to try to make List be the common > "ordered > collection" interface. It is because this is how people are using List actually. It's the default type which is used to say, the elements are ordered by insertion order. You may think that this is wrong, but i think it's better
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]
On Fri, 11 Feb 2022 13:51:38 GMT, liach wrote:
>> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407),
>> by design, duplicate initialization of ReflectionFactory should be safe as
>> it performs side-effect-free property read actions, and the suggesting of
>> making the `initted` field volatile cannot prevent concurrent initialization
>> either; however, having `initted == true` published without the other
>> fields' values is a possibility, which this patch addresses.
>>
>> This simulates what's done in `CallSite`'s constructor for
>> `ConstantCallSite`. Please feel free to point out the problems with this
>> patch, as I am relatively inexperienced in this field of fences and there
>> are relatively less available documents. (Thanks to
>> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/)
>
> liach has updated the pull request incrementally with two additional commits
> since the last revision:
>
> - The fast path should always come first. good lesson learned!
>restore config field comments
> - Try making the config a record and see if it works
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
620:
> 618: * The configurations exist as an object to avoid race conditions.
> 619: * See bug 8261407. The object methods backed by indy may not be
> available.
> 620: */
This comment is a bit unclear. With the suggestion of moving the static
`ReflectionFactory::config()` method and the comment I suggest below, I think
that should be adequate to understand. If not, we should improve the comment
rather than relying on the readers to read the bug report.
Maybe the comment can be:
Configuration for core reflection which is configurable via system properties.
The user-configured settings are not available during early VM startup.
Note that the object methods of this Config record are called via indy which is
available to use after initPhase1. We can workaround that limitation by
implementing the object methods.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
631:
> 629: // To avoid this penalty we reuse the existing JVM entry
> points
> 630: // for the first few invocations of Methods and Constructors
> and
> 631: // then switch to the bytecode-based implementations.
This block of comment describes why the default for `noInflation` is false.
This can be moved to L645 as the comment for the `DEFAULT` config.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
638:
> 636: // true if deserialization constructor checking is disabled
> 637: boolean disableSerialConstructorChecks
> 638: ) {
Formatting: preferable to follow the style of the local file, e.g. like the
`newConstructor` method declaration
private record Config(boolean noInflation,
int inflationThreshold,
int useDirectMethodHandle,
boolean useNativeAccessorOnly,
boolean disableSerialConstructorChecks) {
```
Same comment for L645-651 and L719-725
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
660:
> 658: * run, before the system properties are set up.
> 659: */
> 660: private static @Stable Config instance;
The comment makes me think that this static field and the static `instance`
method belong to `ReflectionFactory` class. The static initializer of
java.lang.reflect.Method/AccessibleObject causes this class (ReflectionFactory)
to be initialzed early during VM initialization where the configuration is not
available. The `Config` class is just a data type representing the setting.
When the `Config` class is initialized, it's irrelevant. That should help the
readers easier to understand.
A minor update to the comment may help too:
/**
* The configuration is lazily initialized after the module system is
initialized.
*
* The static initializer of this class is run before the system properties are
set up.
* The class initialization is caused by the class initialization of
java.lang.reflect.Method
* (more properly, caused by the class initialization for
java.lang.reflect.AccessibleObject)
* that happens very early VM startup, initPhase1.
*/
private static @Stable Config config;
If this static field is moved back to ReflectionFactory class, your original
variable name `config` and the accessor method `config()` is clear.
`Config::load` can be renamed to `Config::instance` then.
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: JDK-8277520: Implement JDK-8 default methods for IdentityHashMap [v2]
On Thu, 16 Dec 2021 20:48:39 GMT, liach wrote: >> Might need a CSR as now `computeIfAbsent` `computeIfPresent` `compute` >> `merge` would throw CME if the functions modified the map itself, and there >> are corresponding specification changes. > > liach has updated the pull request with a new target base due to a merge or a > rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains five additional commits since the > last revision: > > - Merge branch 'master' of https://git.openjdk.java.net/jdk into > identityhashmap-default > - update dates > - Also test cme for identity hash map > - Fix putIfAbsent > - JDK-8277520: Implement JDK-8 default methods for IdentityHashMap Slightly busy now, will write benchmarks later and publish results for before and after. - PR: https://git.openjdk.java.net/jdk/pull/6532
Re: RFR: 8279995: jpackage --add-launcher option should allow overriding description
On Wed, 9 Feb 2022 07:37:42 GMT, Alexander Matveev wrote: > Added ability to override description for additional launchers via > "description" property. Added automated tests for .exe files in Windows and .desktop files on Linux. - PR: https://git.openjdk.java.net/jdk/pull/7399
Re: RFR: 8279995: jpackage --add-launcher option should allow overriding description [v2]
> Added ability to override description for additional launchers via > "description" property. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8279995: jpackage --add-launcher option should allow overriding description [v2] - Changes: - all: https://git.openjdk.java.net/jdk/pull/7399/files - new: https://git.openjdk.java.net/jdk/pull/7399/files/a764be5b..dbb36905 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7399=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7399=00-01 Stats: 50 lines in 1 file changed: 49 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7399.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7399/head:pull/7399 PR: https://git.openjdk.java.net/jdk/pull/7399
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v3]
On Fri, 11 Feb 2022 15:30:58 GMT, Sam Brannen wrote: >> The getCanonicalName is not specified to behave that way, should be a RFE I >> suppose, but appears to in practice; changed as suggested in subsequent >> push. Thanks. > > Thanks, Joe. > > Regarding the RFE, do you plan to open an issue to address the documentation > for `getCanonicalName`? Filed https://bugs.openjdk.java.net/browse/JDK-8281671 - PR: https://git.openjdk.java.net/jdk/pull/7418
RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null - Commit messages: - Merge branch 'master' into JDK-8281000 - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null Changes: https://git.openjdk.java.net/jdk/pull/7448/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7448=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281000 Stats: 140 lines in 4 files changed: 139 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v3]
On Fri, 11 Feb 2022 20:34:43 GMT, Joe Darcy wrote: >> Two changes to the toString output for annotations to give better source >> fidelity: >> >> 1) For enum constants, call their name method rather than their toString >> method. An enum class can override the toString method to print something >> other than the name. >> >> 2) Switch from using binary names (names with "$" for nested types) to >> canonical names (names with "." with nested types) >> >> Various existing regression tests are updated to accommodate the changes. >> >> Please also review the CSR: >> https://bugs.openjdk.java.net/browse/JDK-8281568 > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Respond to more review feedback. > - Merge branch 'master' into JDK-8281462 > - Respond to review feedback. > - JDK-8281462: Annotation toString output for enum not reusable for source > input PS I checked and the implementation of printing annotation in javac for annotation processing uses enum constant names (rather than toString values) and uses canonical rather than binary names for nested types. - PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v2]
On Fri, 11 Feb 2022 15:24:45 GMT, Sam Brannen wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Respond to review feedback.
>
> src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
> line 148:
>
>> 146: StringBuilder result = new StringBuilder(128);
>> 147: result.append('@');
>> 148: // Guard against shouldn't-happen NPE for a missing canonical
>> name
>
> NIT: A NPE would not be thrown. Rather, `"null"` would be appended to the
> buffer. Right?
Update comment in latest push.
-
PR: https://git.openjdk.java.net/jdk/pull/7418
RFR: JDK-8281003 - MethodHandles::lookup throws NPE if caller is null
JDK-8281003 - MethodHandles::lookup throws NPE if caller is null - Commit messages: - Merge branch 'master' into JDK-8281003 - Merge branch 'master' into JDK-8281003 - removed commented out code - Moved null caller MethodHandles.lookup() test to a more reasonable location - JDK-8281003 - MethodHandles::lookup throws NPE if caller is null Changes: https://git.openjdk.java.net/jdk/pull/7447/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7447=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281003 Stats: 88 lines in 4 files changed: 82 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7447.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7447/head:pull/7447 PR: https://git.openjdk.java.net/jdk/pull/7447
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v3]
> Two changes to the toString output for annotations to give better source > fidelity: > > 1) For enum constants, call their name method rather than their toString > method. An enum class can override the toString method to print something > other than the name. > > 2) Switch from using binary names (names with "$" for nested types) to > canonical names (names with "." with nested types) > > Various existing regression tests are updated to accommodate the changes. > > Please also review the CSR: > https://bugs.openjdk.java.net/browse/JDK-8281568 Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Respond to more review feedback. - Merge branch 'master' into JDK-8281462 - Respond to review feedback. - JDK-8281462: Annotation toString output for enum not reusable for source input - Changes: - all: https://git.openjdk.java.net/jdk/pull/7418/files - new: https://git.openjdk.java.net/jdk/pull/7418/files/2989ff11..0af92ea9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7418=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7418=01-02 Stats: 3897 lines in 70 files changed: 2865 ins; 435 del; 597 mod Patch: https://git.openjdk.java.net/jdk/pull/7418.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7418/head:pull/7418 PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v6]
> 8281631: HashMap.putAll can cause redundant space waste XenoAmess has updated the pull request incrementally with one additional commit since the last revision: 9072610: HashMap.putAll can cause redundant space waste - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/c6654478..4e42cae1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=04-05 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste
On Fri, 11 Feb 2022 18:40:53 GMT, Stuart Marks wrote: > Let's stick with the `Math.ceil` expression please. I'm afraid Math.ceil is much too time costing, but fine if you want. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 18:24:49 GMT, Andrew Haley wrote: > Just multiply by 0.75. > > On a modern design, floating-point multiply is 4 clocks latency, 4 ops/clock > throughput. FP max is 2 clocks latency, conversions int-float and vice versa > 3 clocks latency, 4 ops/clock throughput. Long division is 7-9 clocks, > 2ops/clock throughput. Shift and add 2 clocks, 2/3 ops/clock througput. > Compare is 1 clock, 3 ops/clock throughput, conditional move is 1 clock, 3 > ops/clock throughput. > > Seems like it's a wash. @theRealAph no multiply but divide. besides, did you count the cost for Math.ceil? it is the heaviest part. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v5]
On Fri, 11 Feb 2022 17:10:43 GMT, XenoAmess wrote:
>> 8281631: HashMap.putAll can cause redundant space waste
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 9072610: HashMap.putAll can cause redundant space waste
> > FWIW, (int) Math.ceil(expected / 0.75) and (int) ((expected * 4L + 2L) /
> > 3L) would be equivalent.
>
> No, they are not equivalent. If `expected` exceeds a certain value around
> 1.6bn, then the intermediate result using the second expression will be
> greater than Integer.MAX_VALUE. Casting this to `int` can result in a
> negative number.
> > FWIW, (int) Math.ceil(expected / 0.75) and (int) ((expected * 4L + 2L) /
> > 3L) would be equivalent.
that is exactly why we added this check when it can reach such situation.
if (size > (int) (Integer.MAX_VALUE * 0.75)) {
return Integer.MAX_VALUE;
}
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v5]
On Fri, 11 Feb 2022 18:42:11 GMT, Stuart Marks wrote: > (It's too late now, but I'd suggest avoiding force-pushing changes into a > branch that's opened in a PR. Doing so confuses the comment history, as the > comments refer to code that is no longer present.) got it. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: [External] : Sequenced Collections
Hi Rémi, I see that you're trying to reduce the number of interfaces introduced by unifying things around an existing interface, List. Yes, it's true that List is an ordered collection. However, your analysis conveniently omits other facts about List that make it unsuitable as a general "ordered collection" interface. Specifically: 1) List supports element access by int index; and 2) List is externally ordered. That is, its ordering is determined by a succession of API calls, irrespective of element values. This is in contrast to SortedSet et al which are internally ordered, in that the ordering is determined by the element values. The problem with indexed element access is that it creates a bunch of hidden performance pitfalls for any data structure where element access is other than O(1). So get(i) degrades to O(n), binarySearch degrades from O(log n) to O(n). (This is in the sequential implementation; the random access implementation degrades to O(n log n)). Apparently innocuous indexed for-loops degrade to quadratic. This is one of the reasons why LinkedList is a bad List implementation. If we refactor LinkedHashSet to implement List, we basically have created another situation just like LinkedList. That's a step in the wrong direction. Turning to internal ordering (SortedSet): it's fundamentally incompatible with List's external ordering. List has a lot of positional mutation operations such as add(i, obj); after this call, you expect obj to appear at position i. That can't work with a SortedSet. There is implicit positioning semantics in other methods that don't have index arguments. For example, replaceAll replaces each element of a List with the result of calling a function on that element. Crucially, the function result goes into the same location as the original element. That to cannot work with SortedSet. Well, we can try to deal with these issues somehow, like making certain methods throw UnsupportedOperationException, or by relaxing the semantics of the methods so that they no longer have the same element positioning semantics. Either of these approaches contorts the List interface to such an extent that it's no longer a List. So, no, it's not useful or effective to try to make List be the common "ordered collection" interface. s'marks On 2/10/22 3:14 AM, Remi Forax wrote: I've read the draft of the JEP on sequenced collection, and i think the proposed design can be improved. https://bugs.openjdk.java.net/browse/JDK-8280836 I agree with the motivation, there is a need for an API to consider the element of a list, a sorted set and a linked hash set as an ordered sequence of elements with a simple way to access/add/remove the first/last element and also reverse the elements as view. I disagree about the conclusion that we need to introduce 4 new interfaces for that matter. Here are the reasons 1/ Usually an ordered collection is called a list. Introducing an interface SequencedCollection for something which is usually called a list will cause more harm than good. Or maybe we should rename LISP to SEQP :) 2/ There is already an interface List in Java, that represents an ordered sequence of elements, with LinkedList being the name of the the double linked list implementation. You can argue that there is a slight difference between the semantics of java.util.List and the proposed syntax of java.util.SequencedCollection, but given that people already have difficulties to understand basic data structure concepts, as a teacher i dread to have a discussion on those slight differences that are only true in Java. If the collection API was not already existing, we may discuss about having the same interface java.util.List to both indexed collection and ordered collection, but that boat has sailed a long time ago. So in first approach, we should refactor sorted set and linked hash set to directly implement java.util.List and all the proposed methods into java.util.List. But as you hint in the Risks and Assumptions section, this will cause regression due to inference and also we will have trouble with LinkedHashMap (see below). 3/ LinkedHashMap mixes 3 implementations in one class, some of these implementations does not conform to the semantics of SequencedMap. - You can opt-out having the key sequentially ordered as defined by SequencedMap by using the constructor LinkedHashMap(int initialCapacity, float loadFactor, boolean accessOrder) and passing true as last parameter. - You can opt-out having the key sequentially ordered as defined by SequencedMap by overriding removeEldestEntry(), removing the first entry at the same time you add a new one. Because all these reasons, i think we should move to another design, using delegation instead of inheritance, which for the collection framework means exposing new way to access/modify sorted set and linked hash set through java.util.List views. The concept of views is not a new
Integrated: JDK-8277175 : Add a parallel multiply method to BigInteger
On Tue, 16 Nov 2021 13:22:46 GMT, kabutz wrote: > BigInteger currently uses three different algorithms for multiply. The simple > quadratic algorithm, then the slightly better Karatsuba if we exceed a bit > count and then Toom Cook 3 once we go into the several thousands of bits. > Since Toom Cook 3 is a recursive algorithm, it is trivial to parallelize it. > I have demonstrated this several times in conference talks. In order to be > consistent with other classes such as Arrays and Collection, I have added a > parallelMultiply() method. Internally we have added a parameter to the > private multiply method to indicate whether the calculation should be done in > parallel. > > The performance improvements are as should be expected. Fibonacci of 100 > million (using a single-threaded Dijkstra's sum of squares version) completes > in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with the > sequential multiply() method. This is on my 1-8-2 laptop. The final > multiplications are with very large numbers, which then benefit from the > parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number. > > We have also parallelized the private square() method. Internally, the > square() method defaults to be sequential. > > Some benchmark results, run on my 1-6-2 server: > > > Benchmark (n) Mode Cnt Score > Error Units > BigIntegerParallelMultiply.multiply100ss4 51.707 > ± 11.194 ms/op > BigIntegerParallelMultiply.multiply 1000ss4988.302 > ± 235.977 ms/op > BigIntegerParallelMultiply.multiply 1ss4 24662.063 > ± 1123.329 ms/op > BigIntegerParallelMultiply.parallelMultiply100ss4 49.337 > ± 26.611 ms/op > BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560 > ± 268.903 ms/op > BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551 > ± 1899.444 ms/op > > > We can see that for larger calculations (fib 100m), the execution is 2.7x > faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for > small (fib 1m) it is roughly the same. Considering that the fibonacci > algorithm that we used was in itself sequential, and that the last 3 > calculations would dominate, 2.7x faster should probably be considered quite > good on a 1-6-2 machine. This pull request has now been integrated. Changeset: 83ffbd2e Author:Dr Heinz M. Kabutz Committer: Paul Sandoz URL: https://git.openjdk.java.net/jdk/commit/83ffbd2e7aed8a9c788395ccbe920ddff221ae16 Stats: 601 lines in 4 files changed: 582 ins; 0 del; 19 mod 8277175: Add a parallel multiply method to BigInteger Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v5]
On Fri, 11 Feb 2022 17:10:43 GMT, XenoAmess wrote: >> 8281631: HashMap.putAll can cause redundant space waste > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > 9072610: HashMap.putAll can cause redundant space waste (It's too late now, but I'd suggest avoiding force-pushing changes into a branch that's opened in a PR. Doing so confuses the comment history, as the comments refer to code that is no longer present.) - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste
On Fri, 11 Feb 2022 12:25:08 GMT, XenoAmess wrote: > FWIW, (int) Math.ceil(expected / 0.75) and (int) ((expected * 4L + 2L) / 3L) > would be equivalent. No, they are not equivalent. If `expected` exceeds a certain value around 1.6bn, then the intermediate result using the second expression will be greater than Integer.MAX_VALUE. Casting this to `int` can result in a negative number. In the first expression, a double value that exceeds the range of `int` will saturate at Integer.MAX_VALUE. jshell> (int) ((17 * 4L + 2L) / 3L) $24 ==> -2028300629 jshell> (int) Math.ceil(17 / 0.75) $25 ==> 2147483647 Let's stick with the `Math.ceil` expression please. - PR: https://git.openjdk.java.net/jdk/pull/7431
RFR: JDK-8266670: Better modeling of access flags in core reflection
This is an early review of changes to better model JVM access flags, that is "modifiers" like public, protected, etc. but explicitly at a VM level. Language level modifiers and JVM level access flags are closely related, but distinct. There are concepts that overlap in the two domains (public, private, etc.), others that only have a language-level modifier (sealed), and still others that only have an access flag (synthetic). The existing java.lang.reflect.Modifier class is inadequate to model these subtleties. For example, the bit positions used by access flags on different kinds of elements overlap (such as "volatile" for fields and "bridge" for methods. Just having a raw integer does not provide sufficient context to decode the corresponding language-level string. Methods like Modifier.methodModifiers() were introduced to cope with this situation. With additional modifiers and flags on the horizon with projects like Valhalla, addressing the existent modeling deficiency now ahead of time is reasonable before further strain is introduced. This PR in its current form is meant to give the overall shape of the API. It is missing implementations to map from, say, method modifiers to access flags, taking into account overlaps in bit positions. The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in once the API is further along. - Commit messages: - JDK-8266670: Better modeling of modifiers in core reflection Changes: https://git.openjdk.java.net/jdk/pull/7445/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7445=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266670 Stats: 345 lines in 6 files changed: 345 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7445.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7445/head:pull/7445 PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 18:13:36 GMT, Roger Riggs wrote: >> @RogerRiggs >> so you recommend `(int) Math.min(((m.size() * 4L + 2L) / 3L), >> Integer.MAX_VALUE)`? OK then, changed it. >> please review again, thanks! > > Works for me. Thanks Just multiply by 0.75. On a modern design, floating-point multiply is 4 clocks latency, 4 ops/clock throughput. FP max is 2 clocks latency, conversions int-float and vice versa 3 clocks latency, 4 ops/clock throughput. Long division is 7-9 clocks, 2ops/clock throughput. Shift and add 2 clocks, 2/3 ops/clock througput. Compare is 1 clock, 3 ops/clock throughput, conditional move is 1 clock, 3 ops/clock throughput. Seems like it's a wash. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 17:04:14 GMT, XenoAmess wrote: >> Performance is a lesser issue. Given all of the other computation that >> occurs to populate the map, this computation is in the noise. It is also >> likely that with more instructions to fetch and execute and a possible >> branch, the integer check is slower. >> With current hardware, the long divide dominates the cost. Addition is >> almost free. >> >> Code maintainability is more important; keep the source code simple and >> concise. > > @RogerRiggs > so you recommend `(int) Math.min(((m.size() * 4L + 2L) / 3L), > Integer.MAX_VALUE)`? OK then, changed it. > please review again, thanks! Works for me. Thanks - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
On Fri, 11 Feb 2022 10:11:50 GMT, Maurizio Cimadamore wrote: > there is still residual common logic in how clients are expected to load > libraries (e.g. either using a file name/absolute path, or using a library > name, without file separator chars). These assumption seem very heavily > influenced by how System::load/loadLibrary do things, I believe - and I > wonder if they can, in the future, create other obstacles for a raw kind of > library loading (which expects to be mostly a wrapper around > dlopen/LoadLibrary). But that's a question/problem for another day. Good point. With the removal of the restriction, the raw library loading implementation can further be cleaned up (not be tied with the jni library loading logic). I'll look into what it takes and whether I should do it in this PR. - PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v3]
On Fri, 11 Feb 2022 13:45:47 GMT, Alan Bateman wrote:
>> Lance Andersen has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Return a null InputStream when the ZipEntry is not found in the Jar
>> - Address formatting and message feedback
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 881:
>
>> 879: ze = getJarEntry(entryName);
>> 880: } else {
>> 881: throw new ZipException("ZipEntry::getName returned null");
>
> Throwing ZipException when ZipEntry::getName returns null is still surprising
> but not terrible. The spec for getInputStream specifies ZipException for
> when a zip file format occurs so we might have to extend that to add "or the
> zip entry name is null".
If you would like me to update the ZipException to clarify this I can. The
original issue was due to a format error in the CEN so the current wording
covers that. It does not cover the case where ZipEntry is subclassed and
ZipEntry::getName() returns null which is what your suggested wording would
address.
Please note the above change addresses the signed jar scenario. I can add an
additional check in JarFile::getInputStream to check for null from
ZipEntry::getName so that it covers unsigned jars and signed jars not being
verified.
The issue will not occur with ZipEntry::getInputStream given its use of
ZipEntry.name which can never be null.
Please let me know your preference
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]
On Fri, 11 Feb 2022 13:51:38 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making the `initted` field volatile cannot prevent concurrent initialization >> either; however, having `initted == true` published without the other >> fields' values is a possibility, which this patch addresses. >> >> This simulates what's done in `CallSite`'s constructor for >> `ConstantCallSite`. Please feel free to point out the problems with this >> patch, as I am relatively inexperienced in this field of fences and there >> are relatively less available documents. (Thanks to >> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/) > > liach has updated the pull request incrementally with two additional commits > since the last revision: > > - The fast path should always come first. good lesson learned! >restore config field comments > - Try making the config a record and see if it works mandy, does this patch look good? i added a comment to warn about indy for records so as to avoid future unintended problems, while the current setup looks safe. - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 16:32:29 GMT, Roger Riggs wrote:
>> @RogerRiggs
>>
>> Hi. I added a overflow check.
>>
>> The check makes sure it cannot overflow now.
>>
>> I wrote a test to show this overflow check be correct.
>>
>>
>> public class A {
>>
>> /**
>> * use for calculate HashMap capacity, using default load factor 0.75
>> *
>> * @param size size
>> * @return HashMap capacity under default load factor
>> */
>> private static int calculateHashMapCapacity(int size) {
>> if (size > (int) (Integer.MAX_VALUE * 0.75)) {
>> return Integer.MAX_VALUE;
>> }
>> return size + (size + 2) / 3;
>> }
>>
>> public static void main(String[] args) {
>> for (int i = 1; i < Integer.MAX_VALUE ; ++i) {
>> if (calculateHashMapCapacity(i) <= 0) {
>> System.err.println(i);
>> return;
>> }
>> }
>> }
>> }
>>
>>
>>
>> I also see the compiled result to make sure the `(int) (Integer.MAX_VALUE *
>> 0.75)` calculation is made at compiling but not runtime, which will not make
>> this function too much slower.
>>
>> please review again, thanks!
>
> Performance is a lesser issue. Given all of the other computation that occurs
> to populate the map, this computation is in the noise. It is also likely
> that with more instructions to fetch and execute and a possible branch, the
> integer check is slower.
> With current hardware, the long divide dominates the cost. Addition is
> almost free.
>
> Code maintainability is more important; keep the source code simple and
> concise.
@RogerRiggs
so you recommend `(int) Math.min(((m.size() * 4L + 2L) / 3L),
Integer.MAX_VALUE)`? OK then, changed it.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v5]
> 8281631: HashMap.putAll can cause redundant space waste XenoAmess has updated the pull request incrementally with one additional commit since the last revision: 9072610: HashMap.putAll can cause redundant space waste - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/2a1bf274..c6654478 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=03-04 Stats: 14 lines in 1 file changed: 0 ins; 13 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
On Fri, 11 Feb 2022 13:42:01 GMT, liach wrote: >> ...having suggested that rearrangement, perhaps the right way to do it is to >> enable some VM.isXXX queries themselves to be constant-foldable so that >> other callers too may benefit. Like this: >> https://github.com/plevart/jdk/commit/e918ccc52bbc288f6721af5fa66d8f7a8cc880cf >> WDYT? > > I believe your patch to fold these methods is a good choice: for example, > `FileSystems.getDefault()` will be constant-foldable as a result. > For shutdown, the benefit may look negligible, but a consistency in style is > beneficial. > To make this more efficient, I recommend looking at the callers to > `VM.initLevel()` and replace with such boolean checks if possible: for > example, `ClassLoader.getSystemClassLoader` may be constant-foldable if its > default branch of switch on init level become a dedicated fast path. > > Since this change affects multiple components and beyond the reflection > factory itself, I don't think I will include it here; I will just use the > right arrangement. This belongs to another jbs ticket. Right, I wasn't suggesting to include that in the patch. Local rearrangement is OK anyway. Latest code looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]
On Fri, 11 Feb 2022 13:51:38 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making the `initted` field volatile cannot prevent concurrent initialization >> either; however, having `initted == true` published without the other >> fields' values is a possibility, which this patch addresses. >> >> This simulates what's done in `CallSite`'s constructor for >> `ConstantCallSite`. Please feel free to point out the problems with this >> patch, as I am relatively inexperienced in this field of fences and there >> are relatively less available documents. (Thanks to >> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/) > > liach has updated the pull request incrementally with two additional commits > since the last revision: > > - The fast path should always come first. good lesson learned! >restore config field comments > - Try making the config a record and see if it works Marked as reviewed by plevart (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 15:48:44 GMT, XenoAmess wrote:
>> src/java.base/share/classes/java/util/WeakHashMap.java line 247:
>>
>>> 245: */
>>> 246: private static int calculateHashMapCapacity(int size){
>>> 247: return size + (size + 2) / 3;
>>
>> Consider integer overflow; if size were Integer.MAX_VALUE / 2; the
>> computation would overflow.
>> The negative result would eventually throw an exception. Using Long for the
>> computation would avoid that and keep the expression simple.
>
> @RogerRiggs
>
> Hi. I added a overflow check.
>
> The check makes sure it cannot overflow now.
>
> I wrote a test to show this overflow check be correct.
>
>
> public class A {
>
> /**
> * use for calculate HashMap capacity, using default load factor 0.75
> *
> * @param size size
> * @return HashMap capacity under default load factor
> */
> private static int calculateHashMapCapacity(int size) {
> if (size > (int) (Integer.MAX_VALUE * 0.75)) {
> return Integer.MAX_VALUE;
> }
> return size + (size + 2) / 3;
> }
>
> public static void main(String[] args) {
> for (int i = 1; i < Integer.MAX_VALUE ; ++i) {
> if (calculateHashMapCapacity(i) <= 0) {
> System.err.println(i);
> return;
> }
> }
> }
> }
>
>
>
> I also see the compiled result to make sure the `(int) (Integer.MAX_VALUE *
> 0.75)` calculation is made at compiling but not runtime, which will not make
> this function too much slower.
>
> please review again, thanks!
Performance is a lesser issue. Given all of the other computation that occurs
to populate the map, this computation is in the noise. It is also likely that
with more instructions to fetch and execute and a possible branch, the integer
check is slower.
With current hardware, the long divide dominates the cost. Addition is almost
free.
Code maintainability is more important; keep the source code simple and concise.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Integrated: 8281259: MutableBigInteger subtraction could be simplified
On Fri, 4 Feb 2022 10:13:28 GMT, Daniel Jeliński wrote: > The proposed form of borrow bit handling is [already used in BigInteger > class](https://github.com/djelinski/jdk/blob/ce26a19be5e907c11164b46f1bcb370b534d5bf6/src/java.base/share/classes/java/math/BigInteger.java#L1558). > > JMH results without the patch: > > Benchmark(maxNumbits) Mode CntScoreError Units > BigIntegers.testGcd 256 avgt 25 3181205,367 ± 155272,427 ns/op > > JMH results with the patch applied: > > Benchmark(maxNumbits) Mode CntScore Error Units > BigIntegers.testGcd 256 avgt 25 2696030,849 ± 14141,940 ns/op This pull request has now been integrated. Changeset: e73ee0ca Author:Daniel Jeliński Committer: Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/e73ee0ca10b644600ee3747b901e5f69104d03df Stats: 16 lines in 2 files changed: 11 ins; 0 del; 5 mod 8281259: MutableBigInteger subtraction could be simplified Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/7342
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 15:04:03 GMT, Roger Riggs wrote:
>> XenoAmess has refreshed the contents of this pull request, and previous
>> commits have been removed. The incremental views will show differences
>> compared to the previous content of the PR. The pull request contains one
>> new commit since the last revision:
>>
>> 9072610: HashMap.putAll can cause redundant space waste
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 247:
>
>> 245: */
>> 246: private static int calculateHashMapCapacity(int size){
>> 247: return size + (size + 2) / 3;
>
> Consider integer overflow; if size were Integer.MAX_VALUE / 2; the
> computation would overflow.
> The negative result would eventually throw an exception. Using Long for the
> computation would avoid that and keep the expression simple.
@RogerRiggs
Hi. I added a overflow check.
The check makes sure it cannot overflow now.
I wrote a test to show this overflow check be correct.
public class A {
/**
* use for calculate HashMap capacity, using default load factor 0.75
*
* @param size size
* @return HashMap capacity under default load factor
*/
private static int calculateHashMapCapacity(int size) {
if (size > (int) (Integer.MAX_VALUE * 0.75)) {
return Integer.MAX_VALUE;
}
return size + (size + 2) / 3;
}
public static void main(String[] args) {
for (int i = 1; i < Integer.MAX_VALUE ; ++i) {
if (calculateHashMapCapacity(i) <= 0) {
System.err.println(i);
return;
}
}
}
}
I also see the compiled result to make sure the `(int) (Integer.MAX_VALUE *
0.75)` calculation is made at compiling but not runtime, which will not make
this function too much slower.
please review again, thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v4]
> 8281631: HashMap.putAll can cause redundant space waste XenoAmess has updated the pull request incrementally with one additional commit since the last revision: 9072610: HashMap.putAll can cause redundant space waste - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/bb42df9c..2a1bf274 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=02-03 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v2]
On Fri, 11 Feb 2022 12:11:54 GMT, Claes Redestad wrote: >> I'm requesting comments and, hopefully, some help with this patch to replace >> `StringCoding.hasNegatives` with `countPositives`. The new method does a >> very similar pass, but alters the intrinsic to return the number of leading >> bytes in the `byte[]` range which only has positive bytes. This allows for >> dealing much more efficiently with those `byte[]`s that has a ASCII prefix, >> with no measurable cost on ASCII-only or latin1/UTF16-mostly input. >> >> Microbenchmark results: >> https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 >> >> - Only implemented on x86 for now, but I want to verify that implementations >> of `countPositives` can be implemented with similar efficiency on all >> platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) >> before moving ahead. This pretty much means holding up this until it's >> implemented on all platforms, which can either contributed to this PR or as >> dependent follow-ups. >> >> - An alternative to holding up until all platforms are on board is to allow >> the implementation of `StringCoding.hasNegatives` and `countPositives` to be >> implemented so that the non-intrinsified method calls into the intrinsified. >> This requires structuring the implementations differently based on which >> intrinsic - if any - is actually implemented. One way to do this could be to >> mimic how `java.nio` handles unaligned accesses and expose which intrinsic >> is available via `Unsafe` into a `static final` field. >> >> - There are a few minor regressions (~5%) in the x86 implementation on >> `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, >> for example `encode-/decodeShortMixed` even see a minor improvement, which >> makes me consider those corner case regressions with little real world >> implications (if you have latin1 Strings, you're likely to also have >> ASCII-only strings in your mix). > > Claes Redestad 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 23 additional > commits since the last revision: > > - Merge branch 'master' into count_positives > - Restore partial vector checks in AVX2 and SSE intrinsic variants > - Let countPositives use hasNegatives to allow ports not implementing the > countPositives intrinsic to stay neutral > - Simplify changes to encodeUTF8 > - Fix little-endian error caught by testing > - Reduce jumps in the ascii path > - Remove unused tail_mask > - Remove has_negatives intrinsic on x86 (and hook up 32-bit x86 to use > count_positives) > - Add more comments, simplify tail branching in AVX512 variant > - Resolve issues in the precise implementation > - ... and 13 more: > https://git.openjdk.java.net/jdk/compare/690b05fa...c4bb3612 Good! I'm currently reading up on aarch64 asm and trying to port that intrinsic over. It might take some time.. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v2]
On Fri, 11 Feb 2022 12:11:54 GMT, Claes Redestad wrote: >> I'm requesting comments and, hopefully, some help with this patch to replace >> `StringCoding.hasNegatives` with `countPositives`. The new method does a >> very similar pass, but alters the intrinsic to return the number of leading >> bytes in the `byte[]` range which only has positive bytes. This allows for >> dealing much more efficiently with those `byte[]`s that has a ASCII prefix, >> with no measurable cost on ASCII-only or latin1/UTF16-mostly input. >> >> Microbenchmark results: >> https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 >> >> - Only implemented on x86 for now, but I want to verify that implementations >> of `countPositives` can be implemented with similar efficiency on all >> platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) >> before moving ahead. This pretty much means holding up this until it's >> implemented on all platforms, which can either contributed to this PR or as >> dependent follow-ups. >> >> - An alternative to holding up until all platforms are on board is to allow >> the implementation of `StringCoding.hasNegatives` and `countPositives` to be >> implemented so that the non-intrinsified method calls into the intrinsified. >> This requires structuring the implementations differently based on which >> intrinsic - if any - is actually implemented. One way to do this could be to >> mimic how `java.nio` handles unaligned accesses and expose which intrinsic >> is available via `Unsafe` into a `static final` field. >> >> - There are a few minor regressions (~5%) in the x86 implementation on >> `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, >> for example `encode-/decodeShortMixed` even see a minor improvement, which >> makes me consider those corner case regressions with little real world >> implications (if you have latin1 Strings, you're likely to also have >> ASCII-only strings in your mix). > > Claes Redestad 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 23 additional > commits since the last revision: > > - Merge branch 'master' into count_positives > - Restore partial vector checks in AVX2 and SSE intrinsic variants > - Let countPositives use hasNegatives to allow ports not implementing the > countPositives intrinsic to stay neutral > - Simplify changes to encodeUTF8 > - Fix little-endian error caught by testing > - Reduce jumps in the ascii path > - Remove unused tail_mask > - Remove has_negatives intrinsic on x86 (and hook up 32-bit x86 to use > count_positives) > - Add more comments, simplify tail branching in AVX512 variant > - Resolve issues in the precise implementation > - ... and 13 more: > https://git.openjdk.java.net/jdk/compare/811eb365...c4bb3612 Hi Claes, doing it for all platforms and cleaning it up sounds good. My PPC64 contribution is already tested and reviewed. I'll try to find a volunteer for s390 which uses exactly the same algorithm as PPC64. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v2]
On Thu, 10 Feb 2022 22:12:57 GMT, Joe Darcy wrote: >> Two changes to the toString output for annotations to give better source >> fidelity: >> >> 1) For enum constants, call their name method rather than their toString >> method. An enum class can override the toString method to print something >> other than the name. >> >> 2) Switch from using binary names (names with "$" for nested types) to >> canonical names (names with "." with nested types) >> >> Various existing regression tests are updated to accommodate the changes. >> >> Please also review the CSR: >> https://bugs.openjdk.java.net/browse/JDK-8281568 > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Marked as reviewed by sbran...@github.com (no known OpenJDK username). - PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v2]
On Thu, 10 Feb 2022 22:09:16 GMT, Joe Darcy wrote: >> src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java >> line 256: >> >>> 254: return Objects.toString(finalComponent.getCanonicalName(), >>> 255: "") + >>> 256: arrayBrackets.toString() + ".class"; >> >> Since we're using the canonical name now (which takes the array brackets >> into account), can't the whole method be simplified down to the following? >> >> Suggestion: >> >> return Objects.toString(clazz.getCanonicalName(), "> name>") + ".class"; > > The getCanonicalName is not specified to behave that way, should be a RFE I > suppose, but appears to in practice; changed as suggested in subsequent push. > Thanks. Thanks, Joe. Regarding the RFE, do you plan to open an issue to address the documentation for `getCanonicalName`? - PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 15:04:03 GMT, Roger Riggs wrote: > if size were Integer.MAX_VALUE / 2; the computation would overflow Actually will not, it must be slightly larger. it will only overflow when it be larger than Integer.MAX_VALUE * 0.75 But yes, it can overflow when there be a map as large as that. > Using Long for the computation would avoid that and keep the expression > simple. but it be slower. I do think a int check can do same thing, and would add it . Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v2]
On Thu, 10 Feb 2022 22:12:57 GMT, Joe Darcy wrote:
>> Two changes to the toString output for annotations to give better source
>> fidelity:
>>
>> 1) For enum constants, call their name method rather than their toString
>> method. An enum class can override the toString method to print something
>> other than the name.
>>
>> 2) Switch from using binary names (names with "$" for nested types) to
>> canonical names (names with "." with nested types)
>>
>> Various existing regression tests are updated to accommodate the changes.
>>
>> Please also review the CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8281568
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Respond to review feedback.
src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
line 148:
> 146: StringBuilder result = new StringBuilder(128);
> 147: result.append('@');
> 148: // Guard against shouldn't-happen NPE for a missing canonical
> name
NIT: A NPE would not be thrown. Rather, `"null"` would be appended to the
buffer. Right?
-
PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
On Fri, 11 Feb 2022 13:04:38 GMT, XenoAmess wrote:
>> 8281631: HashMap.putAll can cause redundant space waste
>
> XenoAmess has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 9072610: HashMap.putAll can cause redundant space waste
src/java.base/share/classes/java/util/WeakHashMap.java line 247:
> 245: */
> 246: private static int calculateHashMapCapacity(int size){
> 247: return size + (size + 2) / 3;
Consider integer overflow; if size were Integer.MAX_VALUE / 2; the computation
would overflow.
The negative result would eventually throw an exception. Using Long for the
computation would avoid that and keep the expression simple.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]
> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), > by design, duplicate initialization of ReflectionFactory should be safe as it > performs side-effect-free property read actions, and the suggesting of making > the `initted` field volatile cannot prevent concurrent initialization either; > however, having `initted == true` published without the other fields' values > is a possibility, which this patch addresses. > > This simulates what's done in `CallSite`'s constructor for > `ConstantCallSite`. Please feel free to point out the problems with this > patch, as I am relatively inexperienced in this field of fences and there are > relatively less available documents. (Thanks to > https://shipilev.net/blog/2014/on-the-fence-with-dependencies/) liach has updated the pull request incrementally with two additional commits since the last revision: - The fast path should always come first. good lesson learned! restore config field comments - Try making the config a record and see if it works - Changes: - all: https://git.openjdk.java.net/jdk/pull/6889/files - new: https://git.openjdk.java.net/jdk/pull/6889/files/f32ff988..affda902 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6889=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6889=05-06 Stats: 55 lines in 1 file changed: 9 ins; 19 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/6889.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6889/head:pull/6889 PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v3]
On Thu, 10 Feb 2022 21:35:56 GMT, Lance Andersen wrote:
>> Hi all,
>>
>> Please review the attached patch to address
>>
>> - That JarFile::getInputStream did not check for a null ZipEntry passed as a
>> parameter
>> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an
>> unexpected exception occurs
>>
>> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar
>> test runs
>>
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Return a null InputStream when the ZipEntry is not found in the Jar
> - Address formatting and message feedback
src/java.base/share/classes/java/util/jar/JarFile.java line 881:
> 879: ze = getJarEntry(entryName);
> 880: } else {
> 881: throw new ZipException("ZipEntry::getName returned null");
Throwing ZipException when ZipEntry::getName returns null is still surprising
but not terrible. The spec for getInputStream specifies ZipException for when
a zip file format occurs so we might have to extend that to add "or the zip
entry name is null".
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
On Fri, 11 Feb 2022 08:25:16 GMT, Peter Levart wrote:
>> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
>> 685:
>>
>>> 683: instance = c = load();
>>> 684: }
>>> 685: return c;
>>
>> If you do that the "old" way, you loose the ability for JIT to constant-fold
>> the `instance` and by transitivity the Config instance fields, since the
>> check for `VM.isModuleSystemInited()` can't be elided. As suggested, the
>> fast-path check should be done 1st, like:
>>
>>
>> private static @Stable Config instance;
>>
>> private static Config instance() {
>> Config c = instance;
>> if (c != null) {
>> return c;
>> }
>>
>> // Defer initialization until module system is initialized so as
>> // to avoid inflation and spinning bytecode in unnamed modules
>> // during early startup.
>> if (!VM.isModuleSystemInited()) {
>> return DEFAULT;
>> }
>>
>> instance = c = load();
>> return c;
>> }
>
> ...having suggested that rearrangement, perhaps the right way to do it is to
> enable some VM.isXXX queries themselves to be constant-foldable so that other
> callers too may benefit. Like this:
> https://github.com/plevart/jdk/commit/e918ccc52bbc288f6721af5fa66d8f7a8cc880cf
> WDYT?
I believe your patch to fold these methods is a good choice: for example,
`FileSystems.getDefault()` will be constant-foldable as a result.
For shutdown, the benefit may look negligible, but a consistency in style is
beneficial.
To make this more efficient, I recommend looking at the callers to
`VM.initLevel()` and replace with such boolean checks if possible: for example,
`ClassLoader.getSystemClassLoader` may be constant-foldable if its default
branch of switch on init level become a dedicated fast path.
Since this change affects multiple components and beyond the reflection factory
itself, I don't think I will include it here; I will just use the right
arrangement.
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v3]
> 8281631: HashMap.putAll can cause redundant space waste XenoAmess has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 9072610: HashMap.putAll can cause redundant space waste - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/f18fc5e4..bb42df9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=01-02 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc [v2]
On Fri, 11 Feb 2022 08:54:51 GMT, Leo Korinth wrote: >> Remove unused imports under test/lib and jtreg/gc. They create lots of >> warnings if editing using an IDE. Tests in hotspot_gc passed. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > updating copyright Looks good. Thanks. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7426
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste [v2]
> 8281631: HashMap.putAll can cause redundant space waste XenoAmess has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 9072610: HashMap.putAll can cause redundant space waste - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/b4098ac6..f18fc5e4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=00-01 Stats: 14 lines in 3 files changed: 10 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste
On Fri, 11 Feb 2022 11:55:13 GMT, stefan-zobel wrote: > > I investigated most of the usages. They just give a size, and get a > > capacity, even not change the 0.75 So maybe we can use some int calculation > > to replace the 0.75, thus replace Math.ceil for such situations. > > FWIW, `(int) Math.ceil(expected / 0.75)` and `(int) ((expected * 4L + 2L) / > 3L)` would be equivalent. Yes, and `(int) ((expected * 4L + 2L) / 3L)` actually equals `(expected + (expected + 2) / 3)` IMO, thus even avoid cast to long. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v2]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad 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 23 additional commits since the last revision: - Merge branch 'master' into count_positives - Restore partial vector checks in AVX2 and SSE intrinsic variants - Let countPositives use hasNegatives to allow ports not implementing the countPositives intrinsic to stay neutral - Simplify changes to encodeUTF8 - Fix little-endian error caught by testing - Reduce jumps in the ascii path - Remove unused tail_mask - Remove has_negatives intrinsic on x86 (and hook up 32-bit x86 to use count_positives) - Add more comments, simplify tail branching in AVX512 variant - Resolve issues in the precise implementation - ... and 13 more: https://git.openjdk.java.net/jdk/compare/42073fce...c4bb3612 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7231/files - new: https://git.openjdk.java.net/jdk/pull/7231/files/2a855eb6..c4bb3612 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7231=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7231=00-01 Stats: 18287 lines in 533 files changed: 12765 ins; 2983 del; 2539 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste
On Thu, 10 Feb 2022 18:09:19 GMT, XenoAmess wrote: > I investigated most of the usages. They just give a size, and get a capacity, > even not change the 0.75 So maybe we can use some int calculation to replace > the 0.75, thus replace Math.ceil for such situations. FWIW, `(int) Math.ceil(expected / 0.75)` and `(int) ((expected * 4L + 2L) / 3L)` would be equivalent. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives
On Wed, 26 Jan 2022 12:51:31 GMT, Claes Redestad wrote: > I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). > Hi Claes, it can get implemented similarly on PPC64: #7430 You can integrate > it if you prefer that, but better after it got a Review. Hi Martin, perfect! Ideally we can get all platforms that has a `hasNegatives` intrinsic moved over so we can just switch it over big-bang style: remove the `@IntrinsicCandidate`, avoid contortions to pick the "right" implementation on the Java level based on which intrinsic is available and drop all VM-internal scaffolding for `hasNegatives`. Then it makes perfect sense to fold your patch into this PR, rather than have a tail of follow-ups. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
On Fri, 11 Feb 2022 03:49:45 GMT, Mandy Chung wrote: >> This patch removes the restriction in the raw library loading mechanism that >> does not allow mix-n-match of loading a library as a JNI library and as a >> raw library. >> >> The raw library loading mechanism is designed for panama to load native >> library essentially equivalent to dlopen/dlclose calls independent of JNI >> library loading. If a native library is loaded as a JNI library and a raw >> library, it will get different NativeLibrary instances. When a class loader >> is being unloaded, JNI_Unload will be invoked but the native library may not >> be unloaded until NativeLibrary::unload is explicitly called for the raw >> library. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review comment Looks good. I like the fact that the factory for raw library now takes a MH.lookup instead of a class. And I also like that the paths for loading a library in raw mode vs. JNI mode are more clearly separated, I think this should help Panama, thanks! One fly-by comment is that there is still residual common logic in how clients are expected to load libraries (e.g. either using a file name/absolute path, or using a library name, without file separator chars). These assumption seem very heavily influenced by how System::load/loadLibrary do things, I believe - and I wonder if they can, in the future, create other obstacles for a raw kind of library loading (which expects to be mostly a wrapper around dlopen/LoadLibrary). But that's a question/problem for another day. - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc [v2]
On Fri, 11 Feb 2022 08:54:51 GMT, Leo Korinth wrote: >> Remove unused imports under test/lib and jtreg/gc. They create lots of >> warnings if editing using an IDE. Tests in hotspot_gc passed. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > updating copyright Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7426
RFR: 8281631: HashMap.putAll can cause redundant space waste
8281631: HashMap.putAll can cause redundant space waste - Commit messages: - 9072610: HashMap.putAll can cause redundant space waste Changes: https://git.openjdk.java.net/jdk/pull/7431/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7431=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281631 Stats: 8 lines in 2 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc [v2]
> Remove unused imports under test/lib and jtreg/gc. They create lots of > warnings if editing using an IDE. Tests in hotspot_gc passed. Leo Korinth has updated the pull request incrementally with one additional commit since the last revision: updating copyright - Changes: - all: https://git.openjdk.java.net/jdk/pull/7426/files - new: https://git.openjdk.java.net/jdk/pull/7426/files/6aaa1a3a..7d3e7a1b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7426=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7426=00-01 Stats: 59 lines in 59 files changed: 0 ins; 0 del; 59 mod Patch: https://git.openjdk.java.net/jdk/pull/7426.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7426/head:pull/7426 PR: https://git.openjdk.java.net/jdk/pull/7426
Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc [v2]
On Fri, 11 Feb 2022 08:54:51 GMT, Leo Korinth wrote: >> Remove unused imports under test/lib and jtreg/gc. They create lots of >> warnings if editing using an IDE. Tests in hotspot_gc passed. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > updating copyright I have a maven project that compiles test/lib and jtreg/gc, so everything changed does compile, I should have mentioned that. I have updated copyright year on all files now as well. - PR: https://git.openjdk.java.net/jdk/pull/7426
Re: RFR: 8281631: HashMap.putAll can cause redundant space waste
On Thu, 10 Feb 2022 17:46:36 GMT, XenoAmess wrote: > 8281631: HashMap.putAll can cause redundant space waste According to the discussion at mailing list, we decide to try only change the calculation inside HashMap and WeakHashMap, and see what would happen. The next step is fixing all such **size/0.75+1** in jdk (expected several hundreds places...) I investigated most of the usages. They just give a size, and get a capacity, even not change the 0.75 So maybe we can use some int calculation to replace the 0.75, thus replace Math.ceil for such situations. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
On Fri, 11 Feb 2022 08:05:30 GMT, Peter Levart wrote:
>> liach has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> Make config a pojo, move loading code into config class
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 685:
>
>> 683: instance = c = load();
>> 684: }
>> 685: return c;
>
> If you do that the "old" way, you loose the ability for JIT to constant-fold
> the `instance` and by transitivity the Config instance fields, since the
> check for `VM.isModuleSystemInited()` can't be elided. As suggested, the
> fast-path check should be done 1st, like:
>
>
> private static @Stable Config instance;
>
> private static Config instance() {
> Config c = instance;
> if (c != null) {
> return c;
> }
>
> // Defer initialization until module system is initialized so as
> // to avoid inflation and spinning bytecode in unnamed modules
> // during early startup.
> if (!VM.isModuleSystemInited()) {
> return DEFAULT;
> }
>
> instance = c = load();
> return c;
> }
...having suggested that rearrangement, perhaps the right way to do it is to
enable some VM.isXXX queries themselves to be constant-foldable so that other
callers too may benefit. Like this:
https://github.com/plevart/jdk/commit/e918ccc52bbc288f6721af5fa66d8f7a8cc880cf
WDYT?
-
PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v6]
On Thu, 10 Feb 2022 22:53:56 GMT, liach wrote:
>> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407),
>> by design, duplicate initialization of ReflectionFactory should be safe as
>> it performs side-effect-free property read actions, and the suggesting of
>> making the `initted` field volatile cannot prevent concurrent initialization
>> either; however, having `initted == true` published without the other
>> fields' values is a possibility, which this patch addresses.
>>
>> This simulates what's done in `CallSite`'s constructor for
>> `ConstantCallSite`. Please feel free to point out the problems with this
>> patch, as I am relatively inexperienced in this field of fences and there
>> are relatively less available documents. (Thanks to
>> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/)
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Make config a pojo, move loading code into config class
Changes requested by plevart (Reviewer).
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
685:
> 683: instance = c = load();
> 684: }
> 685: return c;
If you do that the "old" way, you loose the ability for JIT to constant-fold
the `instance` and by transitivity the Config instance fields, since the check
for `VM.isModuleSystemInited()` can't be elided. As suggested, the fast-path
check should be done 1st, like:
private static @Stable Config instance;
private static Config instance() {
Config c = instance;
if (c != null) {
return c;
}
// Defer initialization until module system is initialized so as
// to avoid inflation and spinning bytecode in unnamed modules
// during early startup.
if (!VM.isModuleSystemInited()) {
return DEFAULT;
}
instance = c = load();
return c;
}
-
PR: https://git.openjdk.java.net/jdk/pull/6889
