RFR: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
Can I please get a review for this change which proposes to fix the issue noted in https://bugs.openjdk.java.net/browse/JDK-8281634? The commit introduces the missing `err.invalid.filters` key in the jdeps resource bundle. I have run a simple check to make sure this resource bundle doesn't miss any other `err.` keys. From a simple search, following are the unique `err.` keys used in the code of jdeps (under `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): err.exception.message err.invalid.options err.multirelease.version.associated err.missing.arg err.multirelease.jar.malformed err.option.already.specified err.missing.dependences err.module.not.found err.invalid.path err.genmoduleinfo.not.jarfile err.invalid.arg.for.option err.multirelease.option.notfound err.filter.not.specified err.unknown.option err.command.set err.invalid.filters err.genmoduleinfo.unnamed.package err.option.after.class Apart from the `err.invalid.filters` key which this PR is fixing, none of the other keys are missing from the resource bundle. I haven't updated the resource bundles for Japanese and Chinese languages because I don't know their equivalent values and looking at the commit history of those files, it appears that those changes are done as separate tasks (should a JBS issue be raised for this?) The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been updated to reproduce the issue and verify this fix. An important detail about the update to the test is that while working on the update to this test, I realized that the current implementation in the test isn't fully functional. As a result, this test is currently, incorrectly considered as passing. Specifically, the test was missing a `assertTrue` call against the ouput/error content generated by the run of the `jdeps` tool. This PR adds that assertion. Once that assertion is added, it started showing up 3 genuine failures. These failures are within that test code: - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be a typo when this section in the test was added. The commit history of the source of jdeps tool shows that this option was always `-jdkinternals`. This PR fixes that part in the test. - The test expects an error message "-R cannot be used with --inverse option" when `-R` and `--inverse` are used together. However, at some point the source of jdeps tool was changed (as part of https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error message. That changes appears to have missed changing this test case error message and since this test has been falsely passing, it never got caught. This PR now fixes that issue by expecting the correct error message. - The test was expecting an error message "--list-deps and --list-reduced-deps options are specified" when "--list-deps" was used along with "--summary". This appears to be a copy/paste error in the test case and wasn't caught because the test was falsely passing. This too has been fixed in this PR. With the fixes in this test, the test now reproduces the original issue and verifies the fix. I realize that this PR has changes in the test that aren't directly related to the issue raised in https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are necessary to get the test functional. However if a separate JBS issue needs to be opened to track those changes, please do let me know and I'll address these test changes in a separate PR. - Commit messages: - 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters Changes: https://git.openjdk.java.net/jdk/pull/7455/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7455=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281634 Stats: 14 lines in 2 files changed: 5 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7455.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7455/head:pull/7455 PR: https://git.openjdk.java.net/jdk/pull/7455
Re: [External] : Sequenced Collections
> From: "Tagir Valeev"
> To: "Stuart Marks"
> Cc: "Remi Forax" , "core-libs-dev"
>
> Sent: Saturday, February 12, 2022 4:24:24 AM
> Subject: Re: [External] : Sequenced Collections
> Wow, I missed that the Sequenced Collections JEP draft was posted!
> Of course, I strongly support this initiative and am happy that my proposal
> got
> some love and is moving forward. In general, I like the JEP in the way it is.
> I
> have only two slight concerns:
> 1. I'm not sure that having addition methods (addFirst, addLast, putFirst,
> putLast) is a good idea, as not every mutable implementation may support them.
> While this adds some API unification, it's quite a rare case when this could
> be
> necessary. I think most real world applications of Sequenced* types would be
> around querying, or maybe draining (so removal operations are ok). Probably it
> would be enough to add addFirst, addLast, putFirst, putLast directly to the
> compatible implementations/subinterfaces like List, LinkedHashSet, and
> LinkedHashMap removing them from the Sequenced* interfaces. In this case,
> SortedSet interface will not be polluted with operations that can never be
> implemented. Well my opinion is not very strong here.
> 2. SequencedCollection name is a little bit too long. I think every extra
> letter
> adds a hesitation for users to use the type, especially in APIs where it could
> be the most useful. I see the Naming section and must admit that I don't have
> better ideas. Well, maybe just Sequenced would work? Or too vague?
> Speaking of Remi's suggestion, I don't think it's a good idea. Maybe it could
> be
> if we designed the Collection API from scratch.
??
Here is the first sentence of the javadoc for java.util.List "An ordered
collection (also known as a sequence )."
And the first paragraph of java.util.RandomAccess "Marker interface used by
List implementations to indicate that they support fast (generally constant
time) random access. The primary purpose of this interface is to allow generic
algorithms to alter their behavior to provide good performance when applied to
either random or sequential access lists"
You can find that the actual design, mixing ordered collection and indexed
collection into one interface named List not great, but you can not say that
this is not the actual design.
> But given the current state of Java collections, it's better to add new
> interfaces than to put the new semantics to the java.util.List and greatly
> increase the amount of non-random-accessed lists in the wild.
> There are tons of code that implicitly assume fast random access of every
> incoming list (using indexed iteration inside). The suggested approach could
> become a performance disaster.
If you take several Java developers, some will stick to the javadoc definition,
a List is either sequential or random access and some will think that a List is
only random access. Because of that, adding more sequential implementations
under the List interface is an issue.
Introducing SequencesCollection (more on the name later), a super interface of
List solves that issue, the new implementations will only implement the
sequential part of interface List.
But it does not solve the other problems, mainly adding 4 interfaces when one
is enough, not being backward compatible because of inference and the weird
semantics of LinkedHashMap.
We still need SortedSet or LinkedHashSet to not directly implement
SequencesCollection but to use delegation and a have a method returning a view.
The same reasoning applied to SortedMap, LinkedHashMap.
By using views, there is no need to the two other proposed interfaces
SequenceSet and SequenceMap.
Another question is ListIterator, a list can be iterated forward and backward,
a SequenceCollection can do almost the same thing, with iterator() and
reversed().iterator(). It's not exactly the same semantics but i don't think it
exist an implementation of SequenceCollection that can be implemented without
the property that given one element, it's predecessor and successor can be
found in O(1).
Do we need a new SequenceCollectionIterator that provides the method
next/hasNext/previous/hasPrevious but not add/set/nextIndex/previousIndex ?
For the name, Java uses single simple name of one syllable for the important
interface List, Set, Map or Deque (the javadoc of Deque insist that Deque
should be pronounced using one syllable).
So the name should be Seq.
The main issue with the name "Seq" is that it is perhaps too close to the name
"Set".
Also, it can not be "Sequence" because of CharSequence.
interface Seq extends Collection {
void addFirst();
void addLast();
E getFirst();
E getLast();
E removeFirst();
E removeLast();
Seq reversed();
}
interface List extends Seq { }
interface SortedSet implements Set { // or NavigableSet
// new methods
Seq asSeq();
}
interface SortedMap implements Map { // or NavigableMap
// new methods
Seq keySeq(); //
Re: RFR: 8279508: Auto-vectorize Math.round API [v3]
On Sun, 13 Feb 2022 13:08:41 GMT, Jatin Bhateja wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4066:
>>
>>> 4064: }
>>> 4065:
>>> 4066: void
>>> C2_MacroAssembler::vector_cast_double_special_cases_evex(XMMRegister dst,
>>> XMMRegister src, XMMRegister xtmp1,
>>
>> What does this do? Comment, even pseudo code, would be nice.
>
>> Hi, IIRC for evex encoding you can embed the RC control bit directly in the
>> evex prefix, removing the need to rely on global MXCSR register. Thanks.
>
> Hi @merykitty , You are correct, we can embed RC mode in instruction
> encoding of round instruction (towards -inf,+inf, zero). But to match the
> semantics of Math.round API one needs to add 0.5[f] to input value and then
> perform rounding over resultant value, which is why @sviswa7 suggested to use
> a global rounding mode driven by MXCSR.RC so that intermediate floating
> inexact values also are resolved as desired, but OOO execution may misplace
> LDMXCSR and hence may have undesired side effects.
> What does this do? Comment, even pseudo code, would be nice.
Thanks @theRealAph , I shall append the comments over the routine.
BTW, entire rounding algorithm can also be implemented using Vector API which
can perform if-conversion using masked operations.
class roundf {
public static VectorSpecies ISPECIES = IntVector.SPECIES_512;
public static VectorSpecies SPECIES = FloatVector.SPECIES_512;
public static int round_vector(float[] a, int[] r, int ctr) {
IntVector shiftVBC = (IntVector) ISPECIES.broadcast(24 - 2 + 127);
for (int i = 0; i < a.length; i += SPECIES.length()) {
FloatVector fv = FloatVector.fromArray(SPECIES, a, i);
IntVector iv = fv.reinterpretAsInts();
IntVector biasedExpV = iv.lanewise(VectorOperators.AND, 0x7F80);
biasedExpV = biasedExpV.lanewise(VectorOperators.ASHR, 23);
IntVector shiftV = shiftVBC.lanewise(VectorOperators.SUB, biasedExpV);
VectorMask cond = shiftV.lanewise(VectorOperators.AND, -32)
.compare(VectorOperators.EQ, 0);
IntVector res = iv.lanewise(VectorOperators.AND, 0x007F)
.lanewise(VectorOperators.OR, 0x007F + 1);
VectorMask cond1 = iv.compare(VectorOperators.LT, 0);
VectorMask cond2 = cond1.and(cond);
res = res.lanewise(VectorOperators.NEG, cond2);
res = res.lanewise(VectorOperators.ASHR, shiftV)
.lanewise(VectorOperators.ADD, 1)
.lanewise(VectorOperators.ASHR, 1);
res = fv.convert(VectorOperators.F2I, 0)
.reinterpretAsInts()
.blend(res, cond);
res.intoArray(r, i);
}
return r[ctr];
}
-
PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v3]
On Sun, 13 Feb 2022 10:58:19 GMT, Andrew Haley wrote: >> Jatin Bhateja has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - 8279508: Adding vectorized algorithms to match the semantics of rounding >> operations. >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8279508 >> - 8279508: Adding a test for scalar intrinsification. >> - 8279508: Auto-vectorize Math.round API > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4066: > >> 4064: } >> 4065: >> 4066: void >> C2_MacroAssembler::vector_cast_double_special_cases_evex(XMMRegister dst, >> XMMRegister src, XMMRegister xtmp1, > > What does this do? Comment, even pseudo code, would be nice. > Hi, IIRC for evex encoding you can embed the RC control bit directly in the > evex prefix, removing the need to rely on global MXCSR register. Thanks. Hi @merykitty , You are correct, we can embed RC mode in instruction encoding round instructions (towards -inf,+inf, zero). But to match the semantics of Math.round API one needs to add 0.5[f] to input value and then perform rounding over resultant value, which is why @sviswa7 suggested to use a global rounding mode driven by MXCSR.RC so that intermediate floating inexact values also are resolved as desired, but OOO execution may misplace LDMXCSR and hence may have undesired side effects. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v3]
On Sun, 13 Feb 2022 03:09:43 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 | 584.99 | 1870.70 | 3.20 | >> 510.35 | 548.60 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 257.17 | 965.33 | 3.75 | >> 293.60 | 273.15 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.69 | 3592.54 | 4.35 | >> 825.32 | 1836.42 | 2.23 >> FpRoundingBenchmark.test_round_float | 2048.00 | 388.55 | 1895.77 | 4.88 | >> 412.31 | 945.82 | 2.29 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8279508: Adding vectorized algorithms to match the semantics of rounding > operations. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8279508 > - 8279508: Adding a test for scalar intrinsification. > - 8279508: Auto-vectorize Math.round API src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4066: > 4064: } > 4065: > 4066: void > C2_MacroAssembler::vector_cast_double_special_cases_evex(XMMRegister dst, > XMMRegister src, XMMRegister xtmp1, What does this do? Comment, even pseudo code, would be nice. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v3]
On Sun, 13 Feb 2022 03:09:43 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 | 584.99 | 1870.70 | 3.20 | >> 510.35 | 548.60 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 257.17 | 965.33 | 3.75 | >> 293.60 | 273.15 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.69 | 3592.54 | 4.35 | >> 825.32 | 1836.42 | 2.23 >> FpRoundingBenchmark.test_round_float | 2048.00 | 388.55 | 1895.77 | 4.88 | >> 412.31 | 945.82 | 2.29 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8279508: Adding vectorized algorithms to match the semantics of rounding > operations. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8279508 > - 8279508: Adding a test for scalar intrinsification. > - 8279508: Auto-vectorize Math.round API Also, it seems you have tried using `roundss/sd/ps/pd` followed by a cast to correct the rounding behaviour but decided to take another approach. Some comments around the functions explaining why that is so would be preferable. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7094
