RFR: 8282407: Missing ')' in MacResources.properties
- Fixed by adding missing ']'. - I changed '()' to '[]', since other error messages use '[]' and not '()'. - Commit messages: - 8282407: Missing ')' in MacResources.properties Changes: https://git.openjdk.java.net/jdk/pull/7797/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7797=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282407 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7797.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7797/head:pull/7797 PR: https://git.openjdk.java.net/jdk/pull/7797
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:40:39 GMT, Stuart Marks wrote: > I think we can rely on the monotonicity of these functions. If populating a > map both with 49 and with 96 mappings results in a table length of 128, we > don't need to test that all the intermediate inputs also result in a table > length of 128. Including all the intermediate inputs makes the source code > more bulky and requires future readers/maintainers to hunt around in the long > list of tests to figure out which ones are significant. Really, the only ones > that are significant are the boundary cases, so just keep those. Adding more > tests that aren't relevant actually does hurt, even if they execute quickly. > So: please cut out all the extra test cases that aren't near the boundary > cases. what I worried is, the boundary this is based on the current table size calculating mechanic in HashMap. If people change the mechanic in HashMap, then the boundary would change. But well, this is a white box text for HashMap (and HashMap-like) classes after all, so maybe I'm just over overthinking too much. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:40:39 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - change KeyStructure to String >> - fix test > > Regarding the number of test cases for `tableSizeForCases` and > `populatedCapacityCases`... the issue is not necessarily with execution time > (although if the test is slow it is a problem). The issues are more around: > Is this testing anything useful, and does this make sense to the reader? > > I think we can rely on the monotonicity of these functions. If populating a > map both with 49 and with 96 mappings results in a table length of 128, we > don't need to test that all the intermediate inputs also result in a table > length of 128. Including all the intermediate inputs makes the source code > more bulky and requires future readers/maintainers to hunt around in the long > list of tests to figure out which ones are significant. Really, the only ones > that are significant are the boundary cases, so just keep those. Adding more > tests that aren't relevant actually does hurt, even if they execute quickly. > So: please cut out all the extra test cases that aren't near the boundary > cases. > > I'm not sure what's going on with the build. The builds are in GitHub Actions > and they aren't necessarily reliable, so I wouldn't worry about them too > much. I'll run the final version through our internal build/test system > before integration. (I've also done this previously, and the results were > fine.) @stuart-marks all the changes to the test you requested at last review comments are done. please have a look, thanks! - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
> 8281631: HashMap copy constructor and putAll can over-allocate table XenoAmess has updated the pull request incrementally with one additional commit since the last revision: refine test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/51871859..a980bda4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=33 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=32-33 Stats: 350 lines in 1 file changed: 2 ins; 291 del; 57 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 copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:09:25 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - change KeyStructure to String
>> - fix test
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 61:
>
>> 59: String String = Integer.toString(i);
>> 60: map.put(String, String);
>> 61: }
>
> Small details here... the `String` variable should be lower case. But really
> this method should be inlined into `putN`, since that's the method that needs
> to generate mappings. Then, `makeMap` should call `putN`.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 107:
>
>> 105: for (int i = 0; i < size; ++i) {
>> 106: putMap(map, i);
>> 107: }
>
> Replace this loop with a call to `putN`.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 131:
>
>> 129: void putN(Map map, int n) {
>> 130: for (int i = 0; i < n; i++) {
>> 131: putMap(map, i);
>
> Inline `putMap` here.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 317:
>
>> 315: public void defaultCapacity(Supplier> s) {
>> 316: Map map = s.get();
>> 317: putMap(map, 0);
>
> `map.put("", "")`
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 343:
>
>> 341: public void requestedCapacity(String label, int cap,
>> Supplier> s) {
>> 342: Map map = s.get();
>> 343: putMap(map, 0);
>
> No need to generate a key/value pair here, just use string literals, e.g.
> `map.put("", "")`.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 430:
>
>> 428: map.putAll(makeMap(size));
>> 429: })
>> 430: );
>
> Wait, did this get reindented? Adding line breaks totally destroys the
> tabular nature of test data. Please restore. Yes, the lines end up being
> longer than the usual limit, but the benefit of having things in a nice table
> outweighs to cost of having the lines be somewhat over-limit.
@stuart-marks done.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v16]
On Fri, 11 Mar 2022 01:25:28 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge branch 'randomwrapper' of https://github.com/YShow/jdk.git into > randomwrapper > - Remove atomicLong allocation and override next(int) method to throw UOE Alright all changes were made, tests passed - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v17]
> Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 Yasser Bazzi has updated the pull request incrementally with one additional commit since the last revision: make changes proposed - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/8155593e..53c651f1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=15-16 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7001/head:pull/7001 PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8282929: Localized monetary symbols are not reflected in `toLocalizedPattern` return value [v2]
On Fri, 11 Mar 2022 22:20:38 GMT, Naoto Sato wrote: >> `DecimalFormat.toLocalizedPattern()` was not honoring the monetary >> decimal/grouping separator symbols. Fix is straightforward to use the >> correct symbols depending on the formatter type. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Refined the test Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7790
RFR: JDK-8236128: Allow jpackage create installers for services
Implementation of [JDK-8275062: "Allow jpackage create installers for services"](https://bugs.openjdk.java.net/browse/JDK-8275062) CSR - Commit messages: - Whitespace cleanup - Whitespace cleanup - JDK-8236128: Allow jpackage create installers for services - Sync l10n files. Fix copyright year - Minor formatting fix - Merge fix - Merge fixed - Merge branch 'master' into JDK-8236128 - Merge branch 'JDK-8236128' of https://github.com/alexeysemenyukoracle/jdk into JDK-8236128 - Bugfix - ... and 110 more: https://git.openjdk.java.net/jdk/compare/cd234f5d...0ff8e9aa Changes: https://git.openjdk.java.net/jdk/pull/7793/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7793=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8236128 Stats: 2769 lines in 64 files changed: 2508 ins; 121 del; 140 mod Patch: https://git.openjdk.java.net/jdk/pull/7793.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7793/head:pull/7793 PR: https://git.openjdk.java.net/jdk/pull/7793
RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
Background, from JBS:
src/java.base/share/native/libverify/check_code.c: In function 'read_all_code':
src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may
be used uninitialized [-Werror=maybe-uninitialized]
942 | check_and_push(context, lengths, VM_MALLOC_BLK);
| ^~~
src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument 2
of type 'const void *' to 'check_and_push' declared here
4145 | static void check_and_push(context_type *context, const void *ptr, int
kind)
| ^~
Because the second argument of check_and_push is "const void*" GCC assumes that
the malloc:ed data, which has not yet been initialized, will not be/can not be
modified later which in turn suggests it may be used without ever being
initialized.
The same general issue was addressed in
[JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably for
GCC 11.1.
Details:
Instead of sprinkling more calloc calls around or using pragmas/gcc attributes
I chose to change the check_and_push function to take a (non-const) void*
argument, and provide a new wrapper function `check_and_push_const` which
handles the const argument case. For the (non-const) VM_MALLOC_BKP that means
the pointer never needs to go through a const conversion.
To avoid having multiple ways of solving the same problem I also chose to
revert the change made in JDK-8266168, reverting the calloc back to a malloc
call.
Testing:
tier1 + builds-tier{2,3,4,5}
-
Commit messages:
- 8283059: Uninitialized warning in check_code.c with GCC 11.2
Changes: https://git.openjdk.java.net/jdk/pull/7794/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7794=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8283059
Stats: 22 lines in 1 file changed: 5 ins; 0 del; 17 mod
Patch: https://git.openjdk.java.net/jdk/pull/7794.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7794/head:pull/7794
PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v2]
On Fri, 11 Mar 2022 22:49:25 GMT, Mandy Chung wrote: >> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null >> receiver check rather than NPE thrown by `Object::getClass`. The message of >> NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be >> helpful but not in this case. > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Move the null check after isInstance check > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8282776 > - JDK-8282776: Bad NullPointerException message when invoking an interface > MethodHandle on a null receiver I did a quick sanity run of the reflection microbenchmarks. I eyeball the numbers and don't spot any difference. OTOH, since the `isInstance` check covers the null check, moving the null check inside the exception case is a better fix. - PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v2]
> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Move the null check after isInstance check - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8282776 - JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver - Changes: - all: https://git.openjdk.java.net/jdk/pull/7766/files - new: https://git.openjdk.java.net/jdk/pull/7766/files/8af60582..c7224f2f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7766=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7766=00-01 Stats: 688 lines in 35 files changed: 522 ins; 56 del; 110 mod Patch: https://git.openjdk.java.net/jdk/pull/7766.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7766/head:pull/7766 PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 8282929: Localized monetary symbols are not reflected in `toLocalizedPattern` return value [v2]
> `DecimalFormat.toLocalizedPattern()` was not honoring the monetary > decimal/grouping separator symbols. Fix is straightforward to use the correct > symbols depending on the formatter type. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refined the test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7790/files - new: https://git.openjdk.java.net/jdk/pull/7790/files/a6cbf914..5064e354 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7790=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7790=00-01 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7790.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7790/head:pull/7790 PR: https://git.openjdk.java.net/jdk/pull/7790
Re: RFR: 8279508: Auto-vectorize Math.round API [v2]
On Thu, 3 Mar 2022 05:42:23 GMT, Jatin Bhateja wrote: >> The testing for this PR doesn't look adequate to me. I don't see any testing >> for the values where the behavior of round has been redefined at points in >> the last decade. See JDK-8010430 and JDK-6430675, both of which have >> regression tests in the core libs area. Thanks. > > Hi @jddarcy , can you kindly validate your feedback, it has been incorporated. @jatin-bhateja There is a failure reported in the Pre-submit tests on Windows x64 for compiler/vectorization/TestRoundVect.java. Could you please take a look? - PR: https://git.openjdk.java.net/jdk/pull/7094
RFR: 8282929: Localized monetary symbols are not reflected in `toLocalizedPattern` return value
`DecimalFormat.toLocalizedPattern()` was not honoring the monetary decimal/grouping separator symbols. Fix is straightforward to use the correct symbols depending on the formatter type. - Commit messages: - 8282929: Localized monetary symbols are not reflected in `toLocalizedPattern` return value Changes: https://git.openjdk.java.net/jdk/pull/7790/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7790=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282929 Stats: 70 lines in 2 files changed: 65 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7790.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7790/head:pull/7790 PR: https://git.openjdk.java.net/jdk/pull/7790
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v16]
On Fri, 11 Mar 2022 01:25:28 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Merge branch 'randomwrapper' of https://github.com/YShow/jdk.git into
> randomwrapper
> - Remove atomicLong allocation and override next(int) method to throw UOE
A small spec change is necessary; please make it here, and I'll copy it into
the CSR.
src/java.base/share/classes/java/util/Random.java line 98:
> 96: private RandomWrapper(RandomGenerator randomToWrap) {
> 97: super(null);
> 98: this.generator = Objects.requireNonNull(randomToWrap);
Can remove `requireNonNull` here; see other comments. At your option, add a
comment saying that `randomToWrap` must not be null. This is easy to verify
since there's only one caller elsewhere in this file.
src/java.base/share/classes/java/util/Random.java line 320:
> 318: * @param generator the {@code RandomGenerator} calls are delegated
> to
> 319: * @return the delegating {@code Random} instance
> 320: */
Need to add
@throws NullPointerException if generator is null
src/java.base/share/classes/java/util/Random.java line 327:
> 325: return new RandomWrapper(generator);
> 326: }
> 327:
I'd suggest adding
Objects.requireNonNull(generator)
as the first line of this method. In fact if null is passed, NPE is thrown
already, as the `instanceof` will be false and then the `RandomWrapper`
constructor will throw NPE. However, verifying this requires a bit of hunting
around and some flow analysis to determine this. Might as well make the null
check explicit at the top of this method. The now-redundant check can be
removed from `RandomWrapper`.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 15:56:26 GMT, XenoAmess wrote:
>> 8281631: HashMap copy constructor and putAll can over-allocate table
>
> XenoAmess has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - change KeyStructure to String
> - fix test
Regarding the number of test cases for `tableSizeForCases` and
`populatedCapacityCases`... the issue is not necessarily with execution time
(although if the test is slow it is a problem). The issues are more around: Is
this testing anything useful, and does this make sense to the reader?
I think we can rely on the monotonicity of these functions. If populating a map
both with 49 and with 96 mappings results in a table length of 128, we don't
need to test that all the intermediate inputs also result in a table length of
128. Including all the intermediate inputs makes the source code more bulky and
requires future readers/maintainers to hunt around in the long list of tests to
figure out which ones are significant. Really, the only ones that are
significant are the boundary cases, so just keep those. Adding more tests that
aren't relevant actually does hurt, even if they execute quickly. So: please
cut out all the extra test cases that aren't near the boundary cases.
I'm not sure what's going on with the build. The builds are in GitHub Actions
and they aren't necessarily reliable, so I wouldn't worry about them too much.
I'll run the final version through our internal build/test system before
integration. (I've also done this previously, and the results were fine.)
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 61:
> 59: String String = Integer.toString(i);
> 60: map.put(String, String);
> 61: }
Small details here... the `String` variable should be lower case. But really
this method should be inlined into `putN`, since that's the method that needs
to generate mappings. Then, `makeMap` should call `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 107:
> 105: for (int i = 0; i < size; ++i) {
> 106: putMap(map, i);
> 107: }
Replace this loop with a call to `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 131:
> 129: void putN(Map map, int n) {
> 130: for (int i = 0; i < n; i++) {
> 131: putMap(map, i);
Inline `putMap` here.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 317:
> 315: public void defaultCapacity(Supplier> s) {
> 316: Map map = s.get();
> 317: putMap(map, 0);
`map.put("", "")`
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 343:
> 341: public void requestedCapacity(String label, int cap,
> Supplier> s) {
> 342: Map map = s.get();
> 343: putMap(map, 0);
No need to generate a key/value pair here, just use string literals, e.g.
`map.put("", "")`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 430:
> 428: map.putAll(makeMap(size));
> 429: })
> 430: );
Wait, did this get reindented? Adding line breaks totally destroys the tabular
nature of test data. Please restore. Yes, the lines end up being longer than
the usual limit, but the benefit of having things in a nice table outweighs to
cost of having the lines be somewhat over-limit.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one [v2]
On Fri, 11 Mar 2022 19:07:59 GMT, Daniel Fuchs wrote:
>> Carter Kozak has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add 8283049 to LoggerFinderLoaderTest @bug docs
>
> test/jdk/java/lang/System/LoggerFinder/internal/LoggerFinderLoaderTest/LoggerFinderLoaderTest.java
> line 233:
>
>> 231: }
>> 232: } else if (singleton) {
>> 233: if
>> (!warning.contains("java.util.ServiceConfigurationError: More than one
>> LoggerFinder implementation")) {
>
> please add 8283049 to the `@bug` list above since the test actually verifies
> the fix.
Updated, thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/7780
Re: RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one [v2]
> 8283049: Fix non-singleton LoggerFinder error message: s/on/one Carter Kozak has updated the pull request incrementally with one additional commit since the last revision: Add 8283049 to LoggerFinderLoaderTest @bug docs - Changes: - all: https://git.openjdk.java.net/jdk/pull/7780/files - new: https://git.openjdk.java.net/jdk/pull/7780/files/a78180d5..8406b3e4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7780=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7780=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7780.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7780/head:pull/7780 PR: https://git.openjdk.java.net/jdk/pull/7780
Re: RFR: 8279508: Auto-vectorize Math.round API [v14]
> Summary of changes: > - Intrinsify Math.round(float) and Math.round(double) APIs. > - Extend auto-vectorizer to infer vector operations on encountering scalar IR > nodes for above intrinsics. > - Test creation using new IR testing framework. > > Following are the performance number of a JMH micro included with the patch > > Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) > > > Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain > ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio > -- | -- | -- | -- | -- | -- | -- | -- > FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | > 510.36 | 548.39 | 1.07 > FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | > 293.48 | 274.01 | 0.93 > FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | > 751.83 | 2274.13 | 3.02 > FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | > 388.52 | 1334.18 | 3.43 > > > Kindly review and share your feedback. > > Best Regards, > Jatin Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision: 8279508: Reducing the invocation count and compile thresholds for RoundTests.java. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7094/files - new: https://git.openjdk.java.net/jdk/pull/7094/files/fcb73212..2519a58c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7094=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7094=12-13 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7094.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7094/head:pull/7094 PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Thu, 10 Mar 2022 14:29:36 GMT, Joe Darcy wrote: >> Hi @jddarcy , >> >> Test has been modified on the same lines using generic options which >> manipulate compilation thresholds and agnostic to target platforms. >> >> * @run main/othervm -XX:Tier3CompileThreshold=100 >> -XX:CompileThresholdScaling=0.01 -XX:+TieredCompilation RoundTests >> >> Verified that RoundTests::test* methods gets compiled by c2. >> Test execution time with and without change is almost same ~7.80sec over >> Skylake-server. >> >> Regards > > To be more explicit, the existing RoundTests.java test runs in a fraction of > a second. The updated test runs many times slower, even if now under 10 > second, at least on some platforms. > > Can something closer to the original performance be restored? > > As a tier 1 library test, these tests are run quite frequently. Hi @jddarcy , Earlier none of the test methods in RoundTests.java were compiled on account of low invocation count, a loop with 2000 iterations under the influence controlled compilation threshold now triggers tier4 compilation of test points. I did several runs in Skylake machine with patch and without patch and could see no perceptible difference in runtime due to modification. I have further reduced the invocation count and compile threshold. Thanks - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one
On Thu, 10 Mar 2022 22:28:14 GMT, Carter Kozak wrote:
> 8283049: Fix non-singleton LoggerFinder error message: s/on/one
I changed the bug title to match the PR title.
test/jdk/java/lang/System/LoggerFinder/internal/LoggerFinderLoaderTest/LoggerFinderLoaderTest.java
line 233:
> 231: }
> 232: } else if (singleton) {
> 233: if
> (!warning.contains("java.util.ServiceConfigurationError: More than one
> LoggerFinder implementation")) {
please add 8283049 to the `@bug` list above since the test actually verifies
the fix.
-
PR: https://git.openjdk.java.net/jdk/pull/7780
Re: RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one
On Thu, 10 Mar 2022 22:28:14 GMT, Carter Kozak wrote: > 8283049: Fix non-singleton LoggerFinder error message: s/on/one Unfortunately I lack the permissions to create an issue for this. @dfuch is there any chance you could help me create the issue? - PR: https://git.openjdk.java.net/jdk/pull/7780
RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one
8283049: Fix non-singleton LoggerFinder error message: s/on/one - Commit messages: - 8283049: Fix non-singleton LoggerFinder error message: s/on/one Changes: https://git.openjdk.java.net/jdk/pull/7780/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7780=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283049 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7780.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7780/head:pull/7780 PR: https://git.openjdk.java.net/jdk/pull/7780
Re: Request for an issue for LoggerFinderLoader error-message typo
Hi Carter, I have created https://bugs.openjdk.java.net/browse/JDK-8283049 I will sponsor the changed after it's been reviewed and integrated. best regards, -- daniel On 11/03/2022 15:42, Carter Kozak wrote: When multiple LoggerFinder implementations are found, a ServiceConfigurationError is thrown with message "More than on LoggerFinder implementation" rather than "More than one LoggerFinder implementation" (s/on/one).
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v7]
> We propose to provide a runtime anonymous carrier class object generator;
> java.lang.runtime.Carrier. This generator class is designed to share
> anonymous classes when shapes are similar. For example, if several clients
> require objects containing two integer fields, then Carrier will ensure that
> each client generates carrier objects using the same underlying anonymous
> class.
>
> See JBS for details.
Jim Laskey has updated the pull request incrementally with one additional
commit since the last revision:
Revert to {@return} style comments.
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7744/files
- new: https://git.openjdk.java.net/jdk/pull/7744/files/f6b07254..10d1cc93
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7744=06
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=05-06
Stats: 8 lines in 1 file changed: 0 ins; 4 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/7744.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v6]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Clarify primitive store in array carriers. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/890ad4fa..f6b07254 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=04-05 Stats: 10 lines in 1 file changed: 5 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v5]
On Fri, 11 Mar 2022 15:17:30 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Use long array for primitives > - Use long arrays for primitives That might be more in the realm of Valhalla. This is more for capturing arguments from vararg bootstrap methods. But, I can see your use case. - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 15:56:26 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - change KeyStructure to String > - fix test  seems ci fails for network failure. @stuart-marks I refine the tests using String to replace KeyStructure. Please have a look. thanks! - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v5]
On Fri, 11 Mar 2022 15:44:49 GMT, Maurizio Cimadamore
wrote:
>> Jim Laskey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Use long array for primitives
>> - Use long arrays for primitives
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 51:
>
>> 49: * while avoiding primitive boxing associated with collection objects.
>> Component values
>> 50: * can be primitive or Object. Clients can create new carrier instances
>> by describing a
>> 51: * carrier shape, that is, a MethodType whose parameter types
>> describe the
>
> Suggestion:
>
> * carrier shape, that is, a {@linkplain MethodType method type}
> whose parameter types describe the
Done
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v5]
On Fri, 11 Mar 2022 15:53:08 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/java/lang/runtime/Carrier.java line 380: >> >>> 378: } >>> 379: >>> 380: return Unsafe.ARRAY_LONG_BASE_OFFSET + >> >> Shouldn't you add the offset of the first `int` value in the array to the >> resulting expression? E.g. I'm assuming we're storing `long`s first and >> `int`s after? If so, `int` values will be located at an offset from the >> start of the array. > > Ah ok - I see - the constructor method handle binds the correct offset, > adding the required constant offset (for all the long values). So my comment > it's mostly stylistic. I'll add commentary. - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver
On Wed, 9 Mar 2022 22:52:41 GMT, Mandy Chung wrote: > A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Historically I've found that `Objects.requireNonNull` compile down to be roughly equivalent to an implicit null check as long as a NPE is actually never thrown, see https://bugs.openjdk.java.net/browse/JDK-8042127 With the recent change to force-inline `requireNonNull` the profile pollution part of this issue might no longer be relevant. I'll see if I can carve out time to do some experiments to revisit and perhaps even close out that old bug, but I don't think this is much to worry about for this change since provoking such NPEs should be rare in production settings. Always a risk adding explicit calls, of course: A few added cycles on startup, a few extra bytecode that _could_ interfere with inlining of the `checkReceiver` method.. I suggest a quick sanity run of our reflection (micro)benchmarks - PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v5]
On Fri, 11 Mar 2022 15:17:30 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Use long array for primitives > - Use long arrays for primitives This is very exciting, it sounds like we can implement unboxed variable arguments lists based on it in the future ([JDK-8182862](https://bugs.openjdk.java.net/browse/JDK-8182862)), is that so? - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v5]
On Fri, 11 Mar 2022 15:17:30 GMT, Jim Laskey wrote:
>> We propose to provide a runtime anonymous carrier class object generator;
>> java.lang.runtime.Carrier. This generator class is designed to share
>> anonymous classes when shapes are similar. For example, if several clients
>> require objects containing two integer fields, then Carrier will ensure that
>> each client generates carrier objects using the same underlying anonymous
>> class.
>>
>> See JBS for details.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Use long array for primitives
> - Use long arrays for primitives
Marked as reviewed by mcimadamore (Reviewer).
src/java.base/share/classes/java/lang/runtime/Carrier.java line 51:
> 49: * while avoiding primitive boxing associated with collection objects.
> Component values
> 50: * can be primitive or Object. Clients can create new carrier instances
> by describing a
> 51: * carrier shape, that is, a MethodType whose parameter types
> describe the
Suggestion:
* carrier shape, that is, a {@linkplain MethodType method type} whose
parameter types describe the
src/java.base/share/classes/java/lang/runtime/Carrier.java line 380:
> 378: }
> 379:
> 380: return Unsafe.ARRAY_LONG_BASE_OFFSET +
Shouldn't you add the offset of the first `int` value in the array to the
resulting expression? E.g. I'm assuming we're storing `long`s first and `int`s
after? If so, `int` values will be located at an offset from the start of the
array.
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
> 8281631: HashMap copy constructor and putAll can over-allocate table XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - change KeyStructure to String - fix test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/9b9156c1..51871859 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=32 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=31-32 Stats: 133 lines in 1 file changed: 0 ins; 51 del; 82 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: JDK-8282798 java.lang.runtime.Carrier [v5]
On Fri, 11 Mar 2022 15:48:37 GMT, Maurizio Cimadamore wrote: >> Jim Laskey has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Use long array for primitives >> - Use long arrays for primitives > > src/java.base/share/classes/java/lang/runtime/Carrier.java line 380: > >> 378: } >> 379: >> 380: return Unsafe.ARRAY_LONG_BASE_OFFSET + > > Shouldn't you add the offset of the first `int` value in the array to the > resulting expression? E.g. I'm assuming we're storing `long`s first and > `int`s after? If so, `int` values will be located at an offset from the start > of the array. Ah ok - I see - the constructor method handle binds the correct offset, adding the required constant offset (for all the long values). So my comment it's mostly stylistic. - PR: https://git.openjdk.java.net/jdk/pull/7744
Request for an issue for LoggerFinderLoader error-message typo
Hello, I'd be grateful if someone could help me create an issue for a typo in LoggerFinderLoader. When multiple LoggerFinder implementations are found, a ServiceConfigurationError is thrown with message "More than on LoggerFinder implementation" rather than "More than one LoggerFinder implementation" (s/on/one). I've created a PR with a proposed fix, but I lack the ability to create an issue to track it: https://github.com/openjdk/jdk/pull/7780 Thank you, Carter Kozak
Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10 [v2]
On Thu, 10 Mar 2022 17:55:44 GMT, Alisen Chung wrote: >> msg drop for jdk19, Mar 9, 2022 > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > moved CurrencyNames changes to jdk.localedata The security related changes look fine, except for the year 2021. - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v4]
On Fri, 11 Mar 2022 10:58:23 GMT, Maurizio Cimadamore wrote: >> Jim Laskey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Leave public for the time being > > src/java.base/share/classes/java/lang/runtime/Carrier.java line 48: > >> 46: >> 47: /** >> 48: * This class is used to create anonymous objects that have number and >> types of > > This is a bit ambiguous: anonymous "objects that have number and types of > objects determined at runtime" almost seems like the clients have no control. > I think what you want to say is that the shape is given by the client (e.g. > how many longs, int, objects) - but the underlying implementation can vary. > Maybe something like: > > > A carrier is an anonymous objects that can store component values. > Valid component value types are [...]. Clients can create new carrier > instances by describing a carrier shape, that is, a MethodType whose > parameter types describe the types of the carrier component values. Changed - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v4]
On Thu, 10 Mar 2022 22:53:39 GMT, Jim Laskey wrote: >> src/java.base/share/classes/java/lang/runtime/Carrier.java line 346: >> >>> 344: * Carrier for longs and integers. >>> 345: */ >>> 346: private final int[] integers; >> >> I believe (@rose00 correct me if I'm wrong) that an `int[]` doesn't >> guarantee proper 64-but alignment of the first element in the array in >> 32-bit platforms. We ran into similar issues with the MemorySegment API, I >> believe. I think using a `long[]` if you want explicit 64-bit alignment is >> the safest choice. > > Makes sense. Easy change. Done - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v5]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Use long array for primitives - Use long arrays for primitives - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/0849c65d..890ad4fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=03-04 Stats: 85 lines in 1 file changed: 14 ins; 26 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v4]
On Fri, 11 Mar 2022 10:42:02 GMT, Maurizio Cimadamore wrote: >> Jim Laskey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Leave public for the time being > > src/java.base/share/classes/java/lang/runtime/Carrier.java line 57: > >> 55: * @since 19 >> 56: */ >> 57: /*non-public*/ > > What does this non-public comment mean? We might switch to package private for String Template preview. I forgot that the test can't locate the class if I make it package private, > src/java.base/share/classes/java/lang/runtime/Carrier.java line 146: > >> 144: * @return constructor with arguments recasted and reordered >> 145: */ >> 146: private static MethodHandle reshapeConstructor(CarrierShape >> carrierShape, > > I like how reshape constructors and components is now shared across both > implementations Refactoring is just another tool for the lazy. ;-) - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Fri, 11 Mar 2022 08:28:32 GMT, Ioi Lam wrote: >> Is reproducibility also a topic for users calling -Xdump with custom JNI >> coding? Or maybe having the VM instrumented somehow? Since it seems such an >> easy fix, I would prevent attaching too. At least the user would get a clear >> error message. > >> Is reproducibility also a topic for users calling -Xdump with custom JNI >> coding? Or maybe having the VM instrumented somehow? Since it seems such an >> easy fix, I would prevent attaching too. At least the user would get a clear >> error message. > > It's impossible to execute arbitrary Java code when running "java > -Xshare:dump", so this means there's no way to load a JNI library when > creating a *static* CDS archive. The loading of JVMTI agents is also not > supported. So this is not a case we need to handle. > > During *dynamic* CDS dumps, arbitrary Java code can execute, but we don't > have a requirement for the *dynamic* CDS archive to be deterministic (at > least not for now). Thanks for that clarification. Never mind then :) - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8058924: FileReader(String) documentation is insufficient
On Thu, 10 Mar 2022 02:30:35 GMT, Brian Burkhalter wrote: > Add a statement to the `java.io` package documentation clarifying how a > `String` representing a _pathname string_ is interpreted in the package. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7767
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v4]
On Thu, 10 Mar 2022 19:35:16 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Leave public for the time being src/java.base/share/classes/java/lang/runtime/Carrier.java line 48: > 46: > 47: /** > 48: * This class is used to create anonymous objects that have number and > types of This is a bit ambiguous: anonymous "objects that have number and types of objects determined at runtime" almost seems like the clients have no control. I think what you want to say is that the shape is given by the client (e.g. how many longs, int, objects) - but the underlying implementation can vary. Maybe something like: A carrier is an anonymous objects that can store component values. Valid component value types are [...]. Clients can create new carrier instances by describing a carrier shape, that is, a MethodType whose parameter types describe the types of the carrier component values. src/java.base/share/classes/java/lang/runtime/Carrier.java line 57: > 55: * @since 19 > 56: */ > 57: /*non-public*/ What does this non-public comment mean? src/java.base/share/classes/java/lang/runtime/Carrier.java line 146: > 144: * @return constructor with arguments recasted and reordered > 145: */ > 146: private static MethodHandle reshapeConstructor(CarrierShape > carrierShape, I like how reshape constructors and components is now shared across both implementations - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v10]
On Thu, 10 Mar 2022 18:34:27 GMT, Jim Laskey wrote:
>> Several attempts have been made to improve Formatter's numeric performance
>> by caching the current Locale zero. Such fixes, however, ignore the real
>> issue, which is the slowness of fetching DecimalFormatSymbols. By directly
>> caching DecimalFormatSymbols in the Formatter, this enhancement streamlines
>> the process of accessing Locale DecimalFormatSymbols and specifically
>> getZeroDigit(). The result is a general improvement in the performance of
>> numeric formatting.
>>
>>
>> @Benchmark
>> public void bigDecimalDefaultLocale() {
>> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalLocaleAlternate() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalThaiLocale() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void integerDefaultLocale() {
>> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerLocaleAlternate() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerThaiLocale() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>>
>> Before:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>>
>> After:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
>
> Jim Laskey 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 14 additional commits since
> the last revision:
>
> - Merge branch 'master' into 8282625
> - Correct indentation
> - Add unit test for DecimalFormatSymbols.getLocale()
> - Add comment about DecimalFormatSymbols being mutable objects.
> - Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
> Formatter. No significant performance degradation."
>
>This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
> - Revert "Drop DecimalFormatSymbols.getLocale change"
>
>This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
> - Revert "Correct caching test"
>
>This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
> - Correct caching test
> - Drop DecimalFormatSymbols.getLocale change
> - Cache DecimalFormatSymbols in DecimalFormatSymbols instead of Formatter.
> No significant performance degradation.
> - ... and 4 more:
> https://git.openjdk.java.net/jdk/compare/76644795...84fa1fe7
Marked as reviewed by jpai (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v10]
On Fri, 11 Mar 2022 10:16:33 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/text/DecimalFormatSymbols.java line 192:
>>
>>> 190:
>>> 191: /**
>>> 192: * {@return locale used to create this instance}
>>
>> Hello Jim, the opening and closing `{`, I think aren't needed for a `@return`
>
> This is actually a feature of JavaDoc. Accessor methods that require little
> description (self evident) can use {@return ...} to define the description
> and return in one go.
Didn't know that, thank you for clarifying.
The rest of the changes look good to me.
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v10]
On Fri, 11 Mar 2022 04:28:00 GMT, Jaikiran Pai wrote:
>> Jim Laskey 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 14 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into 8282625
>> - Correct indentation
>> - Add unit test for DecimalFormatSymbols.getLocale()
>> - Add comment about DecimalFormatSymbols being mutable objects.
>> - Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
>> Formatter. No significant performance degradation."
>>
>>This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
>> - Revert "Drop DecimalFormatSymbols.getLocale change"
>>
>>This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
>> - Revert "Correct caching test"
>>
>>This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
>> - Correct caching test
>> - Drop DecimalFormatSymbols.getLocale change
>> - Cache DecimalFormatSymbols in DecimalFormatSymbols instead of Formatter.
>> No significant performance degradation.
>> - ... and 4 more:
>> https://git.openjdk.java.net/jdk/compare/9cda7890...84fa1fe7
>
> src/java.base/share/classes/java/text/DecimalFormatSymbols.java line 192:
>
>> 190:
>> 191: /**
>> 192: * {@return locale used to create this instance}
>
> Hello Jim, the opening and closing `{`, I think aren't needed for a `@return`
This is actually a feature of JavaDoc. Accessor methods that require little
description (self evident) can use {@return ...} to define the description and
return in one go.
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 Please don't auto-close this bot :) - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8282572: EnumSet should be a sealed class
On Tue, 8 Mar 2022 10:50:35 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to implement the > enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? > > The (public) `EnumSet` class has 2 package private (JDK) internal sub-classes > - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't have any > sub-classes of their own. > > In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` > and `RegularEnumSet` are the two permitted sub classes. Both of these > sub-classes are now marked as `final` too. Usage of `non-sealed` modifier for > these 2 sub-classes was an option too. But I decided to start with the more > restrictive option since we don't have any other sub-classes and if and when > we do have their sub-classes, it's possible to change them to `non-sealed`. > > The `EnumSet` class implements the `java.io.Serializable` interface. As part > of this change, manual tests have been run to make sure that changing this > class to `sealed` and marking the sub-classes as `final` don't break any > serialization/deserialization semantics, across Java version and/or user > application versions. > > `tier1` testing across various platforms is currently in progress. LGTM - Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7741
Re: RFR: 8282572: EnumSet should be a sealed class
On Tue, 8 Mar 2022 10:50:35 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to implement the > enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? > > The (public) `EnumSet` class has 2 package private (JDK) internal sub-classes > - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't have any > sub-classes of their own. > > In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` > and `RegularEnumSet` are the two permitted sub classes. Both of these > sub-classes are now marked as `final` too. Usage of `non-sealed` modifier for > these 2 sub-classes was an option too. But I decided to start with the more > restrictive option since we don't have any other sub-classes and if and when > we do have their sub-classes, it's possible to change them to `non-sealed`. > > The `EnumSet` class implements the `java.io.Serializable` interface. As part > of this change, manual tests have been run to make sure that changing this > class to `sealed` and marking the sub-classes as `final` don't break any > serialization/deserialization semantics, across Java version and/or user > application versions. > > `tier1` testing across various platforms is currently in progress. The linked CSR has now been approved. - PR: https://git.openjdk.java.net/jdk/pull/7741
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Fri, 11 Mar 2022 07:13:35 GMT, Thomas Stuefe wrote: > Is reproducibility also a topic for users calling -Xdump with custom JNI > coding? Or maybe having the VM instrumented somehow? Since it seems such an > easy fix, I would prevent attaching too. At least the user would get a clear > error message. It's impossible to execute arbitrary Java code when running "java -Xshare:dump", so this means there's no way to load a JNI library when creating a *static* CDS archive. The loading of JVMTI agents is also not supported. So this is not a case we need to handle. During *dynamic* CDS dumps, arbitrary Java code can execute, but we don't have a requirement for the *dynamic* CDS archive to be deterministic (at least not for now). - PR: https://git.openjdk.java.net/jdk/pull/7748
