Re: What is meant by "document context" in JEP 198?
On 25/11/2022 02:04, Ethan McCue wrote: I suppose while I'm asking questions - what exactly are the parts of the JDK making use of ad-hoc json? Maybe we could ship *something* as a jdk.internal module for those use cases? JEP 198 pre-dates records, sealed classes, pattern matching, ... and I will assume will re-drafted or replaced when the time comes. Right now, I don't think there are too many places in the JDK that interact with JSON. The `jfr` tool can print the records from a JFR recording as a JSON document. The HotSpotDiagnosMXBean API and `jcmd` tool can generate a thread dump as a JSON document. I think the only parsing at this time is the HotSpot compiler control (JEP 165) where directives file is a subset of JSON but that is implemented in C++ in libjvm and probably not what you are looking for. There are number of places in the JDK that read configuration and it might be that some of these could consume configuration in JSON in the future. -Alan
Re: RFR: JDK-8297298 - SequenceInputStream should override transferTo [v2]
On Thu, 24 Nov 2022 20:22:16 GMT, Markus KARG wrote: > Kindly requesting review. :-) The override looks fine. I think [bplb](https://github.com/bplb) plans to review/sponsor it. - PR: https://git.openjdk.org/jdk/pull/11248
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Fri, 25 Nov 2022 06:46:52 GMT, ExE Boss wrote: > I believe `UnsupportedOperationException` would be better for this. It would but Per is right that this it is throwing ISE today, it's just that the PR adds an explicit check and that highlights that the wrong exception is thrown. Buffers that are views over memory that is thread confined can't be used with the Asynchronous APIs and this will needed to be specified. There is further work required in this area too because IOUtil was intended for synchronous I/O. The "async" support was added on a temporary basis and will need to be re-visited at some point. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 14:56:07 GMT, Per Minborg wrote:
>> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 480:
>>
>>> 478: static MemorySessionImpl acquireScope(ByteBuffer bb, boolean
>>> async) {
>>> 479: if (async && NIO_ACCESS.isThreadConfined(bb)) {
>>> 480: throw new IllegalStateException("Confined session not
>>> supported");
>>
>> No sure about ISE here. There is no mutable state on the input that would
>> allow the acquire to succeed. I don't think anyone has run into it yet but
>> we will need to look at the Asynchronous read/write methods to decide
>> which exception to specify.
>
> This was the old behaviour which was retained in this PR.
I believe `UnsupportedOperationException` would be better for this.
-
PR: https://git.openjdk.org/jdk/pull/11260
Integrated: 8297385: Remove duplicated null typos in javadoc
On Wed, 23 Nov 2022 06:58:09 GMT, Dongxu Wang wrote: > 8297385: Remove duplicated null typos in javadoc This pull request has now been integrated. Changeset: 0ed8b337 Author:Dongxu Wang Committer: Yi Yang URL: https://git.openjdk.org/jdk/commit/0ed8b337eaa59881a62af5dcc0abb454761f2e71 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8297385: Remove duplicated null typos in javadoc Reviewed-by: dfuchs, rriggs - PR: https://git.openjdk.org/jdk/pull/11311
Re: What is meant by "document context" in JEP 198?
I suppose while I'm asking questions - what exactly are the parts of the
JDK making use of ad-hoc json? Maybe we could ship *something* as a
jdk.internal module for those use cases?
On Thu, Nov 24, 2022 at 8:55 PM Ethan McCue wrote:
> I'm reading JEP 198 and sketching out what an implementation could look
> like pursuant to this old conversation.
>
> https://mail.openjdk.org/pipermail/discuss/2020-April/005401.html
>
> My biggest question right now is what does the JEP mean exactly by
> "document context"
>
>
>- Parsing APIs which allow a choice of parsing token stream, event
>(includes document hierarchy context) stream, or immutable tree
>representation views of JSON documents and data streams.
>
>
>
> So token stream I understand as something akin to
>
> sealed interface Token {
> record StartArray() implements Token {}
> record EndArray() implements Token {}
> record StartObject() implements Token {}
> record EndObject() implements Token {}
> record Number(Json.Number value) implements Token {}
> record String(Json.String value) implements Token {}
> record True() implements Token {}
> record False() implements Token {}
> record Null() implements Token {}
> }
>
> Which matches up with the model in
> https://fasterxml.github.io/jackson-core/javadoc/2.7/com/fasterxml/jackson/core/JsonToken.html
>
> And an immutable tree representation as akin to
>
> sealed interface Json {
> sealed interface Array extends Json, List ...
> sealed interface Object extends Json, Map ...
> sealed abstract class Number extends java.lang.Number implements Json
> ...
> sealed interface String extends Json, CharSequence ...
> sealed interface Boolean ...
> sealed interface Null extends Json ...
> }
>
> Which, ignoring the open questions of
> * Does the immutable tree need to be eagerly realized?
> * Do we need to wait for valhalla to land
> * Do we need to wait for full pattern matching to land
>
> (because they make me sad)
>
> I'm still unsure what information needs to be included in an "Event"
> stream that would constitute "document context". Is it pointers to parent
> collections? The current "Path"?
>
> sealed interface PathElement {
> record Field(String fieldName) implements PathElement {}
> record Index(int index) implements PathElement {}
> }
>
>
>
>
What is meant by "document context" in JEP 198?
I'm reading JEP 198 and sketching out what an implementation could look
like pursuant to this old conversation.
https://mail.openjdk.org/pipermail/discuss/2020-April/005401.html
My biggest question right now is what does the JEP mean exactly by
"document context"
- Parsing APIs which allow a choice of parsing token stream, event
(includes document hierarchy context) stream, or immutable tree
representation views of JSON documents and data streams.
So token stream I understand as something akin to
sealed interface Token {
record StartArray() implements Token {}
record EndArray() implements Token {}
record StartObject() implements Token {}
record EndObject() implements Token {}
record Number(Json.Number value) implements Token {}
record String(Json.String value) implements Token {}
record True() implements Token {}
record False() implements Token {}
record Null() implements Token {}
}
Which matches up with the model in
https://fasterxml.github.io/jackson-core/javadoc/2.7/com/fasterxml/jackson/core/JsonToken.html
And an immutable tree representation as akin to
sealed interface Json {
sealed interface Array extends Json, List ...
sealed interface Object extends Json, Map ...
sealed abstract class Number extends java.lang.Number implements Json
...
sealed interface String extends Json, CharSequence ...
sealed interface Boolean ...
sealed interface Null extends Json ...
}
Which, ignoring the open questions of
* Does the immutable tree need to be eagerly realized?
* Do we need to wait for valhalla to land
* Do we need to wait for full pattern matching to land
(because they make me sad)
I'm still unsure what information needs to be included in an "Event" stream
that would constitute "document context". Is it pointers to parent
collections? The current "Path"?
sealed interface PathElement {
record Field(String fieldName) implements PathElement {}
record Index(int index) implements PathElement {}
}
Re: What are the policies for internal modules?
(always so hard to remember to reply all) On Thu, Nov 24, 2022 at 8:26 PM Ethan McCue wrote: > The use here is giving help to the user on > misspelled classes/methods/fields like "new ArayList". > > For now we've just copy pasted the class since that seems the least > architecturally painful. (And it's not clear that text distance is the full > story with regards to this sort of analysis, so tbd if that stays in its > current form) > > On Mon, Nov 21, 2022 at 3:19 AM Alan Bateman > wrote: > >> On 21/11/2022 01:02, Ethan McCue wrote: >> > There are some modules like jdk.internal.le which contain no >> > publicly exported packages. In some of the experimentation we are >> > doing, We want to use the >> > jdk.internal.org.jline.utils.Levenshtien class from jdk.compiler. >> > >> > Mechanically, we can do this without creating any new modules by >> > adding a qualified export of the utils package to jdk.compiler. We >> > could also make a brand new, tiny pointless module called >> > jdk.internal.levenshtien. That would be the "cleanest" approach, but >> > only if we didn't consider the existence of the internal modules to be >> > public API. >> > >> > So that's my general question - what is the bar for making a new >> > internal module, and is the set of internal modules subject to >> > backwards compatibility concerns? >> >> There aren't many compatibility constraints on jdk.internal modules. >> Assume they can change or be removed in any release. It's important they >> don't export any packages to all modules, otherwise they cease to be >> "internal". Also if they provide services then the name of the module >> may be something that users of jlink may become dependent on, so we have >> to be careful there. >> >> A general point is that we don't want the JDK to backslide to a big ball >> of mud. We put a lot of effort several years ago to de-tangle the >> libraries so it would be disappointing see jdk.compiler, with no >> interest in line editing, add a dependency on jdk.internal.le so that it >> can make use of one of utility classes. So I don't think we should do >> that. >> >> Can you say a bit more about what you are doing? Is this just a method >> to compute the Levenshtein distance between two strings? I assume you >> can just implement that in jdk.compiler without touching the module >> graph. It might be useful to get some sense on how common fuzzy matching >> is in the eco system to see if there is any merit to exposing APIs for >> this. >> >> -Alan >> >>
Re: RFR: JDK-8297298 - SequenceInputStream should override transferTo [v2]
On Sun, 20 Nov 2022 09:41:47 GMT, Markus KARG wrote: >> Since JDK 18 some implementations of InputStream.transferTo, namely >> FileInputStream and ChannelInputStream, offload work to lower layers using >> NIO channels. This provides shorter execution time and reduced resource >> consumption. Unfortunately, this effect is prevented once the input stream >> itself is wrapped by a SequenceInputStream. While compared to other >> InputStreams the SequenceInputStream is a rather edge case (e. g. used to >> merge two files into one), nevertheless it makes sense to get rid of this >> obstacle simply by implementing transferTo (e. g. by allowing to offload the >> file merge, or parts of the file merge, to the operating system). As a >> result, more existing applications will experience the >> offloading-improvements made by JDK 18. >> >> To provide enhanced performance to existing applications, it makes sense to >> address this impediment: SequenceInputStream.transferTo should be >> implemented in a way that iteratively calls transferTo on each enumerated >> InputStream of the SequenceInputStream in ordered sequence. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed bug number Kindly requesting review. :-) - PR: https://git.openjdk.org/jdk/pull/11248
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation [v2]
On Thu, 24 Nov 2022 17:50:42 GMT, Andrey Turbanov wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Require 64-bit arch to avoid (most) hypothetical false positives
>
> test/jdk/java/lang/String/concat/ImplicitStringConcatOOME.java line 129:
>
>> 127: }
>> 128:
>> 129: public static void test(String expected, String actual) {
>
> When this method is called?
Never - accidental left-over from the test I used as a template for this. I'd
remove it if I hadn't already integrated - but opening a bug to clean that out
doesn't seem worthwhile.
-
PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation [v2]
On Thu, 24 Nov 2022 15:01:12 GMT, Claes Redestad wrote:
>> When concatenating Strings, OutOfMemoryError should be thrown on all
>> overflow conditions. This fixes a case that erroneously thro IAE on
>> concatenations of long (`length > Integer.MAX_VALUE/2`) UTF16 strings due
>> failing to check for overflow after shifting index left with the coder.
>>
>> This was caught by a fuzzer test. Added a sanity test that fails without the
>> patch (loosely derived from ImplicitStringConcatMany.java)
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Require 64-bit arch to avoid (most) hypothetical false positives
test/jdk/java/lang/String/concat/ImplicitStringConcatOOME.java line 129:
> 127: }
> 128:
> 129: public static void test(String expected, String actual) {
When this method is called?
-
PR: https://git.openjdk.org/jdk/pull/11354
Integrated: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation
On Thu, 24 Nov 2022 13:23:13 GMT, Claes Redestad wrote: > When concatenating Strings, OutOfMemoryError should be thrown on all overflow > conditions. This fixes a case that erroneously thro IAE on concatenations of > long (`length > Integer.MAX_VALUE/2`) UTF16 strings due failing to check for > overflow after shifting index left with the coder. > > This was caught by a fuzzer test. Added a sanity test that fails without the > patch (loosely derived from ImplicitStringConcatMany.java) This pull request has now been integrated. Changeset: 87d1097d Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/87d1097d9be1ef804bfd4640a4254126242b1d8c Stats: 141 lines in 2 files changed: 139 ins; 0 del; 2 mod 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation Reviewed-by: enikitin, alanb - PR: https://git.openjdk.org/jdk/pull/11354
Integrated: 8297150: Add a @sealedGraph tag to Reference
On Wed, 16 Nov 2022 16:52:50 GMT, Per Minborg wrote: > This PR proposes to opt in for graphic rendering of the sealed hierarchy of > the `Reference` class. > > Rendering capability was added via https://bugs.openjdk.org/browse/JDK-8295653 > > Here is how it would look like: > > src="https://user-images.githubusercontent.com/7457876/202243138-7f2a799a-dc67-4c4e-9322-c18bf05d73e7.png;> This pull request has now been integrated. Changeset: 390e69ad Author:Per Minborg Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/390e69ad065ebefe2e998f6200d19d45cf043b16 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8297150: Add a @sealedGraph tag to Reference Reviewed-by: darcy, alanb - PR: https://git.openjdk.org/jdk/pull/11191
Re: RFR: 8297150: Add a @sealedGraph tag to Reference
On Wed, 16 Nov 2022 16:52:50 GMT, Per Minborg wrote: > This PR proposes to opt in for graphic rendering of the sealed hierarchy of > the `Reference` class. > > Rendering capability was added via https://bugs.openjdk.org/browse/JDK-8295653 > > Here is how it would look like: > > src="https://user-images.githubusercontent.com/7457876/202243138-7f2a799a-dc67-4c4e-9322-c18bf05d73e7.png;> Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11191
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Reformat and fix comment
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/0546b23b..30aff118
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=11260=13
- incr: https://webrevs.openjdk.org/?repo=jdk=11260=12-13
Stats: 26 lines in 3 files changed: 13 ins; 2 del; 11 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation [v2]
On Thu, 24 Nov 2022 15:01:12 GMT, Claes Redestad wrote: >> When concatenating Strings, OutOfMemoryError should be thrown on all >> overflow conditions. This fixes a case that erroneously thro IAE on >> concatenations of long (`length > Integer.MAX_VALUE/2`) UTF16 strings due >> failing to check for overflow after shifting index left with the coder. >> >> This was caught by a fuzzer test. Added a sanity test that fails without the >> patch (loosely derived from ImplicitStringConcatMany.java) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Require 64-bit arch to avoid (most) hypothetical false positives Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation [v2]
On Thu, 24 Nov 2022 14:14:56 GMT, Alan Bateman wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Require 64-bit arch to avoid (most) hypothetical false positives > > test/jdk/java/lang/String/concat/ImplicitStringConcatOOME.java line 36: > >> 34: * @run main/othervm -Xverify:all -Xmx4g ImplicitStringConcatOOME >> 35: * >> 36: * @compile ImplicitStringConcatOOME.java > > I don't think you need the @compile tags but you might need `@requires > sun.arch.data.model == "64"` so that the test only runs on 64-bit systems. Seems to work without yes. Started with a copy of a similar test that used the `@compile` tags to control different compilation modes. Testing each ISC compilation mode seemed excessive for this sanity test. - PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation [v2]
> When concatenating Strings, OutOfMemoryError should be thrown on all overflow > conditions. This fixes a case that erroneously thro IAE on concatenations of > long (`length > Integer.MAX_VALUE/2`) UTF16 strings due failing to check for > overflow after shifting index left with the coder. > > This was caught by a fuzzer test. Added a sanity test that fails without the > patch (loosely derived from ImplicitStringConcatMany.java) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Require 64-bit arch to avoid (most) hypothetical false positives - Changes: - all: https://git.openjdk.org/jdk/pull/11354/files - new: https://git.openjdk.org/jdk/pull/11354/files/884dac30..124a7350 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11354=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11354=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11354.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11354/head:pull/11354 PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 12:06:44 GMT, Alan Bateman wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove redundant method
>
> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 480:
>
>> 478: static MemorySessionImpl acquireScope(ByteBuffer bb, boolean async)
>> {
>> 479: if (async && NIO_ACCESS.isThreadConfined(bb)) {
>> 480: throw new IllegalStateException("Confined session not
>> supported");
>
> No sure about ISE here. There is no mutable state on the input that would
> allow the acquire to succeed. I don't think anyone has run into it yet but we
> will need to look at the Asynchronous read/write methods to decide which
> exception to specify.
This was the old behaviour which was retained in this PR.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:58:12 GMT, Maurizio Cimadamore
wrote:
>> Separately, also (optional) stylistic: the indenting on this and following
>> class is a bit odd. There is a certain tendency to compress lines - e.g. to
>> reach for one-liners, when in reality the body is not that trivial. If I
>> were to write the code I'd insert a newline after the `{` and I'd also break
>> apart initialization (e.g. no two statements separated by `;` in the same
>> line).
>>
>> Also, I noted that you start the body of the record (e.g. `{`) on a new
>> line, which I also find odd.
>
> (to be clear, some of the above comments refer to the code that was already
> there before your changes)
I also found the above as a bit odd but tried to stick with the existing
implementation and style as much as possible.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation
On Thu, 24 Nov 2022 13:23:13 GMT, Claes Redestad wrote: > When concatenating Strings, OutOfMemoryError should be thrown on all overflow > conditions. This fixes a case that erroneously thro IAE on concatenations of > long (`length > Integer.MAX_VALUE/2`) UTF16 strings due failing to check for > overflow after shifting index left with the coder. > > This was caught by a fuzzer test. Added a sanity test that fails without the > patch (loosely derived from ImplicitStringConcatMany.java) test/jdk/java/lang/String/concat/ImplicitStringConcatOOME.java line 36: > 34: * @run main/othervm -Xverify:all -Xmx4g ImplicitStringConcatOOME > 35: * > 36: * @compile ImplicitStringConcatOOME.java I don't think you need the @compile tags but you might need `@requires sun.arch.data.model == "64"` so that the test only runs on 64-bit systems. - PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v33]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Unused variable - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/1b3c39bc..37441eeb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=32 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=31-32 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation
On Thu, 24 Nov 2022 13:23:13 GMT, Claes Redestad wrote: > When concatenating Strings, OutOfMemoryError should be thrown on all overflow > conditions. This fixes a case that erroneously thro IAE on concatenations of > long (`length > Integer.MAX_VALUE/2`) UTF16 strings due failing to check for > overflow after shifting index left with the coder. > > This was caught by a fuzzer test. Added a sanity test that fails without the > patch (loosely derived from ImplicitStringConcatMany.java) LGTM. - Marked as reviewed by enikitin (Author). PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v31]
> Enhance the Java programming language with string templates, which are > similar to string literals but contain embedded expressions. A string > template is interpreted at run time by replacing each expression with the > result of evaluating that expression, possibly after further validation and > transformation. This is a [preview language feature and > API](http://openjdk.java.net/jeps/12). Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: FormatProcessor changes - Changes: - all: https://git.openjdk.org/jdk/pull/10889/files - new: https://git.openjdk.org/jdk/pull/10889/files/09f1ac08..69efbb48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10889=30 - incr: https://webrevs.openjdk.org/?repo=jdk=10889=29-30 Stats: 131 lines in 3 files changed: 81 ins; 16 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/10889.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10889/head:pull/10889 PR: https://git.openjdk.org/jdk/pull/10889
RFR: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation
When concatenating Strings, OutOfMemoryError should be thrown on all overflow conditions. This fixes a case that erroneously thro IAE on concatenations of long (`length > Integer.MAX_VALUE/2`) UTF16 strings due failing to check for overflow after shifting index left with the coder. This was caught by a fuzzer test. Added a sanity test that fails without the patch (loosely derived from ImplicitStringConcatMany.java) - Commit messages: - Verify the sanity test holds when -CompactStrings - Adjust newArray to correctly throw OOME on overflow due shifting the index for UTF16 encoding Changes: https://git.openjdk.org/jdk/pull/11354/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11354=00 Issue: https://bugs.openjdk.org/browse/JDK-8297530 Stats: 143 lines in 2 files changed: 141 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11354.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11354/head:pull/11354 PR: https://git.openjdk.org/jdk/pull/11354
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation I've forgotten the `JvmtiVTMSTransitionDisabler` is not going to work before the `notifyJvmtiEvents` is set to `true`. I agree, we may want to allow `start_VTMS_transition/finish_VTMS_transition` not properly paired as you suggest. But then it is not good that we loose the ability to strictly check/assert pairing of VTMS transition notifications for other cases. Need to think a bit more on this. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove redundant method
src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java line 790:
> 788: try {
> 789: int n = receive0(fd,
> 790: ((DirectBuffer)bb).address() + pos, rem,
Would it be possible to restore the original alignment, just to make it easier
to read.
src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 39:
> 37: // silent unrelated memory mutation and JVM crashes.
> 38: //
> 39: // Guards are available in the JavaNioAccess class.
Did you mean to leave the word "Guards" in the comment? I guess I would say
something "See JavaNioAccess for methods to acquire/release" or something like
that.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 480:
> 478: static MemorySessionImpl acquireScope(ByteBuffer bb, boolean async) {
> 479: if (async && NIO_ACCESS.isThreadConfined(bb)) {
> 480: throw new IllegalStateException("Confined session not
> supported");
No sure about ISE here. There is no mutable state on the input that would allow
the acquire to succeed. I don't think anyone has run into it yet but we will
need to look at the Asynchronous read/write methods to decide which
exception to specify.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 543:
> 541: @Override public void run() { releaseScope(bb, session); }
> 542: static Runnable of(ByteBuffer bb, MemorySessionImpl session) {
> return new Releaser(bb, session); }
> 543: static Runnable ofNullable(ByteBuffer bb, MemorySessionImpl
> session) {
Would it be possible to re-format this to make it more readable? There's just a
bit more going on compared to the original and it's harder to read.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove redundant method
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 519:
> 517: }
> 518:
> 519: record LinkedRunnable(Runnable node, Runnable next)
Some (optional) style comments: I'm not sure this is a record. E.g. both this
and the `Releaser` class are only used externally to call their `run` method.
The fact that they are made up of components is immaterial to clients, which
seems to suggest that an interface would be better - at least subjectively. If
I were to write the code I will declare the implementation inside the
factories, as local/anon classes.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:57:33 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 519:
>>
>>> 517: }
>>> 518:
>>> 519: record LinkedRunnable(Runnable node, Runnable next)
>>
>> Some (optional) style comments: I'm not sure this is a record. E.g. both
>> this and the `Releaser` class are only used externally to call their `run`
>> method. The fact that they are made up of components is immaterial to
>> clients, which seems to suggest that an interface would be better - at least
>> subjectively. If I were to write the code I will declare the implementation
>> inside the factories, as local/anon classes.
>
> Separately, also (optional) stylistic: the indenting on this and following
> class is a bit odd. There is a certain tendency to compress lines - e.g. to
> reach for one-liners, when in reality the body is not that trivial. If I were
> to write the code I'd insert a newline after the `{` and I'd also break apart
> initialization (e.g. no two statements separated by `;` in the same line).
>
> Also, I noted that you start the body of the record (e.g. `{`) on a new line,
> which I also find odd.
(to be clear, some of the above comments refer to the code that was already
there before your changes)
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:54:43 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove redundant method
>
> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 519:
>
>> 517: }
>> 518:
>> 519: record LinkedRunnable(Runnable node, Runnable next)
>
> Some (optional) style comments: I'm not sure this is a record. E.g. both this
> and the `Releaser` class are only used externally to call their `run` method.
> The fact that they are made up of components is immaterial to clients, which
> seems to suggest that an interface would be better - at least subjectively.
> If I were to write the code I will declare the implementation inside the
> factories, as local/anon classes.
Separately, also (optional) stylistic: the indenting on this and following
class is a bit odd. There is a certain tendency to compress lines - e.g. to
reach for one-liners, when in reality the body is not that trivial. If I were
to write the code I'd insert a newline after the `{` and I'd also break apart
initialization (e.g. no two statements separated by `;` in the same line).
Also, I noted that you start the body of the record (e.g. `{`) on a new line,
which I also find odd.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation Would it be possible to summarize behavior for when an agent enables the capability as a virtual thread executes for the first time or it continues after yield? More specifically JVMTI will be notified of a mount end without a correspond mount begin. It might be that we can narrow this down to if finish_VTMS_transition is okay without a preceding start_VTMS_transition. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove redundant method
Looks good - I'm glad that the changes to IOUtil turned out simpler than
expected!
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove redundant method
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/682b6f5a..0546b23b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=11260=12
- incr: https://webrevs.openjdk.org/?repo=jdk=11260=11-12
Stats: 213 lines in 20 files changed: 12 ins; 105 del; 96 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v32]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/903780d6..1b3c39bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=31 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=30-31 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: 8296546: Add @spec tags to API [v3]
On Wed, 23 Nov 2022 18:57:03 GMT, Jonathan Gibbons wrote: >> Please review a "somewhat automated" change to insert `@spec` tags into doc >> comments, as appropriate, to leverage the recent new javadoc feature to >> generate a new page listing the references to all external specifications >> listed in the `@spec` tags. >> >> "Somewhat automated" means that I wrote and used a temporary utility to scan >> doc comments looking for HTML links to selected sites, such as `ietf.org`, >> `unicode.org`, `w3.org`. These links may be in the main description of a doc >> comment, or in `@see` tags. For each link, the URL is examined, and >> "normalized", and inserted into the doc comment with a new `@spec` tag, >> giving the link and tile for the spec. >> >> "Normalized" means... >> * Use `https:` where possible (includes pretty much all cases) >> * Use a single consistent host name for all URLs coming from the same spec >> site (i.e. don't use different aliases for the same site) >> * Point to the root page of a multi-page spec >> * Use a consistent form of the spec, preferring HTML over plain text where >> both are available (this mostly applies to IETF specs) >> >> In addition, a "standard" title is determined for all specs, determined >> either from the content of the (main) spec page or from site index pages. >> >> The net effect is (or should be) that **all** the changes are to just >> **add** new `@spec` tags, based on the links found in each doc comment. >> There should be no other changes to the doc comments, or to the >> implementation of any classes and interfaces. >> >> That being said, the utility I wrote does have additional abilities, to >> update the links that it finds (e.g. changing to use `https:` etc,) but >> those features are _not_ being used here, but could be used in followup PRs >> if component teams so desired. I did notice while working on this overall >> feature that many of our links do point to "outdated" pages, some with >> eye-catching notices declaring that the spec has been superseded. >> Determining how, when and where to update such links is beyond the scope of >> this PR. >> >> Going forward, it is to be hoped that component teams will maintain the >> underlying links, and the URLs in `@spec` tags, such that if references to >> external specifications are updated, this will include updating the `@spec` >> tags. >> >> To see the effect of all these new `@spec` tags, see >> http://cr.openjdk.java.net/~jjg/8296546/api.00/ >> >> In particular, see the new [External >> Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) >> page, which you can also find via the new link near the top of the >> [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) >> pages. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Remove updates from unexported files src/java.se/share/classes/module-info.java line 39: > 37: * > 38: * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol > 39: * @spec jni/index.html Java Native Interface Specification One thing that that bothers me a bit here is that the JNI and JDWP specs will be listed as "External Specifications" in the generated javadoc. This heading is appropriate for RFCs and other standards that we reference but seems misleading for specifications that are part of Java SE. Has this come up already? - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v31]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/15db2a30..903780d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=30 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=29-30 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v30]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with five additional commits since the last revision: - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into JDK-828 - Update src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java Co-authored-by: Paul Sandoz - Update src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java Co-authored-by: Paul Sandoz - Reviewer feedback - Update src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/c10a5d79..15db2a30 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=29 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=28-29 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v29]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Update src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/4bcfa52e..c10a5d79 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=28 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=27-28 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v25]
On Wed, 23 Nov 2022 18:47:28 GMT, Paul Sandoz wrote: >> Andrew Haley has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 52 commits: >> >> - Merge master >> - javadoc >> - Feedback from reviewers >> - Update src/hotspot/share/classfile/vmSymbols.hpp >> - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into >> JDK-828 >> - Update src/java.base/share/classes/java/lang/VirtualThread.java >> >>Co-authored-by: Alan Bateman >> - Update src/java.base/share/classes/java/lang/Thread.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Update src/hotspot/cpu/aarch64/aarch64.ad >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Feedback from reviewers >> - Remove incorrect assertion. >> - ... and 42 more: https://git.openjdk.org/jdk/compare/2afb4c33...30f150e1 > > src/hotspot/share/classfile/vmSymbols.hpp line 401: > >> 399: template(daemon_name, "daemon") >> \ >> 400: template(run_method_name, "run") >> \ >> 401: template(call_method_name, "call") >> \ > > Is this used? No. - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v28]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Reviewer feedback - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/3a6f8037..4bcfa52e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=27 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=26-27 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v27]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into JDK-828 - Fix merge - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/1395b52f..3a6f8037 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=26 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=25-26 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
RFR: 8297561: Redundant index check in String.offsetByCodePoints()
`String.offsetByCodePoints()` delegates to `Character.offsetByCodePoints()` which in turn specifies the same exception thrown under the same conditions and the implementation does exactly the same checks. This means we can remove the check from `String.offsetByCodePoints()` and rely on the one of `Character.offsetByCodePoints()`. - Commit messages: - 8297561: Redundant index check in String.offsetByCodePoints() Changes: https://git.openjdk.org/jdk/pull/11350/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11350=00 Issue: https://bugs.openjdk.org/browse/JDK-8297561 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11350.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11350/head:pull/11350 PR: https://git.openjdk.org/jdk/pull/11350
Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]
On Wed, 16 Nov 2022 15:12:41 GMT, Roger Riggs wrote:
>> Weibing Xiao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> the change according to review comment
>
> test/jdk/java/io/File/TempDirectoryNotExisting.java line 45:
>
>> 43:
>> 44: String userDir = System.getProperty("user.home");
>> 45: String timeStamp = System.currentTimeMillis() + "";
>
> A human readable string might be useful. For example,
> "2022-11-16T15:10:50.622334Z"
> `java.time.Instant.now().toString()`
root cause for JDK-8297528
Please revert the change Weibing and be sure to remove the test from
ProblemList when doing so (next week)
Please also ensure that you run tests against mach5 before integrating any
change in future,
-
PR: https://git.openjdk.org/jdk/pull/11174
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v25]
On Wed, 23 Nov 2022 18:49:07 GMT, Paul Sandoz wrote: >> Andrew Haley has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 52 commits: >> >> - Merge master >> - javadoc >> - Feedback from reviewers >> - Update src/hotspot/share/classfile/vmSymbols.hpp >> - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into >> JDK-828 >> - Update src/java.base/share/classes/java/lang/VirtualThread.java >> >>Co-authored-by: Alan Bateman >> - Update src/java.base/share/classes/java/lang/Thread.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Update src/hotspot/cpu/aarch64/aarch64.ad >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Feedback from reviewers >> - Remove incorrect assertion. >> - ... and 42 more: https://git.openjdk.org/jdk/compare/2afb4c33...30f150e1 > > src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java > line 209: > >> 207: final int bitmask; >> 208: >> 209: private static final Object NIL = new Object(); > > Suggestion: > > static final Object NO_VALUE = new Object(); It not very important, but I'm going to push back (very gently) on this one. "nil: noun. nothing; naught; zero. adjective. having no value or existence." That is the exact literal meaning of this sentinel. Also, "nil" has been used with this meaning in programming languages for 60 years. What is your objection to it here? - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v25]
On Wed, 23 Nov 2022 19:16:23 GMT, Paul Sandoz wrote:
> Looks good (just some last minor comments). I did not focus on the tests, nor
> too closely at all of the HotSpot changes.
>
> Something for future investigation perhaps (if not already thought about):
> consider using a persistent map, a Hash Array Mapped Trie (HAMT), for storing
> scoped keys and values, which could potentially remove the need for the cache
> of replace the cache when many values are in scope. The HAMT's structural
> sharing properties, wide-branching factor, and `Integer.bitCount` being
> intrinsic all make for an efficient implementation.
I've certainly considered a HAMT. (I've considered everything! It's been a long
journey.) The trouble is that using one penalizes binding operations because a
persistent HAMT requires path copying for insertions. This won't matter if your
bind operation is rare, such as at the start of a large block of code. However,
it might not be. Consider a common case such as a parallel stream using
fork/join.
void parallelRunWithCredentials(List aList, Runnable aRunnable) {
var permissions = ScopedValue.where(PERMISSION, getCredentials());
aList.parallelStream().forEach(() -> permissions.run(aRunnable));
}
Because the binding operation `permissions.run()` is invoked for every element
on the list, we must make the binding operation as fast as it can possibly be.
In addition, the cache is _extremely_ fast. Typically it's four instructions,
and C2 almost always hoists values, once looked up in cache, into registers. A
HAMT lookup, while fast and (mostly) O(1), is more complex.
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v26]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with two additional commits since the last revision: - Update src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java Co-authored-by: Paul Sandoz - Update src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/30f150e1..1395b52f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=25 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=24-25 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
