Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v23]
On Wed, 2 Mar 2022 18:48:46 GMT, XenoAmess wrote:
>> 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:
>
> revert changes to IdentityHashMap
src/java.base/share/classes/java/util/HashMap.java line 498:
> 496: if (s > 0) {
> 497: if (table == null) { // pre-size
> 498: double dt = Math.ceil(s / loadFactor);
Quick update on this. The division must be done in double, not float, before
being passed to `Math.ceil`. I'd recommend casting the load factor to double:
double dt = Math.ceil(s / (double) loadFactor);
The reason is that the significand of a float has only 24 bits, so the low
order 8 bits of the eventual `int` will end up being zeroes even when they have
some significant values. This starts to occur at larger sizes. For example, if
`s` is 25165825, the result is 33554434.0 with double division but it's only
33554432.0 with float division. This latter value is too small, so if it were
used the HashMap would need to be resized when `s` mappings are added.
Similar changes should be made in `readObject` below and in `WeakHashMap`.
I haven't had a chance to look at the test yet, sorry. You might want think
about how to test for cases like this. Don't go off the deep end though. I
don't think we want to have a test that creates lots of maps with 25 million
entries, and we also don't want to duplicate the computations in the test
itself like the Enum ConstantDirectory test does.
src/java.base/share/classes/java/util/HashMap.java line 1530:
> 1528: // use defaults
> 1529: } else if (mappings > 0) {
> 1530: double dc = Math.ceil(mappings / lf);
As above, cast `lf` to `double`.
src/java.base/share/classes/java/util/WeakHashMap.java line 252:
> 250: */
> 251: public WeakHashMap(Map m) {
> 252: this(Math.max((int) Math.ceil(m.size() / 0.75),
Use `(double) DEFAULT_LOAD_FACTOR` in the denominator.
src/java.base/share/classes/java/util/WeakHashMap.java line 558:
> 556: */
> 557: if (numKeysToBeAdded > threshold) {
> 558: int targetCapacity = (int)Math.ceil(numKeysToBeAdded /
> loadFactor);
Cast to double here too.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8279508: Auto-vectorize Math.round API [v2]
On Wed, 19 Jan 2022 22:09:26 GMT, Joe Darcy wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8279508: Adding a test for scalar intrinsification. > > 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. - PR: https://git.openjdk.java.net/jdk/pull/7094
RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings
If AbstractStringBuilder only grow, the inflated value which is encoded in UTF16 can't be compressed. toString() can skip compression in this case. This can save an ArrayAllocation in StringUTF16::compress. java.io.BufferedRead::readLine() is a case that StringBuilder grows only. - Commit messages: - Add a microbenchmark. - 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings Changes: https://git.openjdk.java.net/jdk/pull/7671/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7671=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282429 Stats: 44 lines in 5 files changed: 37 ins; 3 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7671.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7671/head:pull/7671 PR: https://git.openjdk.java.net/jdk/pull/7671
Integrated: 8282515: More clean up on NativeLibraries just for JNI library use
On Wed, 2 Mar 2022 17:04:01 GMT, Mandy Chung wrote: > This patch further cleans up NativeLibraries just for JNI library use. > RawNativeLibraries implements its own native load and unload methods. In > addition, this also fixes the implementation of `RawNativeLibraries::load` > not to throw UnsatisfiedLinkError if a library cannot be loaded for any > reason but instead returns null. This pull request has now been integrated. Changeset: 02aa7cef Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/02aa7cef0a9b9de1ee9a136c5641b498c0c3e5cc Stats: 276 lines in 6 files changed: 176 ins; 28 del; 72 mod 8282515: More clean up on NativeLibraries just for JNI library use Reviewed-by: mcimadamore - PR: https://git.openjdk.java.net/jdk/pull/7661
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Sat, 26 Feb 2022 01:07:47 GMT, Sandhya Viswanathan wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8279508: Adding descriptive comments. > > src/hotspot/cpu/x86/x86.ad line 7295: > >> 7293: __ vector_round_double_evex($dst$$XMMRegister, $src$$XMMRegister, >> $xtmp1$$XMMRegister, >> 7294: $xtmp2$$XMMRegister, >> $ktmp1$$KRegister, $ktmp2$$KRegister, >> 7295: >> ExternalAddress(vector_double_signflip()), new_mxcsr, $scratch$$Register, >> vlen_enc); > > The vector_double_signflip() here should be replaced by vector_all_bits_set(). > vcvtpd2qq description: > If a converted result cannot be represented in the destination > format, the floating-point invalid exception is raised, and if this exception > is masked, the indefinite integer value > (2w-1, where w represents the number of bits in the destination format) is > returned. The overflow value observed is 2^(w-1) so using vector_double_signflip() is correct, please ignore this comment. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Sat, 26 Feb 2022 04:55:08 GMT, Jatin Bhateja wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8279508: Adding descriptive comments. > > As per SDM, if post conversion a floating point number is non-representable > in destination format e.g. a floating point value 3.4028235E10 post integer > conversion will overflow the value range of integer primitive type, hence a > -0.0 value or 0x8000 is returned here. Similarly for +/- NaN and +/-Inf > post conversion value returns is -0.0. All these cases i.e. post conversion > non-representable floating point values and NaN/Inf values are handled in a > special manner where algorithm first performs an unordered comparison b/w > original source value and returns a 0 in case of NaN, this weeds out the NaN > case and for rest of the special values we check the MSB bit of the source > and either return an Integer.MAX_VALUE for +ve numbers or a Integer.MIN_VALUE > to adhere to the semantics of Math.round API. > > Existing tests were enhanced to cover various special cases (NaN/Inf/+ve/-ve > value/values which may be inexact after adding 0.5/ values which post > conversion overflow integer value range). @jatin-bhateja The patch looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v11]
On Wed, 2 Mar 2022 02:44:41 GMT, Jatin Bhateja wrote: >> 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: Removing +LogCompilation flag. Marked as reviewed by sviswanathan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
On Sat, 26 Feb 2022 03:38:32 GMT, Quan Anh Mai wrote: >> I believe the indefinite value should be 2^(w - 1) (a.k.a 0x8000) and >> the documentation is typoed. If you look at `cvtss2si`, the indefinite value >> is also written as 2^w - 1 but yet in `MacroAssembler::convert_f2i` we >> compare it with 0x8000. In addition, choosing -1 as an indefinite value >> is weird enough and to complicate it as 2^w - 1 is really unusual. > > `MacroAssembler::convert_f2i` > > https://github.com/openjdk/jdk/blob/c5c6058fd57d4b594012035eaf18a57257f4ad85/src/hotspot/cpu/x86/macroAssembler_x86.cpp#L8919 @jatin-bhateja @merykitty You are right, on overflow we observe 2^(w - 1) i.e. 0x8000 so using vector_float_signflip() is correct. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]
On Wed, 2 Mar 2022 21:53:01 GMT, Tim Prinzing wrote:
>> The caller class returned by Reflection::getCallerClass was used to gain
>> access to it's module in most cases and class loader in one case. I added a
>> method to translate the caller class to caller module so that the decision
>> of what module represents the caller with no stack frame is made in a single
>> place. Calls made to caller.getModule() were replaced with
>> getCallerModule(caller) which returns the system class loader unnamed module
>> if the caller is null.
>>
>> The one place a class loader was produced from the caller in getBundleImpl
>> it was rewritten to route through the getCallerModule method:
>>
>> final ClassLoader loader = (caller != null) ?
>> caller.getClassLoader() : getLoader(getCallerModule(caller));
>>
>> A JNI test was added which calls getBundle to fetch a test bundle from a
>> location added to the classpath, fetches a string out of the bundle and
>> verifies it, and calls clearCache.
>>
>> The javadoc was updated for the caller sensitive methods changed.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> suggested changes
Thanks for the update.
src/java.base/share/classes/java/util/ResourceBundle.java line 1567:
> 1565: * module will be used.
> 1566: * @param caller
> 1567: * @return
Maybe you intended to make this `/* */` instead of javadoc? no need to
have `@param` and `@return` for this simple method.
src/java.base/share/classes/java/util/ResourceBundle.java line 2251:
> 2249: @CallerSensitive
> 2250: public static final void clearCache() {
> 2251: final Module callerModule =
> getCallerModule(Reflection.getCallerClass());
nit: final can be dropped here.
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
line 52:
> 50: import java.nio.file.Paths;
> 51: import java.util.Properties;
> 52: import java.util.ResourceBundle;
nit: good to keep the imports in alphabetic order.
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
line 74:
> 72:
> 73: // set up shared library path
> 74: final String sharedLibraryPathEnvName =
> Platform.sharedLibraryPathVariableName();
Nit: `final` adds noise but no other value to the test. Can consider dropping
it.
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
line 89:
> 87:
> 88:
> 89: private static void makePropertiesFile() throws IOException {
This can be removed?
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/exeNullCallerResourceBundle.c
line 28:
> 26:
> 27: #include "jni.h"
> 28: #undef NDEBUG
is this line unintended?
-
PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote: > Refactoring of Method and Constructor to share a single empty array for > parameters and exceptions as well as type variables. > > Existing core reflection regression tests pass with the change. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v6]
On Wed, 2 Mar 2022 14:06:10 GMT, Claes Redestad wrote: >> Claes Redestad has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 29 commits: >> >> - Resolve merge conflict >> - Fix TestCountPositives to correctly allow 0 return when expected != len >> (for now) >> - aarch64: fix issue with short inputs divisible by wordSize >> - Switch aarch64 intrinsic to a variant of countPositives returning len or >> zero as a first step. >> - Revert micro changes, split out to #7516 >> - Merge branch 'master' of https://github.com/cl4es/jdk into count_positives >> - 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 >> - ... and 19 more: >> https://git.openjdk.java.net/jdk/compare/5035bf5e...685795ce > >> > > Making the `bottom_type()` of `CountPositivesNode` more precise > (`TypeInt::INT` -> `TypeInt::POS`) might help, then. Seems like something we > want to do regardless. @cl4es Looks like you forgot to remove the "@IntrinsicCandidate" annotation for has_negatives. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA
On Wed, 2 Mar 2022 04:02:45 GMT, Anthony Scarpino wrote: > I have some compatibility concerns about the AES change breaking code that > expects a SecretKeySpec of 16 bytes. I can see situations where > '.getEncoded()' returns a byte[32] when user code expects a byte[16]. Also, > I'm pretty sure passing a 32 byte SecretKeySpec into an AES_128_GCM op will > throw an exception. I haven't looked at other modes. Well, I am not sure how specifying a 32-byte SecretKeySpec (instance of a SecretKey) with AES Cipher would trigger AES_128_GCM op? Shouldn't the keysize be detected when Cipher.init() is called and then invoking the right op? True that this default key size change would impact callers who do not specify key size but then made the assumption of key being 16-byte. Well, the key generator javadoc did warn about not setting a default, i.e. different providers may have different default key size and that the default key size may change later for the same provider. So, it's not like we have not warned about it... - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]
> The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: suggested changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7663/files - new: https://git.openjdk.java.net/jdk/pull/7663/files/9e8fb193..eeb2d0fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7663=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7663=01-02 Stats: 48 lines in 1 file changed: 6 ins; 41 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v2]
> The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/util/ResourceBundle.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.java.net/jdk/pull/7663/files - new: https://git.openjdk.java.net/jdk/pull/7663/files/30778063..9e8fb193 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7663=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7663=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
On Wed, 2 Mar 2022 20:28:45 GMT, Joe Darcy wrote: > Refactoring of Method and Constructor to share a single empty array for > parameters and exceptions as well as type variables. > > Existing core reflection regression tests pass with the change. PS I'll update the copyrights before pushing. - PR: https://git.openjdk.java.net/jdk/pull/7667
RFR: JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
Refactoring of Method and Constructor to share a single empty array for parameters and exceptions as well as type variables. Existing core reflection regression tests pass with the change. - Commit messages: - JDK-8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method Changes: https://git.openjdk.java.net/jdk/pull/7667/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7667=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8177107 Stats: 15 lines in 3 files changed: 9 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7667.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7667/head:pull/7667 PR: https://git.openjdk.java.net/jdk/pull/7667
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
On Wed, 2 Mar 2022 18:56:40 GMT, Tim Prinzing wrote:
> The caller class returned by Reflection::getCallerClass was used to gain
> access to it's module in most cases and class loader in one case. I added a
> method to translate the caller class to caller module so that the decision of
> what module represents the caller with no stack frame is made in a single
> place. Calls made to caller.getModule() were replaced with
> getCallerModule(caller) which returns the system class loader unnamed module
> if the caller is null.
>
> The one place a class loader was produced from the caller in getBundleImpl it
> was rewritten to route through the getCallerModule method:
>
> final ClassLoader loader = (caller != null) ?
> caller.getClassLoader() : getLoader(getCallerModule(caller));
>
> A JNI test was added which calls getBundle to fetch a test bundle from a
> location added to the classpath, fetches a string out of the bundle and
> verifies it, and calls clearCache.
>
> The javadoc was updated for the caller sensitive methods changed.
Instead of sprinkling the null caller text in the javadoc in each `getBundle`
method, we can specify that that in the class specification, for example, after
the "Resource bundles in other modules and class path" section.
In cases where the {@code getBundle} factory method is called from a context
where there is no caller frame on the stack (e.g. when called directly from
a JNI attached thread), the caller module is default to the unnamed module for
the
{@linkplain ClassLoader#getSystemClassLoader system class loader}.
What do you think?
src/java.base/share/classes/java/util/ResourceBundle.java line 1588:
> 1586: Control control) {
> 1587: final ClassLoader loader = (caller != null) ?
> 1588: caller.getClassLoader() :
> getLoader(getCallerModule(caller));
Suggestion:
ClassLoader loader = getLoader(getCallerModule(caller));
This can be simplified as above. I would drop `final` too as it only adds
noise to the code.
-
PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
On Wed, 2 Mar 2022 18:56:40 GMT, Tim Prinzing wrote: > The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Looks good to me. I believe a CSR is needed for this change. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: 8282432: Optimize masked "test" Vector API with predicate feature
On Wed, 2 Mar 2022 02:50:27 GMT, Xiaohong Gong wrote:
> The vector `"test"` api is implemented with vector `"compare"`. And the
> masked `"test" `is implemented with `"test(op).and(m)"` which means
> `"compare().and(m)"` finally. Since the masked vector `"compare"` has been
> optimized with predicated instruction for archituctures that support the
> feature, the masked `"test"` can also be optimized by implementing it with
> the predicated compare. This could save the additional ` "and"` for
> architectures that support the predicate feature.
>
> This patch optimized the masked `"test"` by implementing it with masked
> `"compare"`. With this patch, the following codes for the` "IS_NEGATIVE"` op
> for a DoubleVector with SVE:
>
> mov z19.d, #0
> cmpgt p1.d, p7/z, z19.d, z17.d
> and p0.b, p7/z, p1.b, p0.b
>
> are optimized to:
>
> mov z19.d, #0
> cmpgt p0.d, p0/z, z19.d, z17.d
>
> Also update the jtreg tests for masked` "test" ` to make sure they are hot
> enough to be compiled by c2.
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/DoubleVector.java
line 1737:
> 1735: ,
> 1736: M2 extends VectorMask>
> 1737: M1 testTemplate(Class maskType, Test op, M2 mask) {
Can we simplify by making some code FP specific?
- the mask `cast` can be applied in this method rather than in the caller,
simplifying the signature
- for clarify, vector `viewAsIntegralLanes` is only needed for FP (update would
be required for the non-mask template).
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/DoubleVector.java
line 1766:
> 1764: throw new AssertionError(op);
> 1765: }
> 1766: return maskType.cast(m.cast(this.vspecies()));
Suggestion:
return maskType.cast(m.cast(vsp));
Same for non-mask template too.
test/jdk/jdk/incubator/vector/templates/Unit-Test.template line 29:
> 27: VectorMask<$Wideboxtype$> vmask = VectorMask.fromArray(SPECIES,
> mask, 0);
> 28:
> 29: for (int ic = 0; ic < INVOC_COUNT; ic++) {
Can you remove `SmokeTest` from the method name.
-
PR: https://git.openjdk.java.net/jdk/pull/7654
RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
The caller class returned by Reflection::getCallerClass was used to gain access to it's module in most cases and class loader in one case. I added a method to translate the caller class to caller module so that the decision of what module represents the caller with no stack frame is made in a single place. Calls made to caller.getModule() were replaced with getCallerModule(caller) which returns the system class loader unnamed module if the caller is null. The one place a class loader was produced from the caller in getBundleImpl it was rewritten to route through the getCallerModule method: final ClassLoader loader = (caller != null) ? caller.getClassLoader() : getLoader(getCallerModule(caller)); A JNI test was added which calls getBundle to fetch a test bundle from a location added to the classpath, fetches a string out of the bundle and verifies it, and calls clearCache. The javadoc was updated for the caller sensitive methods changed. - Commit messages: - Use the unnamed module defined to the system class loader instead of the - JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame Changes: https://git.openjdk.java.net/jdk/pull/7663/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7663=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280902 Stats: 265 lines in 4 files changed: 254 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v19]
On Fri, 18 Feb 2022 18:32:31 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - migrate to junit >> - change threshold > > Sigh. (I'm sighing at the author of the > Enum/ConstantDirectoryOptimalCapacity.java test, not you.) What a mess. See > https://bugs.openjdk.java.net/browse/JDK-8282120 which I just filed. The > broken test and the OptimalCapacity utilities are mostly useless, so the > change to that test and Class.java should be removed this PR, and the test > added to the Problem List so it doesn't get run anymore. See > test/jdk/ProblemList.txt and add an entry for the now-failing test, with a > reference to 8282120. > > I don't think I'll have time to look at the junit rewrite and to run this > through our internal build/test system today, so sorry, this may need to wait > a week. @stuart-marks changes to IdentityHashMap reverted. please review again. thanks. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v23]
> 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: revert changes to IdentityHashMap - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/e724cc04..475e040f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=22 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=21-22 Stats: 16 lines in 2 files changed: 0 ins; 13 del; 3 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 [v21]
On Wed, 2 Mar 2022 01:44:24 GMT, Stuart Marks wrote: > I'm starting to look at this again. First, a quick note -- I don't think > there should be any IdentityHashMap changes here. That uses a completely > different internal structure and allocation policy, and it's kind of a > distraction from the main point of this change. (As the preceding chain of > comments illustrates.) > > There is a question of whether IdentityHashMap ought to "allow for some > growth" when copying a map, but it's hard to see how much of a problem this > really is. I'd suggest focusing on getting this change done, and then (if you > still have energy) to try to eradicate the `(int) (expected / 0.75) + 1` > idiom that's sprinkled around the JDK. OK, will split this part from this pr. - PR: https://git.openjdk.java.net/jdk/pull/7431
Integrated: 8209784: Include hsdis in the JDK
On Tue, 22 Feb 2022 16:10:22 GMT, Magnus Ihse Bursie wrote: > This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting > this, together with a valid backend using `--with-hsdis`, will bundle the > generated hsdis library with the JDK. > > The PR also includes a refactoring of the hsdis handling in autoconf, > breaking it out to a separate file `lib-hsdis.gmk`, and breaking the > functionality up in separate functions per backend. This pull request has now been integrated. Changeset: b6c35ae4 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/b6c35ae44aeb31deb7a15ee2939156ed00b3df52 Stats: 676 lines in 8 files changed: 386 ins; 276 del; 14 mod 8209784: Include hsdis in the JDK Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/7578
Re: RFR: 8209784: Include hsdis in the JDK [v3]
On Wed, 2 Mar 2022 14:30:43 GMT, Magnus Ihse Bursie wrote: >> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting >> this, together with a valid backend using `--with-hsdis`, will bundle the >> generated hsdis library with the JDK. >> >> The PR also includes a refactoring of the hsdis handling in autoconf, >> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the >> functionality up in separate functions per backend. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > install-hsdis needs to depend on jdk-image Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7578
Re: RFR: 8282515: More clean up on NativeLibraries just for JNI library use
On Wed, 2 Mar 2022 17:04:01 GMT, Mandy Chung wrote: > This patch further cleans up NativeLibraries just for JNI library use. > RawNativeLibraries implements its own native load and unload methods. In > addition, this also fixes the implementation of `RawNativeLibraries::load` > not to throw UnsatisfiedLinkError if a library cannot be loaded for any > reason but instead returns null. Thanks for the review. I also think this is better and cleaner separation. The duplicated native code is small and they just call `JVM_LoadLibrary` and `JVM_UnloadLibrary` that makes it very clear what this native method does. - PR: https://git.openjdk.java.net/jdk/pull/7661
Re: RFR: 8282515: More clean up on NativeLibraries just for JNI library use [v2]
> This patch further cleans up NativeLibraries just for JNI library use. > RawNativeLibraries implements its own native load and unload methods. In > addition, this also fixes the implementation of `RawNativeLibraries::load` > not to throw UnsatisfiedLinkError if a library cannot be loaded for any > reason but instead returns null. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: fix no newline at end of file - Changes: - all: https://git.openjdk.java.net/jdk/pull/7661/files - new: https://git.openjdk.java.net/jdk/pull/7661/files/eae60a40..cbdc39ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7661=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7661=00-01 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7661.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7661/head:pull/7661 PR: https://git.openjdk.java.net/jdk/pull/7661
Re: RFR: 8282515: More clean up on NativeLibraries just for JNI library use
On Wed, 2 Mar 2022 17:04:01 GMT, Mandy Chung wrote: > This patch further cleans up NativeLibraries just for JNI library use. > RawNativeLibraries implements its own native load and unload methods. In > addition, this also fixes the implementation of `RawNativeLibraries::load` > not to throw UnsatisfiedLinkError if a library cannot be loaded for any > reason but instead returns null. Looks good - thanks for cleaning up further. I think separating the library implementation makes a lot of sense. There's some duplication at the native level, but that's acceptable IMHO - and probably better than having a shared piece of logic that can work in two very different modes. src/java.base/share/native/libjava/RawNativeLibraries.c line 98: > 96: JVM_UnloadLibrary(handle); > 97: JNU_ReleaseStringPlatformChars(env, name, cname); > 98: } Watch out for the newline - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7661
RFR: 8282515: More clean up on NativeLibraries just for JNI library use
This patch further cleans up NativeLibraries just for JNI library use. RawNativeLibraries implements its own native load and unload methods. In addition, this also fixes the implementation of `RawNativeLibraries::load` not to throw UnsatisfiedLinkError if a library cannot be loaded for any reason but instead returns null. - Commit messages: - 8282515: More clean up on NativeLibraries just for JNI library use Changes: https://git.openjdk.java.net/jdk/pull/7661/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7661=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282515 Stats: 275 lines in 6 files changed: 175 ins; 28 del; 72 mod Patch: https://git.openjdk.java.net/jdk/pull/7661.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7661/head:pull/7661 PR: https://git.openjdk.java.net/jdk/pull/7661
Integrated: 8282551: Properly initialize L32X64MixRandom state
On Tue, 18 Jan 2022 01:39:52 GMT, Devin Smith wrote: > Did a cursory review of the other generators, they seem to set the state > correctly. This pull request has now been integrated. Changeset: ce18ff85 Author:Devin Smith Committer: Jim Laskey URL: https://git.openjdk.java.net/jdk/commit/ce18ff8527cc51805bf11c50c050ea64c3f9dec0 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8282551: Properly initialize L32X64MixRandom state Reviewed-by: jlaskey - PR: https://git.openjdk.java.net/jdk/pull/7120
Re: RFR: 8282551: Properly initialize L32X64MixRandom state
On Wed, 2 Mar 2022 15:26:35 GMT, Devin Smith wrote: > Thanks for the help. I signed the OCA on Jan 17, 2022 and got confirmation it > was approved on Jan 20, 2022. Correct. Reviewers may know that this has been recorded if the PR has no `oca` label (you can see above that the `oca` label was removed on Jan 20). - PR: https://git.openjdk.java.net/jdk/pull/7120
Re: Should System.exit be controlled by a Scope Local?
On 01/03/2022 18:17, Sean Mullan wrote: On 3/1/22 8:01 AM, Andrew Haley wrote: On 3/1/22 11:45, Andrew Haley wrote: Sure, you wouldn't be able to use the default thread pool, but that's no big deal, I would have thought. I'm sorry, I'll say that again. :-) I meant to say "you wouldn't be able to use the default thread pool if you wanted to use threads with some restrictions (e.g those that couldn't invoke System.exit()). Can you explain in a little more detail as to what the compatibility issues are with preventing threads in thread pools from calling System.exit? AFAIU, I believe the issue is that threads in the default pool are created early, so they don't see the scope local which overrides the semantics of System.exit. Only threads created _after_ the scope local is set would see the overridden/restricted semantics. Maurizio Thanks, Sean
Re: RFR: 8282551: Properly initialize L32X64MixRandom state
On Tue, 18 Jan 2022 01:39:52 GMT, Devin Smith wrote: > Did a cursory review of the other generators, they seem to set the state > correctly. Thanks for the help. I signed the OCA on Jan 17, 2022 and got confirmation it was approved on Jan 20, 2022. - PR: https://git.openjdk.java.net/jdk/pull/7120
Re: RFR: 8282551: Properly initialize L32X64MixRandom state
On Tue, 18 Jan 2022 01:39:52 GMT, Devin Smith wrote: > Did a cursory review of the other generators, they seem to set the state > correctly. @JimLaskey would it be possible to create an issue and review this change? I don't have an account on the OpenJDK issue tracker. I'm not sure how to move this forward, or get the attention of the people responsible for this code. I see @turbanoff you liked and may be involved w/ OpenJDK? - PR: https://git.openjdk.java.net/jdk/pull/7120
Re: RFR: 8282551: Properly initialize L32X64MixRandom state
On Tue, 18 Jan 2022 01:39:52 GMT, Devin Smith wrote: > Did a cursory review of the other generators, they seem to set the state > correctly. Created https://bugs.openjdk.java.net/browse/JDK-8282551 for this. @devinrsmith please change the issue title to `8282551: Properly initialize L32X64MixRandom state`, then it should be picked up by bots. - PR: https://git.openjdk.java.net/jdk/pull/7120
Re: RFR: 8282551: Properly initialize L32X64MixRandom state
On Mon, 21 Feb 2022 19:43:52 GMT, Devin Smith wrote: >> Did a cursory review of the other generators, they seem to set the state >> correctly. > > I'm not sure how to move this forward, or get the attention of the people > responsible for this code. I see @turbanoff you liked and may be involved w/ > OpenJDK? Hi @devinrsmith, pull requests in openjdk organization are not actively monitored. You should send a note to [core-libs-dev](https://mail.openjdk.java.net/mailman/listinfo/core-libs-dev) (subscription required), and someone will guide you from there. - PR: https://git.openjdk.java.net/jdk/pull/7120
RFR: 8282551: Properly initialize L32X64MixRandom state
Did a cursory review of the other generators, they seem to set the state correctly. - Commit messages: - [PATCH] Properly initialize L32X64MixRandom state Changes: https://git.openjdk.java.net/jdk/pull/7120/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7120=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282551 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7120.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7120/head:pull/7120 PR: https://git.openjdk.java.net/jdk/pull/7120
Re: RFR: 8282551: Properly initialize L32X64MixRandom state
On Tue, 18 Jan 2022 01:39:52 GMT, Devin Smith wrote: > Did a cursory review of the other generators, they seem to set the state > correctly. Marked as reviewed by jlaskey (Reviewer). I'll sponsor this change when you receive your OCA. - PR: https://git.openjdk.java.net/jdk/pull/7120
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v6]
On Tue, 1 Mar 2022 19:12:17 GMT, Claes Redestad wrote: > > @theRealAph , @a74nh or someone familiar with aarch64 code, please review > > aarch64 changes. > > Note that the aarch64 changes I've put in for now implements `countPositives` > to return `0` if there's a negative value anywhere, otherwise `len`. This way > we can remove the intrinsic scaffolding for `hasNegatives` once I integrate > s390 and ppc Sure. And we don't have to check which of the 8 bytes in a word has its top bit set, so we don't have to do any more work, just return the count when we stopped searching. So there's no possible performance regression on AArch64 as far as I can see. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v6]
On Wed, 23 Feb 2022 14:19:20 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 pull request now contains 29 commits: > > - Resolve merge conflict > - Fix TestCountPositives to correctly allow 0 return when expected != len > (for now) > - aarch64: fix issue with short inputs divisible by wordSize > - Switch aarch64 intrinsic to a variant of countPositives returning len or > zero as a first step. > - Revert micro changes, split out to #7516 > - Merge branch 'master' of https://github.com/cl4es/jdk into count_positives > - 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 > - ... and 19 more: > https://git.openjdk.java.net/jdk/compare/5035bf5e...685795ce > Making the `bottom_type()` of `CountPositivesNode` more precise (`TypeInt::INT` -> `TypeInt::POS`) might help, then. Seems like something we want to do regardless. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v7]
> 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 pull request now contains 31 commits: - Narrow the bottom_type of CountPositivesNode (always results in a positive int value) - Merge master - Resolve merge conflict - Fix TestCountPositives to correctly allow 0 return when expected != len (for now) - aarch64: fix issue with short inputs divisible by wordSize - Switch aarch64 intrinsic to a variant of countPositives returning len or zero as a first step. - Revert micro changes, split out to #7516 - Merge branch 'master' of https://github.com/cl4es/jdk into count_positives - Merge branch 'master' into count_positives - Restore partial vector checks in AVX2 and SSE intrinsic variants - ... and 21 more: https://git.openjdk.java.net/jdk/compare/d4d12ad1...7789349a - Changes: https://git.openjdk.java.net/jdk/pull/7231/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7231=06 Stats: 527 lines in 29 files changed: 309 ins; 49 del; 169 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: 8209784: Include hsdis in the JDK [v2]
On Wed, 2 Mar 2022 13:12:52 GMT, Magnus Ihse Bursie wrote: >> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting >> this, together with a valid backend using `--with-hsdis`, will bundle the >> generated hsdis library with the JDK. >> >> The PR also includes a refactoring of the hsdis handling in autoconf, >> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the >> functionality up in separate functions per backend. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Somehow the tabs got converted to spaces Top level install-hsdis needs to depend on jdk-image. - Changes requested by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7578
Re: RFR: 8209784: Include hsdis in the JDK [v2]
> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting > this, together with a valid backend using `--with-hsdis`, will bundle the > generated hsdis library with the JDK. > > The PR also includes a refactoring of the hsdis handling in autoconf, > breaking it out to a separate file `lib-hsdis.gmk`, and breaking the > functionality up in separate functions per backend. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Somehow the tabs got converted to spaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/7578/files - new: https://git.openjdk.java.net/jdk/pull/7578/files/0f19696e..6839f911 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7578=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7578=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7578/head:pull/7578 PR: https://git.openjdk.java.net/jdk/pull/7578
