Re: RFR: 8322292: Rearrange comparison of fields in Record.equals()
On Mon, 18 Dec 2023 13:42:35 GMT, Sergey Tsypanov wrote:
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Generally, this is a good idea. A few comments:
1. Arrays are compared by reference rather than element-wise, so they
should/could be at the beginning similar to enums, not at the end.
2. You are sorting the array passed to the bootstrap method. This might be
observable to callers, potentially causing problems, especially when e.g.
creating the method handle for equals first, and then for toString with the
same array.
3. What's the test situation here? Is there enough coverage for things like
that already?
-
PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1862179369
Re: Omission in javadoc Stream.toArray(): Safe array contract stipulation
Hi Reinier,
I see you asked this here as well as on Stack Overflow. I'll repeat my answer from
Stack Overflow here and then continue with some additional discussion.
Quick recap: Collection::toArray has a requirement that the returned array be
"safe", whereas Stream::toArray has no such requirement. In my earlier Stack
Overflow answer (which you quoted) I mentioned something like "if [Stream.toArray]
were to violate its spec", and you asked, what spec?
My answer:
I probably mistakenly assumed that the Stream::toArray spec had the same "safety"
requirements as Collection::toArray. That was our intent, as I recall, but it
didn't make it into the spec.
Now to your questions in this email.
The second question, where to report this, is right here! We can discuss this and
file bugs/PRs etc. if necessary depending on the outcome of the discussion.
The first question has some subtlety to it. Should we not update the specification
for Stream::toArray to have the same "safety" requirement as Collection::toArray?
I guess a change would serve to clarify the specification. The developer of a
clean-room implementation of streams might reasonably think that, if the stream
source is an array, that this array can simply be returned from toArray(). However,
that violates the intent (which is pretty clear in my memory) that the returned
array be freshly created. Usually, adding assertions to interfaces creates a danger
of invalidating existing implementations. However, I don't think returning an
existing array, especially one that might be aliased, was ever considered to be
valid behavior. Thus, a spec change would be more like filling a gap that shouldn't
have been there in the first place, not adding a new semantic requirement.
As a practical matter, there are few third-party implementations, this requirement
is almost impossible to test, and any callers of toArray() won't change. That is, if
they're currently using the returned array, they'll continue using it; but if they
want 100% assurance the array isn't aliased, they'll continue to make a defensive
copy. So, it doesn't seem like changing the spec will have any practical impact.
Have I answered your questions?
s'marks
On 12/14/23 5:46 AM, Reinier Zwitserloot wrote:
Hi core libs team,
I think I found a rather inconsequential and esoteric bug, though the term is
somewhat less objectively defined when the problem exists solely in how complete
some method’s javadoc is.
Two questions: Is there a plausible argument that the javadoc as is, shouldn’t be
updated? If not, what’s the right place to report this javadoc ‘bug’?
The issue:
A snippet of the javadoc of Collection.toArray, from current HEAD jdk22u:
The returned array will be "safe" in that no references to it are
* maintained by this collection. (In other words, this method must
* allocate a new array even if this collection is backed by an array).
* The caller is thus free to modify the returned array.
The javadoc of Stream.toArray, from current HEAD jdk22u - has no such rider
anywhere in its documentation nor on the javadoc of the Stream interface, nor on
the package javadoc. The javadoc is quite short; the complete docs on toArray():
/**
* Returns an array containing the elements of this stream.
*
* This is a terminal
* operation.
*
* @return an array, whose {@linkplain Class#getComponentType runtime
component
* type} is {@code Object}, containing the elements of this stream
*/
The more usually used variant taking a function that makes the array has slightly
longer javadoc, but it similarly makes no mention whatsoever about the contract
stipulation that any implementors must not keep a reference.
A snippet from Stuart Marks on a stack overflow question about a to the asker
weird choice about stream’s toList()’s default implementation
(https://stackoverflow.com/questions/77473755/is-it-necessary-to-copy-a-list-to-be-safe/77474199?noredirect=1#comment136909551_77474199):
@Holger The extra step in the default implementation is there to force a defensive
copy if |this.toArray| were to violate its spec and keep a reference to the
returned array. Without the defensive copy, it would be possible to modify the
list returned from the default |toList| implementation.
Which leads to the obvious question: Where is that ’spec’ that Stuart is referring
to? Either the javadoc is the spec and therefore the javadoc is buggy, in that it
fails to mention this stipulation, or the spec is elsewhere, in which case surely
the javadoc should link to it or copy it.
Possibly this is jus filed away as: “Unlike with Collection, Stream instances are
disposable; after a terminal operation (and toArray is terminal) has been invoked
on it, that stream object has ceased being useful. Therefore, there is no
perceived value to caching any created array, therefore, it doesn’t need mentioning".
Except, as
Withdrawn: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Sat, 9 Dec 2023 23:19:52 GMT, John Jiang wrote: > It looks the `else-if` and `else` clauses in method > `ArraysSupport::hugeLength` could be simplified by `Math::max`. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17043
Re: RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Tue, 19 Dec 2023 03:44:26 GMT, Stuart Marks wrote: >> It looks the `else-if` and `else` clauses in method >> `ArraysSupport::hugeLength` could be simplified by `Math::max`. > > This change would make the code shorter, but in my opinion, it obscures > what's going on. This code tries to be very careful about avoiding problems > caused by integer overflow/wraparound, and in order to do that, it needs to > make sure that the full range of int values is handled properly. With the > explicit if/else code, this is clear. Using Math.max() instead tends to make > this obscure. > > This came up previously; see > > https://github.com/openjdk/jdk/pull/1617#discussion_r536260884 @stuart-marks Thanks for your clarification! Just closed this PR and JBS. - PR Comment: https://git.openjdk.org/jdk/pull/17043#issuecomment-1862076153
Re: RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Sat, 9 Dec 2023 23:19:52 GMT, John Jiang wrote: > It looks the `else-if` and `else` clauses in method > `ArraysSupport::hugeLength` could be simplified by `Math::max`. This change would make the code shorter, but in my opinion, it obscures what's going on. This code tries to be very careful about avoiding problems caused by integer overflow/wraparound, and in order to do that, it needs to make sure that the full range of int values is handled properly. With the explicit if/else code, this is clear. Using Math.max() instead tends to make this obscure. This came up previously; see https://github.com/openjdk/jdk/pull/1617#discussion_r536260884 - PR Comment: https://git.openjdk.org/jdk/pull/17043#issuecomment-1862068604
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li 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: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 > Have you tested with gcc 9? Or is this just supposition based on gcc9 having > removed the experimental status for C++17? I have not tested GCC 8 and 9. @sviswa7 seems to test them. > I have verified that with the above change the builds (release, fastdebug, > slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and > the test/jdk/java/util/Arrays/Sorting.java passes successfully with these > builds. Thanks for your tests. But from the description of the [GCC document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. > Some C++17 features are available since GCC 5, but support was experimental > and the ABI of C++17 features was not stable until GCC 9. What do you think about it? - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1861998852
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote:
>> Hi all,
>>
>> This patch fixes the building failure introduced by
>> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC
>> version (linux & GCC 7.5.0 locally).
>>
>> Thanks for the review.
>>
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li 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:
>
> - Bump the needed version of GCC.
> - Revert previous change.
> - Merge branch 'master' into JDK-8321688
> - JDK-8321688
Let us restrict the libsimdsort.so build to GCC 8.4.0 with the following change:
`diff --git a/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
b/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
index c73fd7920d8..2a2946cff82 100644
--- a/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
+++ b/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
@@ -31,9 +31,9 @@
#define assert(cond, msg) { if (!(cond)) { fprintf(stderr, "assert fails %s
%d: %s\n", __FILE__, __LINE__, msg); abort(); }}
-// GCC >= 7.5 is needed to build AVX2 portions of libsimdsort using C++17
features
-#if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 7) || ((__GNUC__ ==
7) && (__GNUC_MINOR__ >= 5
+// GCC >= 8.4 is needed to build AVX2 portions of libsimdsort using C++17
features
+#if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 8) || ((__GNUC__ ==
8) && (__GNUC_MINOR__ >= 4
#define __SIMDSORT_SUPPORTED_LINUX
#endif
-#endif //SIMDSORT_SUPPORT_HPP
\ No newline at end of file
+#endif //SIMDSORT_SUPPORT_HPP`
I have verified that with the above change the builds (release, fastdebug,
slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and
the test/jdk/java/util/Arrays/Sorting.java passes successfully with these
builds.
-
PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1861889879
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 15:54:57 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 548:
>
>> 546: static ClassDesc classDesc(Class cls) {
>> 547: return cls.isHidden() ?
>> ClassDesc.ofInternalName(cls.getName().replace('.', '/'))
>> 548: :
>> ClassDesc.ofDescriptor(cls.descriptorString());
>
> That said, all the usages already anticipate a valid `ClassDesc` that can be
> encoded in bytecode except `implClass`. You should make 2 versions of
> `classDesc()`, one that eagerly throws for all other usages, and another
> special version that handles hidden class conversion (with the same code as
> existing method) for
> https://github.com/openjdk/jdk/pull/17108/files#diff-76236a0dd20022f749d95ad5890e2bece0c4ac212d1b2ffab90ca86875f14282R173
Split, thank you.
> src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java
> line 162:
>
>> 160: } else {
>> 161: Class src;
>> 162: if (arg == functional || functional.isPrimitive()) {
>
> `arg == functional` avoids a cast, though it's not in parity with the older
> code. Should probably restore the old cast that takes a source argument to
> avoid adding `==` checks to avoid casts.
Original cast is avoided when class names (derived from descriptors, derived
from classes) are equal.
Correct me, please, but I think that this simple `arg == functional` check does
the same job, without unnecessary String conversions and comparisons.
Or is there a case, where the cast is missing or added incorrectly?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429931780
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429919564
Re: RFR: 8320919: Clarify Locale related system properties [v8]
> This is a doc change to clarify what the `Default Locale` is, and how it is > established during the system startup using the system properties. Those > locale-related system properties have existed since the early days of Java, > but have never been publicly documented before. It is also the intention of > this PR to clarify those system properties and how they are overridden. A > corresponding CSR has been drafted. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Reflects review comments - Reflects review comments - Reflects review comments - Reflects review comments - Review comments - Update src/java.base/share/classes/java/util/Locale.java Co-authored-by: Justin Lu - Update src/java.base/share/classes/java/util/Locale.java Co-authored-by: Justin Lu - Update src/java.base/share/classes/java/util/Locale.java Co-authored-by: Justin Lu - Update src/java.base/share/classes/java/util/Locale.java Co-authored-by: Justin Lu - 6th draft - ... and 5 more: https://git.openjdk.org/jdk/compare/1fde8b86...4a76c89c - Changes: https://git.openjdk.org/jdk/pull/17065/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17065=07 Stats: 81 lines in 1 file changed: 71 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/17065.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17065/head:pull/17065 PR: https://git.openjdk.org/jdk/pull/17065
Re: RFR: 8259637: java.io.File.getCanonicalPath() returns different values for same path [v2]
> Modify the `collapse()` function to remove each instance of ".." when the > path is absolute and there is no preceding name. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8259637: Use Stream.of in test - Changes: - all: https://git.openjdk.org/jdk/pull/17089/files - new: https://git.openjdk.org/jdk/pull/17089/files/d2f755b0..cd2b3776 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17089=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17089=00-01 Stats: 14 lines in 1 file changed: 0 ins; 7 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/17089.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17089/head:pull/17089 PR: https://git.openjdk.org/jdk/pull/17089
[jdk22] RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled
Hi all, This pull request contains a backport of commit [fde5b168](https://github.com/openjdk/jdk/commit/fde5b16817c3263236993f2e8c2d2469610d99bd) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Aleksei Voitylov on 14 Dec 2023 and was reviewed by Roger Riggs. Thanks! - Commit messages: - Backport fde5b16817c3263236993f2e8c2d2469610d99bd Changes: https://git.openjdk.org/jdk22/pull/18/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=18=00 Issue: https://bugs.openjdk.org/browse/JDK-8321514 Stats: 27 lines in 2 files changed: 25 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk22/pull/18.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/18/head:pull/18 PR: https://git.openjdk.org/jdk22/pull/18
Withdrawn: 8310901: Convert String::newStringNoRepl with Latin-1 to String::newStringLatin1NoRepl
On Mon, 26 Jun 2023 15:25:08 GMT, Glavo wrote: > Added a new method `newStringLatin1NoRepl` to the `JavaLangAccess`. > > Reasons: > > * Most use cases of `newStringNoRepl` use `ISO_8859_1` as the charset, > creating a new shortcut can make writing shorter; > * Since all possible values of `byte` are legal Latin-1 characters, > `newStringLatin1NoRepl` **will not throw `CharacterCodingException`**, so > users can make the compiler happy without using useless try-catch statements. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14655
Re: RFR: 8320707: Virtual thread test updates
On Sat, 16 Dec 2023 17:25:20 GMT, Alan Bateman wrote: > A lot of test changes have accumulated in the loom repo, this includes both > new tests and updates to existing tests. Some of these updates can be brought > to the main line. This update brings over: > > - The existing tests for pinning use synchronized blocks. In preparation for > changes to allow carrier thread be released when a virtual thread parks > holding a monitor or blocks on monitorenter, these tests are changed to pin > by having a native frame on the stack. This part includes test infrastructure > to make it easy to add more tests that do operations while pinned. The tests > still test what they were originally created to test of course. > > - The test for the JFR jdk.VirtualThreadPinned event is refactored to allow > for additional cases where the event may be reported. > > - ThreadAPI is expanded to cover test for uncaught exception handling. > > - GetStackTraceWhenRunnable is refactored to not use a Selector, otherwise > this test will be invalidated when blocking selection operations release the > carrier. > > - StressStackOverflow is dialed down to run for 1m instead of 2mins. > > - The use of CountDownLatch in a number of tests that poll thread state has > been dropped to keep the tests as simple as possible. Can you explain your motivation for using AtomicBoolean with a polling loop rather than CountDownLatch(1)? I'm working on a test where I just added a CountDownLatch(1) and am wondering if I should do the same, but I'm not sure if there is something about these tests that is motivating the change. - PR Comment: https://git.openjdk.org/jdk/pull/17136#issuecomment-1861578806
Re: RFR: 8259637: java.io.File.getCanonicalPath() returns different values for same path [v2]
On Sun, 17 Dec 2023 08:53:15 GMT, Alan Bateman wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8259637: Use Stream.of in test
>
> test/jdk/java/io/File/GetCanonicalPath.java line 98:
>
>> 96: "/b/c"));
>> 97:
>> 98: return list.stream();
>
> You could use Stream.of here, e.g.
>
>
> return Stream.of(
> Arguments.of("/../../../../../a/b/c", "/a/b/c"),
> Arguments.of("/../../../../../a/../b/c", "/b/c"),
> Arguments.of("/../../../../../a/../../b/c", "/b/c"),
> Arguments.of("/../../../../../a/../../../b/c", "/b/c"),
> Arguments.of("/../../../../../a/../../../../b/c", "/b/c")
> );
So changed in cd2b3776463ddc95b36f7b65be12da7ac2686c53.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17089#discussion_r1430388844
Re: RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Sat, 9 Dec 2023 23:19:52 GMT, John Jiang wrote: > It looks the `else-if` and `else` clauses in method > `ArraysSupport::hugeLength` could be simplified by `Math::max`. Could this simple PR be reviewed? - PR Comment: https://git.openjdk.org/jdk/pull/17043#issuecomment-1860729032
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li 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: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 src/java.base/linux/native/libsimdsort/simdsort-support.hpp line 35: > 33: > 34: // GCC >= 9.1 is needed to build AVX2 portions of libsimdsort using C++17 > features > 35: #if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 9) || ((__GNUC__ > == 9) && (__GNUC_MINOR__ >= 1 Have you tested with gcc 9? Or is this just supposition based on gcc9 having removed the experimental status for C++17? - PR Review Comment: https://git.openjdk.org/jdk/pull/17047#discussion_r1430308457
RFR: 8322292: Rearrange comparison of fields in Record.equals()
Currently if we create a record it's fields are compared in their declaration
order. This might be ineffective in cases when two objects have "heavy" fields
equals to each other, but different "lightweight" fields (heavy and lightweight
in terms of comparison) e.g. primitives, enums, nullable/non-nulls etc.
If we have declared a record like
public record MyRecord(String field1, int field2) {}
It's equals() looks like:
public final equals(Ljava/lang/Object;)Z
L0
LINENUMBER 3 L0
ALOAD 0
ALOAD 1
INVOKEDYNAMIC
equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
[
// handle kind 0x6 : INVOKESTATIC
java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
// arguments:
com.caspianone.openbanking.productservice.controller.MyRecord.class,
"field1;field2",
// handle kind 0x1 : GETFIELD
com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
// handle kind 0x1 : GETFIELD
com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
]
IRETURN
L1
LOCALVARIABLE this
Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
MAXSTACK = 2
MAXLOCALS = 2
This can be improved by rearranging the comparison order of the fields moving
enums and primitives upper, and collections/arrays lower.
-
Commit messages:
- 8322292: Update copyright
- 8322292: Shift arrays and Iterables down
- Improve Record.equals()
Changes: https://git.openjdk.org/jdk/pull/17143/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17143=00
Issue: https://bugs.openjdk.org/browse/JDK-8322292
Stats: 12 lines in 1 file changed: 9 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Event enabled on profile.jfc but disabled on default.jfc. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/629e8f23..8534d7af Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17129=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17129=01-02 Stats: 37 lines in 5 files changed: 27 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: 8275338: Add JFR events for notable serialization situations [v2]
On Sat, 16 Dec 2023 01:27:17 GMT, Erik Gahlin wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Restrict accessibility of nested classes. > > It seems correct to have the event disabled in both configurations. It's not > hard to imagine pathological cases where millions of stream objects are > created, which would create overhead and flood event buffers, making JFR not > usable. > > That said, I wonder how many will actually use, or find out about the event, > if they need to edit the .jfc file or specify > -XX:StartFlightRecording:jdk.SerializationMisdeclaration#enabled=true on > command line? > > How important is the stack trace when diagnosing serialization > misdeclarations? > > We faced a similar situation with the Finalization event a couple of years > ago, but we were able to make the event periodic and enabled by default with > some VM magic. An event that is enabled by default will probably be seen by > 10 000 times as many users, so it might be worth considering other > approaches, even if they are harder to implement, or we lose some of the > information. > @egahlin Yes, disabled by default means less overhead in the usual case. > > On the other hand, the checks and the events generation are done for > serializable classes that actually actively participate in serialization, > lazily, and just once per class, so maybe they are not as invasive and might > be "always enabled". Pathological cases with millions of serializable classes > might indeed lead to problems with "always enable", although they should be > quite rare. > If it's once per class per JVM, I think it's fine to enable it in profile.jfc. It's meant to have low overhead (around 1%) for the typical application, but OK with higher in pathological cases. Loading/initiating a class has a cost, so it will likely dominate. > I don't think that stack traces are that useful for this kind of event, since > they are mostly related to the static class structure rather than to runtime > characteristics. Looking at the event itself should help in locating the > problem quickly: it reports the class, the member, and what's questionable > with it. > If we were to make the event periodic, it's hard to keep the stack trace around, at least in Java, so I wanted to know how important it is. Now if we can have it in profile.jfc, I think it's fine to keep it. If you think the stack trace has no use at all, then you could remove it and perhaps also the thread using the RemoveFields annotation. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1860765344
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v7]
On Fri, 15 Dec 2023 10:49:56 GMT, Serguei Spitsyn wrote: >> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 >> time frame. >> It is fixing a deadlock issue between `VirtualThread` class critical >> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, >> `getAndClearInterrupt()`, `threadState()`, `toString()`), >> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. >> The deadlocking scenario is well described by Patricio in a bug report >> comment. >> In simple words, a virtual thread should not be suspended during >> 'interruptLock' critical sections. >> >> The fix is to record that a virtual thread is in a critical section >> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about >> begin/end of critical section. >> This bit is used in `HandshakeState::get_op_for_self()` to filter out any >> `HandshakeOperation` if a target `JavaThread` is in a critical section. >> >> Some of new notifications with `notifyJvmtiSync()` method is on a >> performance critical path. It is why this method has been intrincified. >> >> New test was developed by Patricio: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> The test is very nice as it reliably in 100% reproduces the deadlock without >> the fix. >> The test is never failing with this fix. >> >> Testing: >> - tested with newly added test: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: improve an assert message Alan and Leonid, thank you for review! Will push after the final mach5 testing is completed. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1860980377
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge - review: improve an assert message - review: moved a couple of comments out of try blocks - review: moved notifyJvmtiDisableSuspend(true) out of try-block - review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks - review: (1) rename notifyJvmti method; (2) add try-final statements to VirtualThread methods - Resolved merge conflict in VirtualThread.java - added @summary to new test SuspendWithInterruptLock.java - add new test SuspendWithInterruptLock.java - 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable - Changes: https://git.openjdk.org/jdk/pull/17011/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17011=07 Stats: 229 lines in 15 files changed: 196 ins; 0 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8275338: Add JFR events for notable serialization situations [v2]
On Sat, 16 Dec 2023 01:27:17 GMT, Erik Gahlin wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Restrict accessibility of nested classes. > > It seems correct to have the event disabled in both configurations. It's not > hard to imagine pathological cases where millions of stream objects are > created, which would create overhead and flood event buffers, making JFR not > usable. > > That said, I wonder how many will actually use, or find out about the event, > if they need to edit the .jfc file or specify > -XX:StartFlightRecording:jdk.SerializationMisdeclaration#enabled=true on > command line? > > How important is the stack trace when diagnosing serialization > misdeclarations? > > We faced a similar situation with the Finalization event a couple of years > ago, but we were able to make the event periodic and enabled by default with > some VM magic. An event that is enabled by default will probably be seen by > 10 000 times as many users, so it might be worth considering other > approaches, even if they are harder to implement, or we lose some of the > information. @egahlin Yes, disabled by default means less overhead in the usual case. On the other hand, the checks and the events generation are done for serializable classes that actually actively participate in serialization, lazily, and just once per class, so maybe they are not as invasive and might be "always enabled". Pathological cases with millions of serializable classes might indeed lead to problems with "always enable", although they should be quite rare. I don't think that stack traces are that useful for this kind of event, since they are mostly related to the static class structure rather than to runtime characteristics. Looking at the event itself should help in locating the problem quickly: it reports the class, the member, and what's questionable with it. Or am I misunderstanding your points? - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1860655448
Integrated: 8259637: java.io.File.getCanonicalPath() returns different values for same path
On Wed, 13 Dec 2023 19:37:15 GMT, Brian Burkhalter wrote: > Modify the `collapse()` function to remove each instance of ".." when the > path is absolute and there is no preceding name. This pull request has now been integrated. Changeset: b98d13fc Author:Brian Burkhalter URL: https://git.openjdk.org/jdk/commit/b98d13fc3c7f6747d9201eb884cf9d3181671ccb Stats: 37 lines in 2 files changed: 30 ins; 1 del; 6 mod 8259637: java.io.File.getCanonicalPath() returns different values for same path Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17089
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v4]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/8fee4564..098df109 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=02-03 Stats: 145 lines in 7 files changed: 51 ins; 55 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:53:26 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/MethodType.java line 1295:
>
>> 1293: public Optional describeConstable() {
>> 1294: try {
>> 1295: var params = new ClassDesc[parameterCount()];
>
> We can probably handle like this:
>
> var retDesc = returnType().describeConstable();
> if (retDesc.isEmpty())
> return Optional.empty();
>
> if (parameterCount() == 0)
> return Optional.of(MethodTypeDesc.of(retDesc.get()));
>
> var params = new ClassDesc[parameterCount()];
> for (int i = 0; i < params.length; i++) {
> var paramDesc = parameterType(i).describeConstable();
> if (paramDesc.isEmpty())
> return Optional.empty();
> params[i] = paramDesc.get();
> }
> return Optional.of(MethodTypeDesc.of(returnType().get(), params));
>
>
> To avoid creating exceptions and allocating array when the params array is
> empty.
yes, it looks better, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 379:
>
>> 377: @Override
>> 378: public ClassEntry classEntry(ClassDesc classDesc) {
>> 379: if (classDesc == ConstantDescs.CD_Object) {
>
> What causes the slow lookup of Object CE? If it's because that hashing needs
> to compute the substring first, I recommend changing the hashing algorithm if
> that's the case.
Right, I'll take this specific shortcut back and will think about more generic
performance improvement.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429863879
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429866344
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:49:26 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 2290:
>
>> 2288: int accessFlags;
>> 2289: try {
>> 2290: ClassModel cm =
>> java.lang.classfile.ClassFile.of().parse(bytes);
>
> No need for fully-qualified name.
Unfortunately there is conflict with MethodHandles.Lookup.ClassFile
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429855148
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:42:09 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1519:
>
>> 1517: // double - d2i,i2b d2i,i2s d2i,i2cd2i
>> d2l d2f <->
>> 1518: if (from != to && from != TypeKind.BooleanType && to !=
>> TypeKind.BooleanType) try {
>> 1519: cob.convertInstruction(from, to);
>
> Need an intermediate int if you are converting from long/float/double to
> byte/short/char.
Will fix, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429794214
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v3]
On Sat, 16 Dec 2023 16:37:16 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply suggestions from code review
>>
>> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 468:
>
>> 466: }
>> 467:
>> 468: private void emitStoreInsn(BasicType type, int index) {
>
> If we are going to use `localsMap` in every removed `emitStoreInsn`, perhaps
> we should restore it instead? Will also make it easier to move away from
> localsMap to CodeBuilder's tracking system.
Right, I'll restore emitLoadInsn and emitStoreInsn
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429791664
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:32:23 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1145:
>
>> 1143: private static Opcode popInsnOpcode(BasicType type) {
>> 1144: switch (type) {
>> 1145: case I_TYPE:
>
> Should restore the enhanced switch syntax
Will fix, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429753608
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
On Sat, 16 Dec 2023 16:12:05 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added missing comment
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1171:
>
>> 1169:
>> cob.getfield(ClassDesc.ofInternalName("java/lang/invoke/MethodHandleImpl$CasesHolder"),
>> "cases",
>> 1170: CD_MethodHandle.arrayType());
>> 1171: int casesLocal = extendLocalsMap(new Class[] {
>> MethodHandle[].class });
>
> We should look into replacing the local map with `CodeBuilder.allocateLocal`
> etc. in the future.
Yes, however actual API does not match directly.
It would require an extension of the API or more significant refactoring of
InvokerBytecodeGenerator.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1429746763
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v3]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/0f356eb3..8fee4564 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=01-02 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
