Re: RFR: 8330595: Invoke ObjectMethods::bootstrap method exactly
On Thu, 18 Apr 2024 20:11:15 GMT, Claes Redestad wrote:
> Investigating a recent regression in mainline I realized we have an
> opportunity to improve the bootstrap overheads of ObjectMethods::bootstrap by
> using `invokeExact` in a way similar to what we already do for LMF and SCF
> BSMs. This avoids generating type checking adapters and equates to a one-off
> startup win of around 5ms in any app that ever has the need to spin up a
> toString, equals or hashCode method on a record.
src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
150:
> 148: } else if (isObjectMethodsBootstrapBSM(bsmType)) {
> 149: MethodHandle[] methodHandles = new
> MethodHandle[argv.length - 2];
> 150: System.arraycopy(argv, 2, methodHandles, 0,
> argv.length - 2);
Are we avoiding `Arrays.copyOfRange(argv, 2, argv.length, MethodHandle.class)`
because of the JIT? If JIT compiler can recognize `MethodHandle.class` constant
and inline I recommend using copyOfRange like for string concat above.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18845#discussion_r1571783566
RE: Vector (and integer) API: unsigned min/max
Using compare the unsigned max(va, vb) could be written as: var va = IntVector.fromArray(ispecies, ia, i); var vb = IntVector.fromArray(ispecies, ib, i); var vm = va.compare(UNSIGNED_LT, vb); va.blend(vb, vm).intoArray(ir, i); Where ia and ib are input integer arrays and ir is the output integer array. For x86_64 platforms supporting AVX512 compare method could be faster than the add with offset method. Best Regards, Sandhya -Original Message- From: Paul Sandoz Sent: Thursday, April 18, 2024 11:40 AM To: David Lloyd Cc: core-libs-dev@openjdk.org; Viswanathan, Sandhya Subject: Re: Vector (and integer) API: unsigned min/max Hi David, It’s not at all outlandish, but would caution it's more work than one might initially think. Could you describe a little more about your use case? that can be helpful to understand the larger picture and demand. Using unsigned comparison would be my recommended approach with the current API. CC'ing Sandhya for her opinion. Generally when we add new Vector operations we also consider the impact on the scalar types and try to fill any gaps there, so the vector operation behavior is composed from the scalar operation behavior (so like you indicated regarding symmetry). We are seeing demand for saturated arithmetic primarily for vector (not “vector” as in hardware vector) search, so we may do something there for specific integral types. Paul. > On Apr 17, 2024, at 7:13 AM, David Lloyd wrote: > > I've been trying the the incubating Vector API and one thing I've missed on > integer-typed vectors is having unsigned min/max operations. There is a > signed min/max operation, and unsigned comparison, but no unsigned min/max. > > I guess this is for symmetry with `Math`, which only has signed `min`/`max`. > However, I would point out that while it's not very hard to implement one's > own unsigned min/max for integer types using `compareUnsigned`, it is a bit > harder to do so with vectors. The best I've come up with is to take one of > two approaches: > > 1. Zero-extend the vector to the next-larger size, perform the > min/max, and reduce it back down again, or 2. Add .MIN_VALUE, > min/max with a value or vector also offset by .MIN_VALUE, and > then subtract the offset again > > I guess a third approach could be to do a comparison using unsigned compares, > and then use the resultant vector mask to select items from the original two > vectors, but I didn't bother to work out this solution given I already had > the other two options. > > Would it be feasible to add unsigned min/max operations for vectors? It looks > like at least AArch64 has support for this as a single instruction, so it > doesn't seem too outlandish. > > And as a separate (but related) question, what about > `Math.minUnsigned`/`Math.maxUnsigned` of `int` and `long` for symmetry? > > -- > - DML • he/him
Re: RFR: 8330542: Add two sample configuration files in preparation for a more secure by default configuration [v2]
On Thu, 18 Apr 2024 21:54:26 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >> jaxp-compat.properties: used to regain compatibility from any more >> restricted configuration than previous versions such as JDK 22 > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add description of the three configuration files. src/java.xml/share/conf/jaxp-strict.properties line 2: > 1: > > 2: # JAXP String Configuration File Nit: typo here: String -> Strict - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1571515112
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote: > Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18846#pullrequestreview-2010068091
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote: > Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18846#pullrequestreview-2010059355
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface [v2]
> **Subject**
> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
> `java.util.List`
>
> **Motivation**
> The motivation behind this proposal is to enhance the functionality of the
> `List` interface by providing a more flexible way to find the index of an
> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
> object as a parameter. This limits the flexibility of these methods as they
> can only find the index of exact object matches.
>
> The proposed methods would accept a `Predicate` as a parameter, allowing
> users to define a condition that the desired element must meet. This would
> provide a more flexible and powerful way to find the index of an element in a
> list.
>
> Here is a brief overview of the changes made in this pull request:
>
> 1. Added the `indexOf(Predicate filter)` method to the `List`
> interface.
> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
> interface.
> 3. Implemented these methods in all non-abstract classes that implement the
> `List` interface.
>
> The changes have been thoroughly tested to ensure they work as expected and
> do not introduce any regressions. The test cases cover a variety of scenarios
> to ensure the robustness of the implementation.
>
> For example, consider the following test case:
>
> List list = new ArrayList<>();
> list.add("Object one");
> list.add("NotObject two");
> list.add("NotObject three");
>
> int index1 = list.indexOf(s -> s.contains("ct t"));
> System.out.println(index1); // Expected output: 1
> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
> System.out.println(index2); // Expected output: 2
>
>
> Currently, to achieve the same result, we would have to use a more verbose
> approach:
>
> int index1 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).contains("ct t"))
> .findFirst()
> .orElse(-1);
> System.out.println(index1); // Output: 1
> int index2 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).startsWith("NotObject"))
> .reduce((first, second) -> second)
> .orElse(-1);
> System.out.println(index2); // Output: 2
>
>
> I believe these additions would greatly enhance the functionality and
> flexibility of the `List` interface, making it more powerful and
> user-friendly. I look forward to your feedback and am open to making any
> necessary changes based on your suggestions.
>
> Thank you for considering this proposal.
>
> Best regards
Evemose has updated the pull request incrementally with one additional commit
since the last revision:
empty commit to trigger check rerun
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18639/files
- new: https://git.openjdk.org/jdk/pull/18639/files/025bcf39..21ce4892
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18639=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18639=00-01
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18639.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18639/head:pull/18639
PR: https://git.openjdk.org/jdk/pull/18639
Integrated: 8295111: dpkg appears to have problems resolving symbolically linked native libraries
On Thu, 18 Apr 2024 19:56:43 GMT, Alexey Semenyuk wrote: > Pass a path with resolved symlinks to `dpkg -S` command. If it fails, try the > original path if they differ. > > Testing on Ubuntu 24.04 passed. Successfully created a .deb package from > SwingSet2.jar with the not-empty list of dependencies. Before the fix the > list of required packages was empty. In jpackage log: > > > [19:30:48.784] Running dpkg > [19:30:49.118] Command [PID: 244104]: > dpkg -S /usr/lib/x86_64-linux-gnu/libm.so.6 > [19:30:49.118] Output: > libc6:amd64: /usr/lib/x86_64-linux-gnu/libm.so.6 > [19:30:49.118] Returned: 0 > > [19:30:49.118] /lib/x86_64-linux-gnu/libm.so.6 is provided by [libc6] > > > It was looking for a package providing "/lib/x86_64-linux-gnu/libm.so.6" > library, but the actual argument to "dpgk -S" command was > "/usr/lib/x86_64-linux-gnu/libm.so.6". This pull request has now been integrated. Changeset: 32946e18 Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/32946e1882e9b22c983cbba3c6bda3cc7295946a Stats: 44 lines in 1 file changed: 19 ins; 3 del; 22 mod 8295111: dpkg appears to have problems resolving symbolically linked native libraries Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/18844
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Tue, 9 Apr 2024 22:02:56 GMT, xxDark wrote: > This does not solve the problem though. Source compatibility would still be > broken as one would still need to add casts everywhere, or am I > misinterpreting something? The code you have provided in example is still valid and non-ambigous, even without cast. The default behaviour of java compiler is to choose method where parameter at corresponding position is closest type upwards in class hierarchy. null is considered "bottom" type, which means it is at the bottom of simultaneously every class hierarchy. Compiler looks upward in class hierarchy and first class it encounters is Predicate, as Object is superclass of Predicate, and chooses indexOf(Predicate) as method to invoke. When casting null to type, compiler treats null as object of casted type rather then bottom type. However, if changes are delivered as is, default behaviour of indexOf(null) will be changed, which is not desired, of course, and, although delegation to indexOf(Object) in case of null as passed value is counterintuitive, it will preserve the current state of things. > See > [this](https://github.com/search?q=%22indexOf%28null%29%22+language%3AJava+=code) > and > [that](https://github.com/search?q=%22lastIndexOf%28null%29%22+language%3AJava+=code) > query. Thats a valid point, haven`t though about qurying where null is valid value. - PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2046174903
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Wed, 10 Apr 2024 11:20:32 GMT, xxDark wrote: > Should be `findIndex`? Similarly with `findLastIndex`. Yeah i guess IDE didnt do a great job with refactor. I will review aall changes manually a bit later - PR Review Comment: https://git.openjdk.org/jdk/pull/18639#discussion_r1559373902
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Tue, 9 Apr 2024 20:48:19 GMT, Chen Liang wrote: > A few remarks: > > 1. The concern on `indexOf(Object)` and `indexOf(Predicate)` is valid; > `Optional` has `orElse` vs. `orElseGet` to avoid these clashes. The behavior > change of `indexOf(null)` is a source incompatibility. > 2. Have you considered `indexOf(predicate/object, startIndex)` like the > indexOf for `String`? > > Also, a significant difference between String/arrays and `List` is that > `List` is structurally modifiable; the index may be no longer valid after a > modification. Thus the index is not as useful. > > For example, in an [unrolled linked > list](https://en.wikipedia.org/wiki/Unrolled_linked_list) implementation, > each block can be independently locked for read/write access, maybe like > this: `[1, 2, 3] -> [4, 5] -> [6, 7, 8, 9]` An index would be useless if any > part of this list updates, but if the position information is stored in a > `ListIterator` and we add an `beforeNext(predicate)` that moves the cursor to > right before the next element that matches the predicate (or the tail if > there's no match), this would be much more useful. The concern about change in behaviour of indexOf(null) is completely valid, although indexOf(null) is kind of exotic scenario. One possible workaround is to temporally (for an intermidiate period) or permanently delegate null value handing to indexOf(Object), which seems odd for me to me tbh, although it preserves source compatibility. Regarding to indexOf(predicate/object, startIndex), i thought about this, but firstly I would like to have a final and approved version of indexOf(predicate) to have a reference I`m certain in. As for beforeNext(Predicate) for ListIterator, I havent thought about that, but now that you`ve mentioned it, I could also consider implementing such thing. However, concern about possible index invalidation is more applicable to the nature of indexOf itself rather then this proposal. This method is primarly used when index is needed right here and now or passed to operation that is not supposed to modify list. I guess ListIterator.beforeNext and List.indexOf serve different, altough alike purpose, and therefore, in my opinion, there are place for both in Java - PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2046103546
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Tue, 9 Apr 2024 21:48:23 GMT, Evemose wrote: > One possible workaround is to temporally (for an intermidiate period) or > permanently delegate null value handing to indexOf(Object), which seems odd > for me to me tbh, although it preserves source compatibility. This does not solve the problem though. Source compatibility would still be broken as one would still need to add casts everywhere, or am I misinterpreting something? > he concern about change in behaviour of indexOf(null) is completely valid, > although indexOf(null) is kind of exotic scenario. See [this](https://github.com/search?q=%22indexOf%28null%29%22+language%3AJava+=code) and [that](https://github.com/search?q=%22lastIndexOf%28null%29%22+language%3AJava+=code) query. - PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2046118678
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Fri, 5 Apr 2024 00:00:58 GMT, Evemose wrote:
> **Subject**
> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
> `java.util.List`
>
> **Motivation**
> The motivation behind this proposal is to enhance the functionality of the
> `List` interface by providing a more flexible way to find the index of an
> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
> object as a parameter. This limits the flexibility of these methods as they
> can only find the index of exact object matches.
>
> The proposed methods would accept a `Predicate` as a parameter, allowing
> users to define a condition that the desired element must meet. This would
> provide a more flexible and powerful way to find the index of an element in a
> list.
>
> Here is a brief overview of the changes made in this pull request:
>
> 1. Added the `indexOf(Predicate filter)` method to the `List`
> interface.
> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
> interface.
> 3. Implemented these methods in all non-abstract classes that implement the
> `List` interface.
>
> The changes have been thoroughly tested to ensure they work as expected and
> do not introduce any regressions. The test cases cover a variety of scenarios
> to ensure the robustness of the implementation.
>
> For example, consider the following test case:
>
> List list = new ArrayList<>();
> list.add("Object one");
> list.add("NotObject two");
> list.add("NotObject three");
>
> int index1 = list.indexOf(s -> s.contains("ct t"));
> System.out.println(index1); // Expected output: 1
> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
> System.out.println(index2); // Expected output: 2
>
>
> Currently, to achieve the same result, we would have to use a more verbose
> approach:
>
> int index1 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).contains("ct t"))
> .findFirst()
> .orElse(-1);
> System.out.println(index1); // Output: 1
> int index2 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).startsWith("NotObject"))
> .reduce((first, second) -> second)
> .orElse(-1);
> System.out.println(index2); // Output: 2
>
>
> I believe these additions would greatly enhance the functionality and
> flexibility of the `List` interface, making it more powerful and
> user-friendly. I look forward to your feedback and am open to making any
> necessary changes based on your suggestions.
>
> Thank you for considering this proposal.
>
> Best regards
A few remarks:
1. The concern on `indexOf(Object)` and `indexOf(Predicate)` is valid;
`Optional` has `orElse` vs. `orElseGet` to avoid these clashes. The behavior
change of `indexOf(null)` is a source incompatibility.
2. Have you considered `indexOf(predicate/object, startIndex)` like the indexOf
for `String`?
Also, a significant difference between String/arrays and `List` is that `List`
is structurally modifiable; the index may be no longer valid after a
modification. Thus the index is not as useful.
For example, in an [unrolled linked
list](https://en.wikipedia.org/wiki/Unrolled_linked_list) implementation, each
block can be independently locked for read/write access, maybe like this:
`[1, 2, 3] -> [4, 5] -> [6, 7, 8, 9]`
An index would be useless if any part of this list updates, but if the position
information is stored in a `ListIterator` and we add an `beforeNext(predicate)`
that moves the cursor to right before the next element that matches the
predicate (or the tail if there's no match), this would be much more useful.
Just a side note, this patch won't be reviewed by official JDK engineers until
it has rfr tag (means this PR is not draft and the automatic checks of issue,
whitespace, oca etc. are passed)
You should post on `core-libs-dev@openjdk.org` to get feedbacks on the API
design, too.
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2046023822
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2047602713
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Mon, 8 Apr 2024 03:57:33 GMT, ExE Boss wrote:
>> **Subject**
>> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
>> `java.util.List`
>>
>> **Motivation**
>> The motivation behind this proposal is to enhance the functionality of the
>> `List` interface by providing a more flexible way to find the index of an
>> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
>> object as a parameter. This limits the flexibility of these methods as they
>> can only find the index of exact object matches.
>>
>> The proposed methods would accept a `Predicate` as a parameter, allowing
>> users to define a condition that the desired element must meet. This would
>> provide a more flexible and powerful way to find the index of an element in
>> a list.
>>
>> Here is a brief overview of the changes made in this pull request:
>>
>> 1. Added the `indexOf(Predicate filter)` method to the `List`
>> interface.
>> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
>> interface.
>> 3. Implemented these methods in all non-abstract classes that implement the
>> `List` interface.
>>
>> The changes have been thoroughly tested to ensure they work as expected and
>> do not introduce any regressions. The test cases cover a variety of
>> scenarios to ensure the robustness of the implementation.
>>
>> For example, consider the following test case:
>>
>> List list = new ArrayList<>();
>> list.add("Object one");
>> list.add("NotObject two");
>> list.add("NotObject three");
>>
>> int index1 = list.indexOf(s -> s.contains("ct t"));
>> System.out.println(index1); // Expected output: 1
>> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
>> System.out.println(index2); // Expected output: 2
>>
>>
>> Currently, to achieve the same result, we would have to use a more verbose
>> approach:
>>
>> int index1 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).contains("ct t"))
>> .findFirst()
>> .orElse(-1);
>> System.out.println(index1); // Output: 1
>> int index2 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).startsWith("NotObject"))
>> .reduce((first, second) -> second)
>> .orElse(-1);
>> System.out.println(index2); // Output: 2
>>
>>
>> I believe these additions would greatly enhance the functionality and
>> flexibility of the `List` interface, making it more powerful and
>> user-friendly. I look forward to your feedback and am open to making any
>> necessary changes bas...
>
> Newly added methods to existing widely‑implemented non‑sealed and non‑preview
> interfaces in `java.base` must be `default` to avoid source incompatibilities
> and runtime errors, so move the implementations from `AbstractList` to `List`:
@ExE-Boss Commited changes. Also, i see you have added not-null assertion at
the beginning of methods inside List. If this is a desired behaviour, I could
also add this to other impls
@ExE-Boss removed unused import
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2042094028
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2042910322
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Fri, 5 Apr 2024 00:31:22 GMT, Evemose wrote:
>> **Subject**
>> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
>> `java.util.List`
>>
>> **Motivation**
>> The motivation behind this proposal is to enhance the functionality of the
>> `List` interface by providing a more flexible way to find the index of an
>> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
>> object as a parameter. This limits the flexibility of these methods as they
>> can only find the index of exact object matches.
>>
>> The proposed methods would accept a `Predicate` as a parameter, allowing
>> users to define a condition that the desired element must meet. This would
>> provide a more flexible and powerful way to find the index of an element in
>> a list.
>>
>> Here is a brief overview of the changes made in this pull request:
>>
>> 1. Added the `indexOf(Predicate filter)` method to the `List`
>> interface.
>> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
>> interface.
>> 3. Implemented these methods in all non-abstract classes that implement the
>> `List` interface.
>>
>> The changes have been thoroughly tested to ensure they work as expected and
>> do not introduce any regressions. The test cases cover a variety of
>> scenarios to ensure the robustness of the implementation.
>>
>> For example, consider the following test case:
>>
>> List list = new ArrayList<>();
>> list.add("Object one");
>> list.add("NotObject two");
>> list.add("NotObject three");
>>
>> int index1 = list.indexOf(s -> s.contains("ct t"));
>> System.out.println(index1); // Expected output: 1
>> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
>> System.out.println(index2); // Expected output: 2
>>
>>
>> Currently, to achieve the same result, we would have to use a more verbose
>> approach:
>>
>> int index1 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).contains("ct t"))
>> .findFirst()
>> .orElse(-1);
>> System.out.println(index1); // Output: 1
>> int index2 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).startsWith("NotObject"))
>> .reduce((first, second) -> second)
>> .orElse(-1);
>> System.out.println(index2); // Output: 2
>>
>>
>> I believe these additions would greatly enhance the functionality and
>> flexibility of the `List` interface, making it more powerful and
>> user-friendly. I look forward to your feedback and am open to making any
>> necessary changes bas...
>
> side note: looks like this code have been reformated and some unused imports
> has been reformated. jdk compiles and works just fine, so i guess its not a
> big deal
> Hello @Evemose, if we add an API we should also consider if implementation
> can efficiently implement it. How is this new API better than using a
> ListIterator directly so it's worth the additional methods?
Hi, @liach ! The implementations i provided in this pull requests are pretty
much imitating behaviour of indexOf(Object). Every class that has overriden
indexOf(Object) recieved implementation of indexOf(Predicate) that works the
same way except for search is based on predicate and not equality. Therefore,
the effectiveness of implementation is straight up depends on effectiveness of
indexOf(Object) for each separate java.util.List impl.
As for ListIterator, where suitable, it has already been used in
implementations in classes where it was considered suitable for indexOf(Object)
(to be more precise, in AbstractList). The main goal of this pull requst is to
provide shorthand for developers as search for index of element that match
predicate is very common task and providing way to do it as comfortable as
possible should vastly enhance development expirience.
For ListIterator specifically: the main downside of its direct usage for
developer is inability to use it fluently (as method call argument, for
example), which forces to either implement method to find index separately or
find it in same method as invocation but before invocation explicitly. Both
cases are pretty frustraiting and, in my opinion, shouldn`t be neccessary for
high-level language like Java.
Summarizing, its not that in current stdlib there are no ways to search for
index of element based on condition, but this addition would be a great way to
make Java development more comfortable and fast.
PS:
Also implementing indexOf with predicate is a common practice for many high
level languages: C#/.NET has FindIndex, Kotlin has indexOfFirst, Swift has
firstIndex etc.
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2040025647
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Fri, 5 Apr 2024 00:00:58 GMT, Evemose wrote:
> **Subject**
> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
> `java.util.List`
>
> **Motivation**
> The motivation behind this proposal is to enhance the functionality of the
> `List` interface by providing a more flexible way to find the index of an
> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
> object as a parameter. This limits the flexibility of these methods as they
> can only find the index of exact object matches.
>
> The proposed methods would accept a `Predicate` as a parameter, allowing
> users to define a condition that the desired element must meet. This would
> provide a more flexible and powerful way to find the index of an element in a
> list.
>
> Here is a brief overview of the changes made in this pull request:
>
> 1. Added the `indexOf(Predicate filter)` method to the `List`
> interface.
> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
> interface.
> 3. Implemented these methods in all non-abstract classes that implement the
> `List` interface.
>
> The changes have been thoroughly tested to ensure they work as expected and
> do not introduce any regressions. The test cases cover a variety of scenarios
> to ensure the robustness of the implementation.
>
> For example, consider the following test case:
>
> List list = new ArrayList<>();
> list.add("Object one");
> list.add("NotObject two");
> list.add("NotObject three");
>
> int index1 = list.indexOf(s -> s.contains("ct t"));
> System.out.println(index1); // Expected output: 1
> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
> System.out.println(index2); // Expected output: 2
>
>
> Currently, to achieve the same result, we would have to use a more verbose
> approach:
>
> int index1 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).contains("ct t"))
> .findFirst()
> .orElse(-1);
> System.out.println(index1); // Output: 1
> int index2 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).startsWith("NotObject"))
> .reduce((first, second) -> second)
> .orElse(-1);
> System.out.println(index2); // Output: 2
>
>
> I believe these additions would greatly enhance the functionality and
> flexibility of the `List` interface, making it more powerful and
> user-friendly. I look forward to your feedback and am open to making any
> necessary changes based on your suggestions.
>
> Thank you for considering this proposal.
>
> Best regards
Won't this make some calls ambiguous?
For example, passing `null`:
List l;
l.indexOf(""); // Fine
l.indexOf(null); // Now fails to compile
Edit: I'm not a reviewer, but I decided to point this out.
src/java.base/share/classes/java/util/LinkedList.java line 1519:
> 1517:
> 1518: public int findIndex(Predicate filter) {
> 1519: return rlist.indexOf(filter);
Should be `findIndex`? Similarly with `findLastIndex`.
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2045324794
PR Review Comment: https://git.openjdk.org/jdk/pull/18639#discussion_r1559268478
RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
**Subject**
Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
`java.util.List`
**Motivation**
The motivation behind this proposal is to enhance the functionality of the
`List` interface by providing a more flexible way to find the index of an
element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
object as a parameter. This limits the flexibility of these methods as they can
only find the index of exact object matches.
The proposed methods would accept a `Predicate` as a parameter, allowing users
to define a condition that the desired element must meet. This would provide a
more flexible and powerful way to find the index of an element in a list.
Here is a brief overview of the changes made in this pull request:
1. Added the `indexOf(Predicate filter)` method to the `List`
interface.
2. Added the `lastIndexOf(Predicate filter)` method to the `List`
interface.
3. Implemented these methods in all non-abstract classes that implement the
`List` interface.
The changes have been thoroughly tested to ensure they work as expected and do
not introduce any regressions. The test cases cover a variety of scenarios to
ensure the robustness of the implementation.
For example, consider the following test case:
List list = new ArrayList<>();
list.add("Object one");
list.add("NotObject two");
list.add("NotObject three");
int index1 = list.indexOf(s -> s.contains("ct t"));
System.out.println(index1); // Expected output: 1
int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
System.out.println(index2); // Expected output: 2
Currently, to achieve the same result, we would have to use a more verbose
approach:
int index1 = IntStream.range(0, list.size())
.filter(i -> list.get(i).contains("ct t"))
.findFirst()
.orElse(-1);
System.out.println(index1); // Output: 1
int index2 = IntStream.range(0, list.size())
.filter(i -> list.get(i).startsWith("NotObject"))
.reduce((first, second) -> second)
.orElse(-1);
System.out.println(index2); // Output: 2
I believe these additions would greatly enhance the functionality and
flexibility of the `List` interface, making it more powerful and user-friendly.
I look forward to your feedback and am open to making any necessary changes
based on your suggestions.
Thank you for considering this proposal.
Best regards
-
Commit messages:
- did some renaming of internal methods utilized by findIndex
- fixed ReverseOrderLinkedListView
- fixed javadocs in List and ArrayList
- Merge remote-tracking branch 'origin/feature/indexOf-with-predicate' into
feature/indexOf-with-predicate
- Removed unused import in AbstractList
- Also moved to List interface
- Moved from AbstractList
- Removed implementation from AbstractList, moving it to List
- Renamed indexOf(Predicate) and lastIndexOf(Predicate) to
findIndex(Predicate) and findLastIndex(Predicate) respectively
- Merge remote-tracking branch 'origin/feature/indexOf-with-predicate' into
feature/indexOf-with-predicate
- ... and 25 more: https://git.openjdk.org/jdk/compare/16576b87...025bcf39
Changes: https://git.openjdk.org/jdk/pull/18639/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18639=00
Issue: https://bugs.openjdk.org/browse/JDK-8329760
Stats: 882 lines in 13 files changed: 570 ins; 47 del; 265 mod
Patch: https://git.openjdk.org/jdk/pull/18639.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18639/head:pull/18639
PR: https://git.openjdk.org/jdk/pull/18639
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Fri, 5 Apr 2024 00:00:58 GMT, Evemose wrote:
> **Subject**
> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
> `java.util.List`
>
> **Motivation**
> The motivation behind this proposal is to enhance the functionality of the
> `List` interface by providing a more flexible way to find the index of an
> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
> object as a parameter. This limits the flexibility of these methods as they
> can only find the index of exact object matches.
>
> The proposed methods would accept a `Predicate` as a parameter, allowing
> users to define a condition that the desired element must meet. This would
> provide a more flexible and powerful way to find the index of an element in a
> list.
>
> Here is a brief overview of the changes made in this pull request:
>
> 1. Added the `indexOf(Predicate filter)` method to the `List`
> interface.
> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
> interface.
> 3. Implemented these methods in all non-abstract classes that implement the
> `List` interface.
>
> The changes have been thoroughly tested to ensure they work as expected and
> do not introduce any regressions. The test cases cover a variety of scenarios
> to ensure the robustness of the implementation.
>
> For example, consider the following test case:
>
> List list = new ArrayList<>();
> list.add("Object one");
> list.add("NotObject two");
> list.add("NotObject three");
>
> int index1 = list.indexOf(s -> s.contains("ct t"));
> System.out.println(index1); // Expected output: 1
> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
> System.out.println(index2); // Expected output: 2
>
>
> Currently, to achieve the same result, we would have to use a more verbose
> approach:
>
> int index1 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).contains("ct t"))
> .findFirst()
> .orElse(-1);
> System.out.println(index1); // Output: 1
> int index2 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).startsWith("NotObject"))
> .reduce((first, second) -> second)
> .orElse(-1);
> System.out.println(index2); // Output: 2
>
>
> I believe these additions would greatly enhance the functionality and
> flexibility of the `List` interface, making it more powerful and
> user-friendly. I look forward to your feedback and am open to making any
> necessary changes based on your suggestions.
>
> Thank you for considering this proposal.
>
> Best regards
currently awaiting for initial issue triage to complete
side note: looks like this code have been reformated and some unused imports
has been reformated. jdk compiles and works just fine, so i guess its not a big
deal
> Won't this make some calls ambiguous? For example, passing `null`:
>
> ```java
> List l;
> l.indexOf(""); // Fine
> l.indexOf(null); // Now fails to compile
> ```
>
> Edit: I'm not a reviewer, but I decided to point this out.
I`ve tested it. Now calling indexOf with null as literal calls
indexOf(Predicate) instead, as Predicate is lower in class hierarchy than
Object (ofc it does not apply to varibles that store null, those works just as
usual)
If you want to call indexOf(Object) instead, you will have to cast null to
desired type manually
> ```java
> List l;
> l.indexOf(""); // Fine
> l.indexOf((String)null); // Will invoke indexOf(Object)
> ```
Question to reviewers: what do you think would be the best way to handle issue
discussed above?
PS: converted PR to draft untill the issue is resolved
PPS: The idea I came up with is to possibly rename indexOf(Predicate) to
something like findIndex(Predicate) or indexOfMatching(Predicate) as a way to
both avoid unexpected behaviour and preserve source compatibility
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2038490998
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2038506989
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2045522079
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2046196342
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Fri, 5 Apr 2024 00:00:58 GMT, Evemose wrote:
> **Subject**
> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
> `java.util.List`
>
> **Motivation**
> The motivation behind this proposal is to enhance the functionality of the
> `List` interface by providing a more flexible way to find the index of an
> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
> object as a parameter. This limits the flexibility of these methods as they
> can only find the index of exact object matches.
>
> The proposed methods would accept a `Predicate` as a parameter, allowing
> users to define a condition that the desired element must meet. This would
> provide a more flexible and powerful way to find the index of an element in a
> list.
>
> Here is a brief overview of the changes made in this pull request:
>
> 1. Added the `indexOf(Predicate filter)` method to the `List`
> interface.
> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
> interface.
> 3. Implemented these methods in all non-abstract classes that implement the
> `List` interface.
>
> The changes have been thoroughly tested to ensure they work as expected and
> do not introduce any regressions. The test cases cover a variety of scenarios
> to ensure the robustness of the implementation.
>
> For example, consider the following test case:
>
> List list = new ArrayList<>();
> list.add("Object one");
> list.add("NotObject two");
> list.add("NotObject three");
>
> int index1 = list.indexOf(s -> s.contains("ct t"));
> System.out.println(index1); // Expected output: 1
> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
> System.out.println(index2); // Expected output: 2
>
>
> Currently, to achieve the same result, we would have to use a more verbose
> approach:
>
> int index1 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).contains("ct t"))
> .findFirst()
> .orElse(-1);
> System.out.println(index1); // Output: 1
> int index2 = IntStream.range(0, list.size())
> .filter(i -> list.get(i).startsWith("NotObject"))
> .reduce((first, second) -> second)
> .orElse(-1);
> System.out.println(index2); // Output: 2
>
>
> I believe these additions would greatly enhance the functionality and
> flexibility of the `List` interface, making it more powerful and
> user-friendly. I look forward to your feedback and am open to making any
> necessary changes based on your suggestions.
>
> Thank you for considering this proposal.
>
> Best regards
Newly added methods to existing widely‑implemented non‑sealed and non‑preview
interfaces in `java.base` must be `default` to avoid source incompatibilities
and runtime errors, so move the implementations from `AbstractList` to `List`:
I’m in favour of using `findIndex`, which also avoids the issue with
`List>::indexOf` being ambiguous and having binary incompatible
bridge methods.
src/java.base/share/classes/java/util/AbstractList.java line 29:
> 27:
> 28: import java.util.function.Consumer;
> 29: import java.util.function.Predicate;
This is now unused:
Suggestion:
src/java.base/share/classes/java/util/AbstractList.java line 221:
> 219: return -1;
> 220: }
> 221:
Suggestion:
src/java.base/share/classes/java/util/AbstractList.java line 269:
> 267: return -1;
> 268: }
> 269:
Suggestion:
src/java.base/share/classes/java/util/List.java line 702:
> 700: * ( href="Collection.html#optional-restrictions">optional)
> 701: */
> 702: int lastIndexOf(Predicate filter);
Suggestion:
/**
* Returns the index of the first occurrence of matching element
* in this list, or -1 if this list does not contain the element.
* More formally, returns the lowest index {@code i} such that
* {@code filter.test(get(i))},
* or -1 if there is no such index.
*
* @implSpec
* This implementation first gets a list iterator (with
* {@code listIterator()}). Then, it iterates over the list until a
* matching element is found or the beginning of the list is reached.
*
* @param filter a predicate to search for
* @return the index of the first occurrence of the specified element in
* this list, or -1 if this list does not contain the element
* @throws NullPointerException if passed filter is null
*/
default int indexOf(Predicate filter) {
Objects.requireNonNull(filter);
ListIterator it = listIterator();
while (it.hasNext()) {
E e = it.next();
if (filter.test(e)) {
return it.previousIndex();
}
}
return -1;
}
/**
* Returns the index of the last occurrence of matching element
* in this list, or -1 if this list does not contain the element.
* More formally, returns the
Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface
On Fri, 5 Apr 2024 00:31:22 GMT, Evemose wrote:
>> **Subject**
>> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to
>> `java.util.List`
>>
>> **Motivation**
>> The motivation behind this proposal is to enhance the functionality of the
>> `List` interface by providing a more flexible way to find the index of an
>> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an
>> object as a parameter. This limits the flexibility of these methods as they
>> can only find the index of exact object matches.
>>
>> The proposed methods would accept a `Predicate` as a parameter, allowing
>> users to define a condition that the desired element must meet. This would
>> provide a more flexible and powerful way to find the index of an element in
>> a list.
>>
>> Here is a brief overview of the changes made in this pull request:
>>
>> 1. Added the `indexOf(Predicate filter)` method to the `List`
>> interface.
>> 2. Added the `lastIndexOf(Predicate filter)` method to the `List`
>> interface.
>> 3. Implemented these methods in all non-abstract classes that implement the
>> `List` interface.
>>
>> The changes have been thoroughly tested to ensure they work as expected and
>> do not introduce any regressions. The test cases cover a variety of
>> scenarios to ensure the robustness of the implementation.
>>
>> For example, consider the following test case:
>>
>> List list = new ArrayList<>();
>> list.add("Object one");
>> list.add("NotObject two");
>> list.add("NotObject three");
>>
>> int index1 = list.indexOf(s -> s.contains("ct t"));
>> System.out.println(index1); // Expected output: 1
>> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
>> System.out.println(index2); // Expected output: 2
>>
>>
>> Currently, to achieve the same result, we would have to use a more verbose
>> approach:
>>
>> int index1 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).contains("ct t"))
>> .findFirst()
>> .orElse(-1);
>> System.out.println(index1); // Output: 1
>> int index2 = IntStream.range(0, list.size())
>> .filter(i -> list.get(i).startsWith("NotObject"))
>> .reduce((first, second) -> second)
>> .orElse(-1);
>> System.out.println(index2); // Output: 2
>>
>>
>> I believe these additions would greatly enhance the functionality and
>> flexibility of the `List` interface, making it more powerful and
>> user-friendly. I look forward to your feedback and am open to making any
>> necessary changes bas...
>
> side note: looks like this code have been reformated and some unused imports
> has been reformated. jdk compiles and works just fine, so i guess its not a
> big deal
Hello @Evemose, if we add an API we should also consider if implementation can
efficiently implement it. How is this new API better than using a ListIterator
directly so it's worth the additional methods?
-
PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2039920614
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v6]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains seven commits:
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18527=05
Stats: 488 lines in 61 files changed: 389 ins; 3 del; 96 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: Vector (and integer) API: unsigned min/max
On Apr 17, 2024, at 7:14 AM, David Lloyd wrote: > > 2. Add .MIN_VALUE, min/max with a value or vector also offset by > .MIN_VALUE, and then subtract the offset again I think that’s the path of least resistance. It’s just a vxor on each operand, with a constant mask. That can be done in Java code. CPU’s that implement native unsigned cmp can peephole optimize.
Re: RFR: 8330542: Add two sample configuration files in preparation for a more secure by default configuration
On Wed, 17 Apr 2024 23:24:06 GMT, Joe Wang wrote: > Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > > jaxp-compat.properties: used to regain compatibility from any more > restricted configuration than previous versions such as JDK 22 Thanks Alan, Lance! I added description of the three config files to both files so that readers can find answers reading any one of them. Also added a release note: https://bugs.openjdk.org/browse/JDK-8330605 - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2065384541
Re: RFR: 8330542: Add two sample configuration files in preparation for a more secure by default configuration [v2]
> Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > > jaxp-compat.properties: used to regain compatibility from any more > restricted configuration than previous versions such as JDK 22 Joe Wang has updated the pull request incrementally with one additional commit since the last revision: add description of the three configuration files. - Changes: - all: https://git.openjdk.org/jdk/pull/18831/files - new: https://git.openjdk.org/jdk/pull/18831/files/e36e5fd4..98fcc3ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18831=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18831=00-01 Stats: 44 lines in 2 files changed: 38 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18831.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18831/head:pull/18831 PR: https://git.openjdk.org/jdk/pull/18831
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v5]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
update test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/f3670e7a..8ad8b818
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=04
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=03-04
Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote: > Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. Should the copyright be updated? - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2065339389
Re: RFR: 8295111: dpkg appears to have problems resolving symbolically linked native libraries
On Thu, 18 Apr 2024 19:56:43 GMT, Alexey Semenyuk wrote: > Pass a path with resolved symlinks to `dpkg -S` command. If it fails, try the > original path if they differ. > > Testing on Ubuntu 24.04 passed. Successfully created a .deb package from > SwingSet2.jar with the not-empty list of dependencies. Before the fix the > list of required packages was empty. In jpackage log: > > > [19:30:48.784] Running dpkg > [19:30:49.118] Command [PID: 244104]: > dpkg -S /usr/lib/x86_64-linux-gnu/libm.so.6 > [19:30:49.118] Output: > libc6:amd64: /usr/lib/x86_64-linux-gnu/libm.so.6 > [19:30:49.118] Returned: 0 > > [19:30:49.118] /lib/x86_64-linux-gnu/libm.so.6 is provided by [libc6] > > > It was looking for a package providing "/lib/x86_64-linux-gnu/libm.so.6" > library, but the actual argument to "dpgk -S" command was > "/usr/lib/x86_64-linux-gnu/libm.so.6". Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18844#pullrequestreview-2009895564
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v4]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons 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 five additional
commits since the last revision:
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/3f745431..f3670e7a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=03
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=02-03
Stats: 42074 lines in 1058 files changed: 18282 ins; 15937 del; 7855 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
Please review a set of updates to clean up use of `/**` comments in the vicinity of declarations. There are various categories of update: * "Box comments" beginning with `/**` * Misplaced doc comments before package or import statements * Misplaced doc comments after the annotations for a declaration * Dangling `/**` comments relating to a group of declarations, separate from the doc comments for each of those declarations * Use of `/**` for the legal header at or near the top of the file In one case, two doc comments before a declaration were merged, which fixes a bug/omission in the documented serialized form. - Commit messages: - JDK-8330178: Clean up non-standard use of /** comments in `java.base` Changes: https://git.openjdk.org/jdk/pull/18846/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18846=00 Issue: https://bugs.openjdk.org/browse/JDK-8330178 Stats: 134 lines in 23 files changed: 50 ins; 56 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/18846.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18846/head:pull/18846 PR: https://git.openjdk.org/jdk/pull/18846
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Thu, 18 Apr 2024 07:34:24 GMT, Jan Lahoda wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > My personal comments here: > - I am fine with a solution like this. In 18753, I wanted to avoid a change > of dynamics between the Java helper and the native part. But if we can change > that, it looks better. I would suggest to take the test from 18753 though - > doing a change like this without a test may lead to hard-to-find regressions > in the future. (Note the current test should guard against both JDK-8329420 > and JDK-8329581.) Or write a different test. > - as Mandy points out, `LaucherHelper` already reads/has variables for > "is-static" and "no-arguments" in `validateMainMethod`, so it should be > possible to just use that values; also as Mandy points out, we can probably > get rid of `CHECK_EXCEPTION_FAIL` and `CHECK_EXCEPTION_NULL_FAIL`, and use > the `..._LEAVE` variants, no? (The `..._FAIL` variants where needed so that > the launcher could continue with the next variant, but since we now only call > the correct variant, we can just stop if something goes wrong?) @lahodaj > I would suggest to take the test from 18753 though - doing a change like this > without a test may lead to hard-to-find regressions in the future. (Note the > current test should guard against both JDK-8329420 and JDK-8329581.) Agreed. I’ll add the test case if this PR proceeds (see my comment above). > as Mandy points out, `LaucherHelper` already reads/has variables for > "is-static" and "no-arguments" in `validateMainMethod`, so it should be > possible to just use that values; Just to clarify, this would still mean converting “isStatic” and “noArgs” from local variables to fields so I am able to read them on the C side of things. Did I understand this correctly? > also as Mandy points out, we can probably get rid of `CHECK_EXCEPTION_FAIL` > and `CHECK_EXCEPTION_NULL_FAIL`, and use the `..._LEAVE` variants, no? (The > `..._FAIL` variants where needed so that the launcher could continue with the > next variant, but since we now only call the correct variant, we can just > stop if something goes wrong?) Agreed, I’ve updated that on my side of things. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2065283183
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles wrote: > Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia hi all, thanks for the comments! Happy to make the updates based on the feedback. However, could someone please advice if we are proceeding with this PR or https://github.com/openjdk/jdk/pull/18753? As @lahodaj noted, theirs avoids changing the dynamics between the Java helper and native code, so I just want to make sure we are on the same page. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2065279462
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v8]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: remove unecessary blank line - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/17b0be7b..f7859fc4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18542=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18542=06-07 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v7]
On Thu, 18 Apr 2024 18:59:20 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > IntelliJ trying to help Marked as reviewed by egahlin (Reviewer). src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 46: > 44: import jdk.jfr.events.FileWriteEvent; > 45: import jdk.jfr.events.InitialSecurityPropertyEvent; > 46: Unnecessary blank line added. - PR Review: https://git.openjdk.org/jdk/pull/18542#pullrequestreview-2009830262 PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1571335968
RFR: 8330595: Invoke ObjectMethods::bootstrap method exactly
Investigating a recent regression in mainline I realized we have an opportunity to improve the bootstrap overheads of ObjectMethods::bootstrap by using `invokeExact` in a way similar to what we already do for LMF and SCF BSMs. This avoids generating type checking adapters and equates to a one-off startup win of around 5ms in any app that ever has the need to spin up a toString, equals or hashCode method on a record. - Commit messages: - 8330595: Invoke ObjectMethods::bootstrap method exactly Changes: https://git.openjdk.org/jdk/pull/18845/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18845=00 Issue: https://bugs.openjdk.org/browse/JDK-8330595 Stats: 17 lines in 1 file changed: 16 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18845.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18845/head:pull/18845 PR: https://git.openjdk.org/jdk/pull/18845
Re: Vector (and integer) API: unsigned min/max
Presently, I'm implementing operations for a WASM emulator/runtime, so it's more a case of transitively acquiring use cases from WASM, though I'm also doing this specifically to practice accomplishing useful things with the vector API. Regarding saturating arithmetic: WASM has operations for that as well (both signed and unsigned), so on that basis alone it would be a welcome addition as far as I'm concerned. FWIW, another simple use case for saturating arithmetic is audio processing/mixing (when processing many channels at once) because saturation closely matches analog clipping, and also perhaps image compositing for similar reasons. On Thu, Apr 18, 2024 at 1:40 PM Paul Sandoz wrote: > Hi David, > > It’s not at all outlandish, but would caution it's more work than one > might initially think. > > Could you describe a little more about your use case? that can be helpful > to understand the larger picture and demand. Using unsigned comparison > would be my recommended approach with the current API. CC'ing Sandhya for > her opinion. > > Generally when we add new Vector operations we also consider the impact on > the scalar types and try to fill any gaps there, so the vector operation > behavior is composed from the scalar operation behavior (so like you > indicated regarding symmetry). > > We are seeing demand for saturated arithmetic primarily for vector (not > “vector” as in hardware vector) search, so we may do something there for > specific integral types. > > Paul. > > > On Apr 17, 2024, at 7:13 AM, David Lloyd wrote: > > > > I've been trying the the incubating Vector API and one thing I've missed > on integer-typed vectors is having unsigned min/max operations. There is a > signed min/max operation, and unsigned comparison, but no unsigned min/max. > > > > I guess this is for symmetry with `Math`, which only has signed > `min`/`max`. However, I would point out that while it's not very hard to > implement one's own unsigned min/max for integer types using > `compareUnsigned`, it is a bit harder to do so with vectors. The best I've > come up with is to take one of two approaches: > > > > 1. Zero-extend the vector to the next-larger size, perform the min/max, > and reduce it back down again, or > > 2. Add .MIN_VALUE, min/max with a value or vector also offset > by .MIN_VALUE, and then subtract the offset again > > > > I guess a third approach could be to do a comparison using unsigned > compares, and then use the resultant vector mask to select items from the > original two vectors, but I didn't bother to work out this solution given I > already had the other two options. > > > > Would it be feasible to add unsigned min/max operations for vectors? It > looks like at least AArch64 has support for this as a single instruction, > so it doesn't seem too outlandish. > > > > And as a separate (but related) question, what about > `Math.minUnsigned`/`Math.maxUnsigned` of `int` and `long` for symmetry? > > > > -- > > - DML • he/him > > -- - DML • he/him
Re: RFR: 8295111: dpkg appears to have problems resolving symbolically linked native libraries
On Thu, 18 Apr 2024 19:56:43 GMT, Alexey Semenyuk wrote: > Pass a path with resolved symlinks to `dpkg -S` command. If it fails, try the > original path if they differ. > > Testing on Ubuntu 24.04 passed. Successfully created a .deb package from > SwingSet2.jar with the not-empty list of dependencies. Before the fix the > list of required packages was empty. In jpackage log: > > > [19:30:48.784] Running dpkg > [19:30:49.118] Command [PID: 244104]: > dpkg -S /usr/lib/x86_64-linux-gnu/libm.so.6 > [19:30:49.118] Output: > libc6:amd64: /usr/lib/x86_64-linux-gnu/libm.so.6 > [19:30:49.118] Returned: 0 > > [19:30:49.118] /lib/x86_64-linux-gnu/libm.so.6 is provided by [libc6] > > > It was looking for a package providing "/lib/x86_64-linux-gnu/libm.so.6" > library, but the actual argument to "dpgk -S" command was > "/usr/lib/x86_64-linux-gnu/libm.so.6". @sashamatveev please review - PR Comment: https://git.openjdk.org/jdk/pull/18844#issuecomment-2065166526
RFR: 8295111: dpkg appears to have problems resolving symbolically linked native libraries
Pass a path with resolved symlinks to `dpkg -S` command. If it fails, try the original path if they differ. Testing on Ubuntu 24.04 passed. Successfully created a .deb package from SwingSet2.jar with the not-empty list of dependencies. Before the fix the list of required packages was empty. In jpackage log: [19:30:48.784] Running dpkg [19:30:49.118] Command [PID: 244104]: dpkg -S /usr/lib/x86_64-linux-gnu/libm.so.6 [19:30:49.118] Output: libc6:amd64: /usr/lib/x86_64-linux-gnu/libm.so.6 [19:30:49.118] Returned: 0 [19:30:49.118] /lib/x86_64-linux-gnu/libm.so.6 is provided by [libc6] It was looking for a package providing "/lib/x86_64-linux-gnu/libm.so.6" library, but the actual argument to "dpgk -S" command was "/usr/lib/x86_64-linux-gnu/libm.so.6". - Commit messages: - 8295111: dpkg appears to have problems resolving symbolically linked native libraries Changes: https://git.openjdk.org/jdk/pull/18844/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18844=00 Issue: https://bugs.openjdk.org/browse/JDK-8295111 Stats: 44 lines in 1 file changed: 19 ins; 3 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/18844.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18844/head:pull/18844 PR: https://git.openjdk.org/jdk/pull/18844
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v7]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: IntelliJ trying to help - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/28bf429d..17b0be7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18542=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18542=05-06 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v6]
On Thu, 18 Apr 2024 18:46:36 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > undo fix being handled in JDK-8329330. Prior to the last commit, this passed all tests tier 1-3 on all platform. It should be ready. - PR Comment: https://git.openjdk.org/jdk/pull/18542#issuecomment-2064929545
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v6]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: undo fix being handled in JDK-8329330. - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/366fca19..28bf429d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18542=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18542=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: Vector (and integer) API: unsigned min/max
Hi David, It’s not at all outlandish, but would caution it's more work than one might initially think. Could you describe a little more about your use case? that can be helpful to understand the larger picture and demand. Using unsigned comparison would be my recommended approach with the current API. CC'ing Sandhya for her opinion. Generally when we add new Vector operations we also consider the impact on the scalar types and try to fill any gaps there, so the vector operation behavior is composed from the scalar operation behavior (so like you indicated regarding symmetry). We are seeing demand for saturated arithmetic primarily for vector (not “vector” as in hardware vector) search, so we may do something there for specific integral types. Paul. > On Apr 17, 2024, at 7:13 AM, David Lloyd wrote: > > I've been trying the the incubating Vector API and one thing I've missed on > integer-typed vectors is having unsigned min/max operations. There is a > signed min/max operation, and unsigned comparison, but no unsigned min/max. > > I guess this is for symmetry with `Math`, which only has signed `min`/`max`. > However, I would point out that while it's not very hard to implement one's > own unsigned min/max for integer types using `compareUnsigned`, it is a bit > harder to do so with vectors. The best I've come up with is to take one of > two approaches: > > 1. Zero-extend the vector to the next-larger size, perform the min/max, and > reduce it back down again, or > 2. Add .MIN_VALUE, min/max with a value or vector also offset by > .MIN_VALUE, and then subtract the offset again > > I guess a third approach could be to do a comparison using unsigned compares, > and then use the resultant vector mask to select items from the original two > vectors, but I didn't bother to work out this solution given I already had > the other two options. > > Would it be feasible to add unsigned min/max operations for vectors? It looks > like at least AArch64 has support for this as a single instruction, so it > doesn't seem too outlandish. > > And as a separate (but related) question, what about > `Math.minUnsigned`/`Math.maxUnsigned` of `int` and `long` for symmetry? > > -- > - DML • he/him
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v21]
On Tue, 16 Apr 2024 00:04:15 GMT, Scott Gibbons wrote: >> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64. See >> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around >> this change. >> >> Overall, making this an intrinsic improves overall performance of >> `Unsafe::setMemory` by up to 4x for all buffer sizes. >> >> Tested with tier-1 (and full CI). I've added a table of the before and >> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`). >> >> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt) > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Add enter() and leave(); remove Windows-specific register stuff @vnkozlov Could you please review this PR from @asgibbons? Looking forward to your inputs. - PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2064852401
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type I'm OK with the minimal changes in the desktop code. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2009552015
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type We need a test for the new method, e.g. to check that the right exception is thrown, and the message is right. The fact that no test needed to be updated when you changed the exception type is a smell. - Changes requested by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2009500390
Re: RFR: 8328481: Implement Module Imports [v10]
On Thu, 18 Apr 2024 12:11:19 GMT, Jan Lahoda wrote: >> This is an implementation of JEP JDK-8315129: Module Import Declarations >> (Preview). Please see the JEP for details: >> https://bugs.openjdk.org/browse/JDK-8315129 >> >> It is mostly straightforward - the module imports are parsed, and then >> expanded to import-on-demand in `TypeEnter`. >> There is a few notable aspects, however: >> - the AST node for import (`JCImport`) is holding the imported element as a >> field access, because so far, the imported element always had to have a '.' >> (even for import-on-demand). But for module imports, it is permissible to >> import from a module whose name does not have a dot (`import module m;`). >> The use of field access for ordinary import seems very useful, so I >> preferred to keep that, and created a new internal-only AST node for module >> imports. There is still only one public API AST node/interface, so this is >> purely an implementation choice. >> - JShell now supports module imports as well; and the default, implicit, >> script is changed to use it to import all of `java.base` if preview is >> enabled. It is expected that the default would be changed if/when the module >> imports feature is finalized. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review feedback - improving the 'module (current) does not read > (target). src/jdk.compiler/share/classes/com/sun/source/tree/ImportTree.java line 57: > 55: */ > 56: @PreviewFeature(feature=PreviewFeature.Feature.MODULE_IMPORTS, > reflective=true) > 57: boolean isModule(); we are not including any test that stresses this new method - PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1571091780
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 18 Apr 2024 13:27:38 GMT, Jan Kratochvil wrote: > Could not we rename `is_containerized()` to `use_container_limit()` ? As that > is the current only purpose of `is_containerized()`. I'm not sure. There is value to have `is_containerized()` like it would behave after this patch. Specifically the first table row difference in [your comment](https://github.com/openjdk/jdk/pull/18201#issuecomment-2063868908) concerns me. JVMs running in a container without limit wouldn't be detected as "containerized". That seems a large share of deployments to miss. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2064487567
Integrated: 8330458: Add missing @since tag to ClassFile.JAVA_23_VERSION
On Wed, 17 Apr 2024 20:46:26 GMT, Joe Darcy wrote: > 8330458: Add missing @since tag to ClassFile.JAVA_23_VERSION This pull request has now been integrated. Changeset: 235ba9a7 Author:Joe Darcy URL: https://git.openjdk.org/jdk/commit/235ba9a7025964b1e43149c7102e26b82b2081ad Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8330458: Add missing @since tag to ClassFile.JAVA_23_VERSION Reviewed-by: jjg, iris, asotona - PR: https://git.openjdk.org/jdk/pull/18828
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 16 Apr 2024 10:30:14 GMT, Alan Bateman wrote: >> Thinking about this some more, would it not be possible to just use the >> mainMethod directly down in C? > > The changes JEP 463 went through many iterations, it was a fine balance of > avoiding too many transitions and upcalls, and at the same time, have > something that can be maintained. There are several JBS issues on this issue > now, probably because there just wasn't enough tests introduced with the JEP. FWIW, just ran into this as well. I was trying to do `System::loadLibrary` and had no clue why I could load non-existent libraries. Turns out I couldn't :-) but the error was never reported back to me. Confusing. - PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1570997544
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Thu, 18 Apr 2024 15:14:10 GMT, Tim Prinzing wrote: >> I think it might be the cause for >> https://bugs.openjdk.org/browse/JDK-8329330 I have sent out a PR to remove >> those separately so the fix can be backported. >> https://github.com/openjdk/jdk/pull/18843 > > That's correct, it is an unrelated cleanup (other than it seems to cause > tests to fail now). @egahlin, do you want me to remove those from this PR? Yes, it would be good if you could remove those changes, so it can be handled separately. - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570979943
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Thu, 18 Apr 2024 14:32:28 GMT, Erik Gahlin wrote: >> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: >> >>> 64: FileWriteEvent.class, >>> 65: SocketReadEvent.class, >>> 66: SocketWriteEvent.class, >> >> I'm guessing that this change which remove these two event classes is a >> drive-by-cleanup that should actually have been done with some previous fix >> in this area? >> Just wanted to double check it was intended as it doesn't seem to be related >> to file events. > > I think it might be the cause for https://bugs.openjdk.org/browse/JDK-8329330 > I have sent out a PR to remove those separately so the fix can be backported. > https://github.com/openjdk/jdk/pull/18843 That's correct, it is an unrelated cleanup (other than it seems to cause tests to fail now). @egahlin, do you want me to remove those from this PR? - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570953838
Re: RFR: 8330542: Add two sample configuration files
On Wed, 17 Apr 2024 23:24:06 GMT, Joe Wang wrote: > Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > > jaxp-compat.properties: used to regain compatibility from any more > restricted configuration than previous versions such as JDK 22 Hi Joe, Overall this looks fine though we need to clarify more as to the differences between jaxp-compat.properties vs jaxp.properties and also make it clearer why anyone would use jaxp.strict.properties As part of the review, I would suggest that a Release Note is created which will hopefully clarify when to use which file. Also when we actually change the defaults in a JDK release to be the equivalent of jaxp-strict.properties, that this file can be removed? If so this should be documented in the Release Note and perhaps a comment in the properties file itself src/java.xml/share/conf/jaxp-compat.properties line 10: > 8: # configuration, properties that have more restrictive settings as in the > 9: # strict configuration (jaxp-strict.properties) are reversed back to their > 10: # defaults. In particular: I think the above needs some more word smithing as we have not articulated what a strict configuration is or how this differs from jaxp.properties src/java.xml/share/conf/jaxp-compat.properties line 16: > 14: # > 15: # This configuration file can be used to reverse back to a working > environment > 16: # prior to any more restrictive configuration that may have been applied. How does this differ from jaxp.properties for JDK 23? I can understand for when we move to secure by default, we just need to be clear on the purpose of each jaxp properties files - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2009160577 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1570932404 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1570934511
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v6]
> This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Minor SimpleStringBuilderStrategy:: overhead reduction - Changes: - all: https://git.openjdk.org/jdk/pull/18690/files - new: https://git.openjdk.org/jdk/pull/18690/files/fd9c40d2..83f4048f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18690=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18690=04-05 Stats: 11 lines in 1 file changed: 1 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v5]
> This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Claes Redestad has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge master, resolve conflicts due pr/18688 - Dump using the hidden class name - Pre-size stringbuilders based on constant size and a small argument factor - @liach feedback - Bump threshold after experiments - Port ASM to CFA - Lower compilation overhead for large concat expressions, initial ASM-based version based on pre-existing implementation - Changes: https://git.openjdk.org/jdk/pull/18690/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18690=04 Stats: 237 lines in 2 files changed: 229 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 14:05:28 GMT, Daniel Fuchs wrote: >> Tim Prinzing has updated the pull request incrementally with one additional >> commit since the last revision: >> >> test file local to test > > src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: > >> 64: FileWriteEvent.class, >> 65: SocketReadEvent.class, >> 66: SocketWriteEvent.class, > > I'm guessing that this change which remove these two event classes is a > drive-by-cleanup that should actually have been done with some previous fix > in this area? > Just wanted to double check it was intended as it doesn't seem to be related > to file events. Yes, and I think it might be the cause for https://bugs.openjdk.org/browse/JDK-8329330 I will send out a PR to remove those separately (in 30 minutes) so a fix can be backported. - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570887596
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 01:34:07 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > test file local to test Sorry - just noticed this comment has been pending for a few days... src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: > 64: FileWriteEvent.class, > 65: SocketReadEvent.class, > 66: SocketWriteEvent.class, I'm guessing that this change which remove these two event classes is a drive-by-cleanup that should actually have been done with some previous fix in this area? Just wanted to double check it was intended as it doesn't seem to be related to file events. - PR Review: https://git.openjdk.org/jdk/pull/18542#pullrequestreview-2006152864 PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1568907662
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 01:34:07 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > test file local to test My comments have been addressed and I don't have any else to add. I assume @egahlin will cast an eye over this too. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18542#pullrequestreview-2008938324
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 11 Apr 2024 12:08:02 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf 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 ten additional > commits since the last revision: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - Fix tests > - Implement Metrics.isContainerized() > - Some clean-up > - Drop cgroups testing on plain Linux > - Implement fall-back logic for non-ro controller mounts > - Make find_ro static and local to compilation unit > - 8261242: [Linux] OSContainer::is_containerized() returns true Could not we rename `is_containerized()` to `use_container_limit()` ? As that is the current only purpose of `is_containerized()`. I did not test it but I expect the values will be: | your patch | my trivial patch | Actual deployment scenario | |||| | `true` | `false` |OpenJDK runs in an unprivileged container **without** a cpu/memory limit | | `true` | `true` | OpenJDK runs in an unprivileged container **with** a cpu/memory limit | | `false` | `false` | OpenJDK runs in a privileged container **without** a cpu/memory limit | | `true` | `true` | OpenJDK runs in a privileged container **with** a cpu/memory limit | | `false` | `false` | OpenJDK runs in a systemd slice **without** a cpu/memory limit | | `true` | `true` | OpenJDK runs in a systemd slice **with** a cpu/memory limit | | `false` | `false` | OpenJDK runs on a physical Linux system (VM or bare metal) | - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2063868908
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2008729407
Re: RFR: 8328481: Implement Module Imports [v10]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback - improving the 'module (current) does not read (target). - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/432393ab..846b038e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=08-09 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v9]
On Thu, 18 Apr 2024 10:47:11 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updating JEP number and caption.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> line 3541:
>
>> 3539: # 0: symbol, 1: symbol
>> 3540: compiler.err.import.module.does.not.read=\
>> 3541: {1} module does not read: {0}
>
> shouldn't it be `module {1} does not read: {0}` ? Also, maybe worth
> reordering the params?
Thanks! Fixed:
https://github.com/openjdk/jdk/pull/18614/commits/846b038ed8145ede9c419daddedb794a429dafac
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1570597292
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
> While `SymbolLookup` correctly uses an `Optional` return to denote whether a > symbol has been found by the lookup or not (which enables composition of > symbol lookups), many clients end up just calling `Optional::get`, or > `Optional::orElseThrow()` on the result. > > This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that > will do a lookup and, if no symbol can be found, throws an > `IllegalArgumentException` with a relevant exception message. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Change exception type - Changes: - all: https://git.openjdk.org/jdk/pull/18474/files - new: https://git.openjdk.org/jdk/pull/18474/files/2ebac9fc..e2f6c4c9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18474=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18474=04-05 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18474.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18474/head:pull/18474 PR: https://git.openjdk.org/jdk/pull/18474
Integrated: 8329997: Add provisions for checking memory segment alignment constraints
On Mon, 15 Apr 2024 07:50:24 GMT, Per Minborg wrote: > This PR proposes to add a new method `MemorySegment::maxByteAlignment` that > returns the maximum byte alignment of a segment (both heap and native > segments). > > Clients can then use this method to determine if a segment is properly > aligned for any given layout (e.g. following a `MemorySegment::reinterpret` > operation). This pull request has now been integrated. Changeset: b648ed0a Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/b648ed0a08b5ed47c1a7d7cbca89d8f389b17013 Stats: 135 lines in 4 files changed: 129 ins; 2 del; 4 mod 8329997: Add provisions for checking memory segment alignment constraints Reviewed-by: jvernee, mcimadamore - PR: https://git.openjdk.org/jdk/pull/18779
Re: RFR: 8328481: Implement Module Imports [v9]
On Thu, 18 Apr 2024 06:34:22 GMT, Jan Lahoda wrote:
>> This is an implementation of JEP JDK-8315129: Module Import Declarations
>> (Preview). Please see the JEP for details:
>> https://bugs.openjdk.org/browse/JDK-8315129
>>
>> It is mostly straightforward - the module imports are parsed, and then
>> expanded to import-on-demand in `TypeEnter`.
>> There is a few notable aspects, however:
>> - the AST node for import (`JCImport`) is holding the imported element as a
>> field access, because so far, the imported element always had to have a '.'
>> (even for import-on-demand). But for module imports, it is permissible to
>> import from a module whose name does not have a dot (`import module m;`).
>> The use of field access for ordinary import seems very useful, so I
>> preferred to keep that, and created a new internal-only AST node for module
>> imports. There is still only one public API AST node/interface, so this is
>> purely an implementation choice.
>> - JShell now supports module imports as well; and the default, implicit,
>> script is changed to use it to import all of `java.base` if preview is
>> enabled. It is expected that the default would be changed if/when the module
>> imports feature is finalized.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updating JEP number and caption.
Marked as reviewed by mcimadamore (Reviewer).
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
line 3541:
> 3539: # 0: symbol, 1: symbol
> 3540: compiler.err.import.module.does.not.read=\
> 3541: {1} module does not read: {0}
shouldn't it be `module {1} does not read: {0}` ? Also, maybe worth reordering
the params?
-
PR Review: https://git.openjdk.org/jdk/pull/18614#pullrequestreview-2008496026
PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1570474076
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Wed, 17 Apr 2024 01:07:04 GMT, Jan Kratochvil wrote: >>> IMHO `is_containerized()` is OK to return `false` even when running in a >>> container but with no limitations set. >> >> The idea here is to use this property to tune OpenJDK for in-container, >> specifically k8s, use. In such a setup it's custom to run a single process >> within set resource constraints. In order to do this, we need a reliable way >> to distinguish that vs. non-containerized setup. If somebody really wants to >> run OpenJDK in a container expecting it to run like a physical OpenJDK >> deployment, that's when `-XX:-UseContainerSupport` should be used. > >> The idea here is to use this property to tune OpenJDK for in-container, >> specifically k8s, use. In such a setup it's custom to run a single process >> within set resource constraints. > > The in-container tuning means to use all the available resources. Containers > in the real world have some memory limits set which is where my modified > patch still correctly identifies it as a container to use all the available > resources of the node which is the whole goal of the container detection code. > >> In order to do this, we need a reliable way to distinguish that vs. >> non-containerized setup. > > I expect it should have been written "We need a reliable way to distinguish > real world in-container vs. non-containerized setup. We do not mind behavior > for artificial containers on OpenJDK development machines.". Which is what my > patch does in an easier and less error-prone way. > >> If somebody really wants to run OpenJDK in a container expecting it to run >> like a physical OpenJDK deployment, that's when `-XX:-UseContainerSupport` >> should be used. > > That behaves still the same with my patch. > > Could you give a countercase where my patch behaves wrongly? @jankratochvil I believe this boils down to what we actually want. Should `OSContainer::is_containerized()` return `false` when run *inside* a container? If so, when is it OK to do that? Should `OSContainer::is_containerized()` return `true` on a physical Linux deployment? IMO, the read-only property of the mount points was something that fit naturally since, we already scan those anyway for (cgv1 vs cgv2 detection). Therefore it was something to consider to make heuristics more accurate. The truth table of the patch in this PR looks like this: | `OSContainer::is_containerized()` value | Actual deployment scenario | | - | - | | `true` | OpenJDK runs in an unprivileged container **without** a cpu/memory limit | | `true` | OpenJDK runs in an unprivileged container **with** a cpu/memory limit | | `true` | OpenJDK runs in a privileged container **with** a cpu/memory limit | | `false` | OpenJDK runs in a privileged container **without** a cpu/memory limit | | `false` | OpenJDK runs in a systemd slice **without** a cpu/memory limit | | `true` | OpenJDK runs in a systemd slice **with** a cpu/memory limit | | `false` | OpenJDK runs on a physical Linux system (VM or bare metal) | As you can see, the case of "OpenJDK runs in a privileged container *without* a cpu/memory limit" gives the wrong result. However, I consider this a fairly uncommon setup and there isn't really anything we can do to detect this kind of setup. Even if we did manage to detect it, why would we care? It's a question of probability. > Could you give a countercase where my patch behaves wrongly? Again, it's not a case of right or wrong IMO. Since we are in the land of heuristics, they will be wrong in some cases. We should make them so that we cover the common cases and, perhaps, are able to use those in serviceability tools to help guide diagnosis and/or further tuning. So far the existing capabilities were OK, but prevent further out-of-the-box tuning and/or accurate data collection. Your proposed patch (it's one I had in a previous iteration too, fwiw) would also return `false` for the case of "OpenJDK runs in an unprivileged container **without** a cpu/memory limit", which seems counterintuitive if OpenJDK actually runs in a container! What's more, it seems a fairly common case. Also, there is a chance of the OpenJDK heuristics to fail memory/cpu limit detection because of bugs and new developments. It seems the safer option to know that OpenJDK is containerized (using other heuristics) in that case. Maybe that's just me. Let's have that discussion more broadly and see if we can reach consensus! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2063477204
Re: RFR: 8330542: Add two sample configuration files
On Wed, 17 Apr 2024 23:24:06 GMT, Joe Wang wrote: > Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > > jaxp-compat.properties: used to regain compatibility from any more > restricted configuration than previous versions such as JDK 22 Just some context here. The JDK has been on a path for many releases to make XML processing more secure by default. At some point there will be a proposal/JEP to flip the switch, something that may create an upgrade challenge for some applications and deployments that haven't embraced the various security features and configuration added over the last 10+ years. The addition of conf/jaxp-strict.properties allows deployments to test more the secure/strict behavior in preparation for a possible future where strict is the default. As examples: trying this out may help identify processing XML that (perhaps unknowingly) makes outbound network connections to fetch DTD, or processing XML that relies on extension functions. So I think very useful to have this configuration available in a shipping JDK but it may need a write-up/JEP before anyone knows about this. - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2063474370
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles wrote: > Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia My personal comments here: - I am fine with a solution like this. In 18753, I wanted to avoid a change of dynamics between the Java helper and the native part. But if we can change that, it looks better. I would suggest to take the test from 18753 though - doing a change like this without a test may lead to hard-to-find regressions in the future. (Note the current test should guard against both JDK-8329420 and JDK-8329581.) Or write a different test. - as Mandy points out, `LaucherHelper` already reads/has variables for "is-static" and "no-arguments" in `validateMainMethod`, so it should be possible to just use that values; also as Mandy points out, we can probably get rid of `CHECK_EXCEPTION_FAIL` and `CHECK_EXCEPTION_NULL_FAIL`, and use the `..._LEAVE` variants, no? (The `..._FAIL` variants where needed so that the launcher could continue with the next variant, but since we now only call the correct variant, we can just stop if something goes wrong?) - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2063217552
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v3]
On Thu, 18 Apr 2024 06:31:03 GMT, Alan Bateman wrote: >> Hi Christoph, seems the USE_REGISTRY_LOOKUP is currently unused (at least >> without additional defines that are not present usually) >> >> src/java.base/windows/native/libjli/java_md.c:52:#ifdef USE_REGISTRY_LOOKUP >> src/java.base/windows/native/libjli/java_md.c:333:#ifdef USE_REGISTRY_LOOKUP >> >> >> I am not sure if this even works any more. Maybe Alan could comment on this >> ? > > @MBaesken I checked into the building with -DUSE_REGISTRY_LOOKUP as that > compiled in code that the Oracle installer needed when it copied java.exe to > somewhere. This is indeed left over code and can be removed from java_md.c. Hi Alan, thanks for checking this ! I removed the USE_REGISTRY_LOOKUP usages. - PR Review Comment: https://git.openjdk.org/jdk/pull/18699#discussion_r1570109527
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v3]
> We have already good JLI tracing capabilities. But GetApplicationHome and > GetApplicationHomeFromDll lack some tracing and should be enhanced. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: remove obsolete USE_REGISTRY_LOOKUP usages - Changes: - all: https://git.openjdk.org/jdk/pull/18699/files - new: https://git.openjdk.org/jdk/pull/18699/files/344d1b89..ddb3e0fe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18699=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18699=01-02 Stats: 12 lines in 1 file changed: 0 ins; 12 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18699.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18699/head:pull/18699 PR: https://git.openjdk.org/jdk/pull/18699
Re: RFR: 8328481: Implement Module Imports [v8]
On Thu, 18 Apr 2024 05:43:03 GMT, Alan Bateman wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixing ListModuleDeps test. > > src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 84: > >> 82: @JEP(number=461, title="Stream Gatherers", status="Preview") >> 83: STREAM_GATHERERS, >> 84: @JEP(number=0, title="Module Imports", status="Preview") > > I see this has been assigned JEP 476 so I assume you can set the number for > the first integration. Done, thanks: https://github.com/openjdk/jdk/pull/18614/commits/432393abb4ac1f5c4730d982a3911284fe866318 - PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1570071215
Re: RFR: 8328481: Implement Module Imports [v9]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Updating JEP number and caption. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/7fa0ad51..432393ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v2]
On Tue, 16 Apr 2024 14:29:13 GMT, Matthias Baesken wrote:
>> src/java.base/windows/native/libjli/java_md.c line 326:
>>
>>> 324: }
>>> 325:
>>> 326: JLI_TraceLauncher("GetJREPath - attempt to get JRE location from
>>> shared lib of the image\n");
>>
>> Maybe add a trace also in the USE_REGISTRY_LOOKUP section
>
> Hi Christoph, seems the USE_REGISTRY_LOOKUP is currently unused (at least
> without additional defines that are not present usually)
>
> src/java.base/windows/native/libjli/java_md.c:52:#ifdef USE_REGISTRY_LOOKUP
> src/java.base/windows/native/libjli/java_md.c:333:#ifdef USE_REGISTRY_LOOKUP
>
>
> I am not sure if this even works any more. Maybe Alan could comment on this ?
@MBaesken I checked into the building with -DUSE_REGISTRY_LOOKUP as that
compiled in code that the Oracle installer needed when it copied java.exe to
somewhere. This is indeed left over code and can be removed from java_md.c.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18699#discussion_r1570072071
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v7]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Merge branch 'master' into JDK-8294960-invoke # Conflicts: # src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java - applied suggested changes - Merge branch 'master' into JDK-8294960-invoke - Reversion of ClassBuilder changes - Merge branch 'master' into JDK-8294960-invoke - Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - added missing comment - fixed InnerClassLambdaMetafactory for hidden caller classes - performance improvements - ... and 3 more: https://git.openjdk.org/jdk/compare/706b421c...bae31c64 - Changes: https://git.openjdk.org/jdk/pull/17108/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17108=06 Stats: 2094 lines in 9 files changed: 421 ins; 838 del; 835 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
