Re: RFR: 8287121: Fix typo in jlink's description resource key lookup
On Sun, 22 May 2022 05:58:25 GMT, Christian Stein wrote:
> Commit
> https://github.com/openjdk/jdk/commit/655500a4f5e3abcff176599604deceefb6ca6640
> for issue [JDK-8286654](https://bugs.openjdk.java.net/browse/JDK-8286654)
> added an optional description accessor on the `ToolProvider` interface. It
> included a typo in` jlink`'s description resource key lookup:
> `"jlink.desciption"`
>
> This follow-up commit fixes the typo by adding the missing `r` character:
> `"jlink.description"`.
>
> This commit also also adds an automated check that ensures all current and
> future tool provider implementations within the JDK don't throw an exception
> when invoking their name and description accessor methods. Specific tool
> names and descriptions are not expected by this general test.
test/jdk/java/util/spi/ToolProviderDescriptionTest.java line 46:
> 44:
> 45: static List listToolDescriptions() {
> 46: return
> StreamSupport.stream(ServiceLoader.load(ToolProvider.class).spliterator(),
> false)
it doesn't matter for this test but SL does have a stream method that could be
used instead:
ServiceLoader.load(ToolProvider.class)
.stream()
.map(ServiceLoader.Provider::get)
.sorted(Comparator.comparing(ToolProvider::name))
...
-
PR: https://git.openjdk.java.net/jdk/pull/8825
RFR: 8287121: Fix typo in jlink's description resource key lookup
Commit https://github.com/openjdk/jdk/commit/655500a4f5e3abcff176599604deceefb6ca6640 for issue [JDK-8286654](https://bugs.openjdk.java.net/browse/JDK-8286654) added an optional description accessor on the `ToolProvider` interface. It included a typo in` jlink`'s description resource key lookup: `"jlink.desciption"` This follow-up commit fixes the typo by adding the missing `r` character: `"jlink.description"`. This commit also also adds an automated check that ensures all current and future tool provider implementations within the JDK don't throw an exception when invoking their name and description accessor methods. Specific tool names and descriptions are not expected by this general test. - Commit messages: - 8287121: Fix typo in jlink's description resource key lookup Changes: https://git.openjdk.java.net/jdk/pull/8825/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8825&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287121 Stats: 58 lines in 2 files changed: 57 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8825.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8825/head:pull/8825 PR: https://git.openjdk.java.net/jdk/pull/8825
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Tue, 17 May 2022 12:43:02 GMT, Yasumasa Suenaga wrote: >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> line 103: >> >>> 101: PRAGMA_STRINGOP_OVERFLOW_IGNORED >>> 102: *dest = op(bits, *dest); >>> 103: PRAGMA_DIAG_POP >> >> I see no stringop here. I'm still trying to understand what the compiler is >> complaining about. > > I guess GCC cannot understand `assert(dest != NULL` immediately preceding it. > > > In file included from > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.inline.hpp:33, > from > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:30, > from > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:30: > In function 'void set_form(jbyte, jbyte*) [with jbyte (* op)(jbyte, jbyte) = > traceid_or]', > inlined from 'void set(jbyte, jbyte*)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:129:23, > inlined from 'static void JfrTraceIdBits::store(jbyte, const T*) [with T > = Klass]' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:135:6, > inlined from 'static void JfrTraceId::tag_as_jdk_jfr_event(const Klass*)' > at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:106:3, > inlined from 'static void JdkJfrEvent::tag_as(const Klass*)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:176:35: I don't think this warning has anything to do with that NULL check. But I'm still not understanding what it is warning about. The "region of size 0" part of the warning message seems important, but I'm not (yet?) seeing how it could be coming up with that. The code involved here is pretty contorted... - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Tue, 17 May 2022 12:38:55 GMT, Yasumasa Suenaga wrote:
>> src/hotspot/share/classfile/classFileParser.cpp line 5970:
>>
>>> 5968: PRAGMA_STRINGOP_OVERFLOW_IGNORED
>>> 5969: _cp->symbol_at_put(hidden_index, _class_name);
>>> 5970: PRAGMA_DIAG_POP
>>
>> I don't understand these warning suppressions for symbol_at_put (here and
>> elsewhere). I don't see any stringops here. What is the compiler
>> complaining about? (There's no mention of classfile stuff in the review
>> cover message.)
>
> Like the others, it is caused by `Array::at_put()`.
>
>
> In file included from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/annotations.hpp:28,
> from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.hpp:29,
> from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/javaClasses.hpp:30,
> from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/precompiled/precompiled.hpp:35:
> In member function 'void Array::at_put(int, const T&) [with T = unsigned
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::symbol_at_put(int, Symbol*)' at
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:362:15,
> inlined from 'void
> ClassFileParser::mangle_hidden_class_name(InstanceKlass*)' at
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/classFileParser.cpp:5966:21:
`Array::_data` is a pseudo flexible array member. "Pseudo" because C++
doesn't have flexible array members. The compiler is completely justified in
complaining about the apparently out-of-bounds accesses.
There is a "well-known" (though moderately ugly) approach to doing flexible
array members in C++. Something like this:
T* data() {
return reinterpret_cast(
reinterpret_cast(this) + data_offset());
}
where `data_offset()` is new and private:
static size_t data_offset() {
return offset_of(Array, _data);
}
Use `data()` everywhere instead of using `_data` directly.
There are other places in HotSpot that use this kind of approach.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Stream.fromForEach(Consumer>)
Hi all,
a stream is kind of push iterator so it can be created from any object that has
a method like forEach(Consumer),
but sadly there is no static method to create a Stream from a Consumer of
Consumer so people usually miss that creating a Stream from events pushed to a
consumer is easy.
By example, let say i've a very simple parser like this, it calls the
listener/callback for each tokens
record Parser(String text) {
void parse(Consumer listener) {
for(var token: text.split(" ")) {
listener.accept(token);
}
}
}
Using the method Stream.fromForEach, we can create a Stream from the sequence
of tokens that are pushed through the listener
var parser = new Parser("1 2");
var stream = Stream.fromForEach(parser::parse);
It can also works with an iterable, an optional or even a collection
Iterable iterable = ...
var stream = Stream.fromForEach(iterable::forEachRemaning);
Optional optional = ...
var stream = Stream.fromForEach(optional::ifPresent);
List list = ...
var stream = Stream.fromForEach(list::forEach);
I known the last two examples already have their own method stream(), it's just
to explain how Stream.fromForEach is supposed to work.
In term of implementation, Stream.fromForEach() is equivalent to creating a
stream using a mapMulti(), so it can be implemented like this
static Stream fromForEach(Consumer> forEach) {
return Stream.of((T) null).mapMulti((__, consumer) ->
forEach.accept(consumer));
}
but i think that Stream.fromForEach(iterable::forEachRemaning) is more readable
than Stream.of(iterable).mapMult(Iterable::forEachRemaining).
The name fromForEach is not great and i'm sure someone will come with a better
one.
regards,
Rémi
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v3]
On Sat, 21 May 2022 15:30:29 GMT, Vladimir Kozlov wrote: > I started our testing. Please, wait results. Testing passed clean. - PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v12]
On Mon, 14 Mar 2022 19:12:29 GMT, iaroslavski wrote:
>> Sorting:
>>
>> - adopt radix sort for sequential and parallel sorts on
>> int/long/float/double arrays (almost random and length > 6K)
>> - fix tryMergeRuns() to better handle case when the last run is a single
>> element
>> - minor javadoc and comment changes
>>
>> Testing:
>> - add new data inputs in tests for sorting
>> - add min/max/infinity values to float/double testing
>> - add tests for radix sort
>
> iaroslavski has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
>
> * Improved mixed insertion sort
> * Optimized insertion sort
> * Improved merging sort
> * Optimized soring tests
hi Vladimir,
the main reason i raise the issue with catching out-of-memory-error is that
it's done very rarely in production code and also it's rather unpredictable
when multiple things are going on (e.g. one thread can allocate a lot of
memory, but other threads receive `oome`).
i've searched through /src/ in openjdk/jdk repository for `oome` (i.e. this
one) and found that most times the `oome` is caught is in `java.desktop` module
(probably this means the `oome` was related to native memory or other resources
external to jvm), so i've narrowed search to `java.base`:
https://github.com/search?q=outofmemoryerror+repo%3Aopenjdk%2Fjdk+path%3A%2Fsrc%2Fjava.base%2F+language%3AJava&type=Code&ref=advsearch&l=Java
and found just 2 cases of catching `oome` without always rethrowing it:
-
https://github.com/openjdk/jdk/blob/d8b0b32f9f4049aa678809aa068978e3a4e29457/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java#L1304
(but this rethrows if it internally fails with `oome` for a second time)
-
https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/java/util/concurrent/SubmissionPublisher.java#L1171
overall, it seems that swallowing `oome` (i.e. in this case: catching and not
rethrowing) is definitely not a readily used approach.
> We need additional buffer for int, long, float and double types only.
>
> We will have 2 constants which limit the number of int/float and
long/double elements in array, like this:
> (...)
> See, that the max number of elements for long/double is in 2 times less than
> for int/float,
because long/double occupies 2 times more bytes.
sounds good
> Why do we need to use Math.min(…, Integer.MAX_VALUE)? (...) So, limit by 2 Gb
> max is required.
yep, understood
> What do you think, what value of shift is the best, 6, 7, 8, 9, 10, 11, 12
> etc.?
> Runtime.getRuntime().maxMemory() >> 10??
>
> Do you have actual scenarios? Actual requirements? Your feedback are welcome!
keeping the extra buffer size below 1% of max memory should be safe enough.
currently the code is:
private static final int MAX_BUFFER_LENGTH =
(int) Math.min(Runtime.getRuntime().maxMemory() >> 6, 256L << 20);
// ...
private static Object tryAllocate(Object a, int length) {
if (length > MAX_BUFFER_LENGTH) {
return null;
}
try {
if (a instanceof int[]) {
return new int[length];
}
if (a instanceof long[]) {
return new long[length];
}
...
which means that in the worst case `sizeof(long) * (max memory >> 6) == 12.5%
of heap size limit` would be used which is a lot when someone runs java code in
memory constrained environment and that is rather common nowadays
(microservices, containers, etc).
-
PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v3]
On Sat, 21 May 2022 10:31:25 GMT, Quan Anh Mai wrote: >> Hi, >> >> This patch optimises the matching rules for floating-point comparison with >> respects to eq/ne on x86-64 >> >> 1, When the inputs of a comparison is the same (i.e `isNaN` patterns), `ZF` >> is always set, so we don't need `cmpOpUCF2` for the eq/ne cases, which >> improves the sequence of `If (CmpF x x) (Bool ne)` from >> >> ucomiss xmm0, xmm0 >> jp label >> jne label >> >> into >> >> ucomiss xmm0, xmm0 >> jp label >> >> 2, The move rules for `cmpOpUCF2` is missing, which makes patterns such as >> `x == y ? 1 : 0` to fall back to `cmpOpU`, which have a really high cost of >> fixing the flags, such as >> >> xorlecx, ecx >> ucomiss xmm0, xmm1 >> jnp done >> pushf >> andq[rsp], 0xff2b >> popf >> done: >> movleax, 1 >> cmovel eax, ecx >> >> The patch changes this sequence into >> >> xorlecx, ecx >> ucomiss xmm0, xmm1 >> movleax, 1 >> cmovpl eax, ecx >> cmovnel eax, ecx >> >> 3, The patch also changes the pattern of `isInfinite` to be more optimised >> by using `Math.abs` to reduce 1 comparison and compares the result with >> `MAX_VALUE` since `>` is more optimised than `==` for floating-point types. >> >> The benchmark results are as follow: >> >> Before >> After >> Benchmark Mode Cnt Score ErrorScore >> Error Unit Ratio >> FPComparison.equalDouble avgt5 2876.242 ± 58.875 594.636 ± >> 8.922 ns/op4.84 >> FPComparison.equalFloatavgt5 3062.430 ± 31.371 663.849 ± >> 3.656 ns/op4.61 >> FPComparison.isFiniteDoubleavgt5 475.749 ± 19.027 518.309 ± >> 107.352 ns/op0.92 >> FPComparison.isFiniteFloat avgt5 506.525 ± 14.417 515.576 ± >> 14.669 ns/op0.98 >> FPComparison.isInfiniteDouble avgt5 1232.800 ± 31.677 621.185 ± >> 11.935 ns/op1.98 >> FPComparison.isInfiniteFloat avgt5 1234.708 ± 70.239 623.566 ± >> 15.206 ns/op1.98 >> FPComparison.isNanDouble avgt5 2255.847 ± 7.238 400.124 ± >> 0.762 ns/op5.64 >> FPComparison.isNanFloatavgt5 2567.044 ± 36.078 546.486 ± >> 1.509 ns/op4.70 >> >> Thank you very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > comments Thank you for trying my suggestion and new comments. Approved. I started our testing. Please, wait results. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v7]
On Tue, 17 May 2022 13:32:42 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > revert changes for memnode.cpp and type.cpp In case of stringop-overflow errors (bytecodeAssembler.cpp, classFileParser.cpp, symbolTable.cpp) noted that offset of `Array::_data` might be [-2147483648, -1]. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v3]
On Wed, 4 May 2022 23:27:45 GMT, Vladimir Kozlov wrote: >> The changes to `Float` and `Double` look good. I don't think we need >> additional tests, see test/jdk/java/lang/Math/IeeeRecommendedTests.java. >> >> At first i thought we no longer need PR #8459 but it seems both PRs are >> complimentary, albeit PR #8459 has more modest performance gains for the >> intrinsics. > >> The changes to `Float` and `Double` look good. I don't think we need >> additional tests, see test/jdk/java/lang/Math/IeeeRecommendedTests.java. > > Thank you, Paul for pointing the test. It means we need to run tier4 (which > runs these tests with -Xcomp) to make sure methods are compiled by C2. @vnkozlov I have added comments to describe the changes in `cmpOpUCF` and the reasons behind the `cmov_regUCF2_eq` match rules. Using `expand` broke the build with `Syntax Error: :For expand in cmovI_regUCF2_eq to work, parameter declaration order in cmovI_regUCF2_ne must follow matchrule`. @sviswa7 (x != y) ? a : b can be calculated using pseudocode as follow: res = (!ZF || PF) ? a : b = !ZF ? a : (PF ? a : b) which can be calculated using cmovp rb, ra // rb1 = PF ? ra : rb cmovne rb, ra // rb2 = !ZF ? ra : rb1 = !ZF ? ra : (PF ? ra : rb) Furthermore, since `(x == y) == !(x != y)`, we have `((x == y) ? a : b) == ((x != y) ? b : a)`, which explains the implementation of `cmov_regUCF2_eq`. @vamsi-parasa Thanks a lot for your suggestion, I have modified the PR description as you say. - PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne [v3]
> Hi, > > This patch optimises the matching rules for floating-point comparison with > respects to eq/ne on x86-64 > > 1, When the inputs of a comparison is the same (i.e `isNaN` patterns), `ZF` > is always set, so we don't need `cmpOpUCF2` for the eq/ne cases, which > improves the sequence of `If (CmpF x x) (Bool ne)` from > > ucomiss xmm0, xmm0 > jp label > jne label > > into > > ucomiss xmm0, xmm0 > jp label > > 2, The move rules for `cmpOpUCF2` is missing, which makes patterns such as `x > == y ? 1 : 0` to fall back to `cmpOpU`, which have a really high cost of > fixing the flags, such as > > xorlecx, ecx > ucomiss xmm0, xmm1 > jnp done > pushf > andq[rsp], 0xff2b > popf > done: > movleax, 1 > cmovel eax, ecx > > The patch changes this sequence into > > xorlecx, ecx > ucomiss xmm0, xmm1 > movleax, 1 > cmovpl eax, ecx > cmovnel eax, ecx > > 3, The patch also changes the pattern of `isInfinite` to be more optimised by > using `Math.abs` to reduce 1 comparison and compares the result with > `MAX_VALUE` since `>` is more optimised than `==` for floating-point types. > > The benchmark results are as follow: > > Before After > Benchmark Mode Cnt Score ErrorScore > Error Unit Ratio > FPComparison.equalDouble avgt5 2876.242 ± 58.875 594.636 ± > 8.922 ns/op4.84 > FPComparison.equalFloatavgt5 3062.430 ± 31.371 663.849 ± > 3.656 ns/op4.61 > FPComparison.isFiniteDoubleavgt5 475.749 ± 19.027 518.309 ± > 107.352 ns/op0.92 > FPComparison.isFiniteFloat avgt5 506.525 ± 14.417 515.576 ± > 14.669 ns/op0.98 > FPComparison.isInfiniteDouble avgt5 1232.800 ± 31.677 621.185 ± > 11.935 ns/op1.98 > FPComparison.isInfiniteFloat avgt5 1234.708 ± 70.239 623.566 ± > 15.206 ns/op1.98 > FPComparison.isNanDouble avgt5 2255.847 ± 7.238 400.124 ± > 0.762 ns/op5.64 > FPComparison.isNanFloatavgt5 2567.044 ± 36.078 546.486 ± > 1.509 ns/op4.70 > > Thank you very much. Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision: comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8525/files - new: https://git.openjdk.java.net/jdk/pull/8525/files/ba93dcf2..7fcfe4a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8525&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8525&range=01-02 Stats: 11 lines in 1 file changed: 10 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8525.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8525/head:pull/8525 PR: https://git.openjdk.java.net/jdk/pull/8525
