Re: RFR: 8330465: Stable Values and Collections (Internal) [v20]
On Fri, 17 May 2024 09:31:33 GMT, Per Minborg wrote:
>> # Stable Values & Collections (Internal)
>>
>> > src="https://github.com/openjdk/jdk/assets/7457876/db4b22a1-af87-4914-adac-b05a87e7cb42;
>> width=20% height=20%>
>>
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API,
>> which provides immutable value holders where elements are initialized _at
>> most once_. Stable Values & Collections offer the performance and safety
>> benefits of final fields while offering greater flexibility as to the timing
>> of initialization.
>>
>> ## Goals
>> * Provide an easy and intuitive API to describe value holders that can
>> change at most once.
>> * Decouple declaration from initialization without significant footprint or
>> performance penalties.
>> * Reduce the amount of static initializer and/or field initialization code.
>> * Uphold integrity and consistency, even in a multi-threaded environment.
>>
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>>
>> ## Performance
>> Performance compared to instance variables using two `AtomicReference` and
>> two protected by double-checked locking under concurrent access by all
>> threads:
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> StableBenchmark.atomicthrpt 10259.478 ?
>> 36.809 ops/us
>> StableBenchmark.dcl thrpt 10225.710 ?
>> 26.638 ops/us
>> StableBenchmark.stablethrpt 10 4382.478 ?
>> 1151.472 ops/us <- StableValue significantly faster
>>
>>
>> Performance compared to static variables protected by `AtomicReference`,
>> class-holder idiom holder, and double-checked locking (all threads):
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> StableStaticBenchmark.atomic thrpt 10 6487.835 ?
>> 385.639 ops/us
>> StableStaticBenchmark.dcl thrpt 10 6605.239 ?
>> 210.610 ops/us
>> StableStaticBenchmark.stable thrpt 10 14338.239 ?
>> 1426.874 ops/us
>> StableStaticBenchmark.staticCHI thrpt 10 13780.341 ?
>> 1839.651 ops/us
>>
>>
>> Performance for stable lists (thread safe) in both instance and static
>> contexts whereby we access a single value compared to `ArrayList` instances
>> (which are not thread-safe) (all threads):
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> StableListElementBenchmark.instanceArrayList thrpt 10 5812.992 ?
>> 1169.730 ops...
>
> Per Minborg has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Add benchmarks for memoized IntFunction and Function
> - Add benchmark for memoized supplier
src/java.base/share/classes/jdk/internal/lang/StableArray.java line 66:
> 64: * @throws IllegalArgumentException if the provided {@code length} is
> {@code < 0}
> 65: */
> 66: static StableArray of(int length) {
I interpret the method name `of` as a method that creates an object that
contains the argument as some kind of member, in the way that `List.of` and
friends work.
My intuitive interpretation of `StableArray.of(10)` is that it returns an array
with the single element 10.
I think a method like this should be named `empty`, or `emptyOfLength` or
something like that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1606529054
Re: RFR: 8310890: Normalize identifier names
On Mon, 26 Jun 2023 14:07:03 GMT, Pavel Rappo wrote: > Please review this cleanup PR to normalize names of identifiers which are > Java variables/fields or tokens in text files. Those names either contain a > pronoun that is very rarely used in code, or seem like they contain such a > pronoun, which, in fact, they don't. Either way, the goal is to improve > readability and clarity. > > Also, this PR fixes a few related typos. make/data/charsetmapping/charsets line 149: > 147: package sun.nio.cs > 148: typesbcs > 149: histname ISO8859_2 Should this column be re-aligned with the longer name? - PR Review Comment: https://git.openjdk.org/jdk/pull/14653#discussion_r1242595069
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Tue, 20 Jun 2023 18:23:09 GMT, Roger Riggs wrote: > A positive number is any number that is greater than 0. **Unlike positive > integers**, which include 0 math.net seems pretty alone in this. I find the notion bizarre. There seems to be an overwhelming majority of sources that consider 0 to be neither negative nor positive. - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1599480422
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:54:59 GMT, Pavel Rappo wrote: > While the new wording is clearly an improvement, this reads weird: > > > @return the comparator value is less... > I suggest you use the safe form as the old text, which used a comma: > ``` > @return the comparator value, that is negative... > ``` - PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1596245859
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs 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:
>
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>Refine timeline order to mention before and after
>Add javadoc @see tags to isBefore and isAfter methods
> The term "positive" is ambiguous, zero is considered positive and indicates
> equality.
Where did you get this idea? A "positive" means a number that is _greater_ than
zero.
See both these dictionaries:
https://dictionary.cambridge.org/dictionary/english/positive
https://www.merriam-webster.com/dictionary/positive
-
PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1596244674
Re: RFR: 8310242: Clarify the name parameter to Class::forName [v4]
On Sat, 17 Jun 2023 00:35:22 GMT, Mandy Chung wrote:
>> This PR clarifies the spec of the 3-arg Class::forName regarding the format
>> of the name for an array type which is of the form: one or more of "[" +
>> binary name of the element type + ";'.
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> missing 'L' for the array class name
src/java.base/share/classes/java/lang/Class.java line 395:
> 393: * attached thread), the system class loader is used.
> 394: *
> 395: * @param className the binary name
> of the class
Where do these links lead? I don't find any section with this ID in the `Class`
Javadoc. Should they lead to `ClassLoader`? That Javadoc has such a section.
src/java.base/share/classes/java/lang/Class.java line 427:
> 425: * Returns the {@code Class} object associated with the class or
> 426: * interface with the given string name, using the given class
> loader.
> 427: * Given the {@linkplain ##binary-name binary name} for a class or
> interface,
Is double hash a mistake?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14528#discussion_r1233359948
PR Review Comment: https://git.openjdk.org/jdk/pull/14528#discussion_r1233360226
Re: RFR: 8310242: Clarify the name parameter to Class::forName [v4]
On Sat, 17 Jun 2023 00:35:22 GMT, Mandy Chung wrote: >> This PR clarifies the spec of the 3-arg Class::forName regarding the format >> of the name for an array type which is of the form: one or more of "[" + >> binary name of the element type + ";'. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > missing 'L' for the array class name src/java.base/share/classes/java/lang/Class.java line 395: > 393: * attached thread), the system class loader is used. > 394: * > 395: * @param className the binary name > of the class The new text is probably more correct, but it is less easy to understand. I think "fully qualified name" of a class is a well-known term, but "binary name" is rather unknown. I suggest changing the text to mention both terms. For example like this: Suggestion: * @param className the binary name) of the class, for example its fully qualified name - PR Review Comment: https://git.openjdk.org/jdk/pull/14528#discussion_r1233357830
Re: RFR: 8305947: SequenceInputStream implementation can use an Iterator rather than Enumeration
On Mon, 19 Dec 2022 11:26:25 GMT, Romain Manni-Bucau wrote:
> enumeration(list) will create an enumeration, a list and an iterator whereas
> the impl only requires an iterator
> this PR drops the enumeration wrapper for binary constructor and just maps
> the enumeration to an iterator for the other case which should be a better
> compromise in practise.
>
> Another side but nice effect is to have some lighter classloading (subgraph)
src/java.base/share/classes/java/io/SequenceInputStream.java line 82:
> 80: */
> 81: public SequenceInputStream(InputStream s1, InputStream s2) {
> 82: this(List.of(s1, s2).iterator());
This changes the behaviour for null streams. This line will throw NPE. The
previous code throwed NPE in `peekNextStream`, only if the stream was used.
Have your make sure that it is okay to change behaviour?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/11718#discussion_r1162428158
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 10:41:33 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > added 4-byte Unicode text to Utf8EntryTest src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java line 96: > 94: * @param the type of the option value > 95: */ > 96: T optionValue(Classfile.Option.Key option); This unconstrained type parameter will result in and implicit conversion to any type that the caller assigns it to, which might result in a `ClassCastException` if the caller gets the type wrong. Is that intentional? Alternative solutions: * Convert `Key` to an ordinary class or sealed interface, and add a type parameter to it, for the value type. * Add a parameter the the method of type `Class`. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v2]
On Mon, 21 Nov 2022 17:43:58 GMT, Markus KARG wrote: > I think the public visibility of my Twitter account is not wide enough to get > really robust answers, unfortunately. One alternative is to search GitHub. It's amazing how fast they can search such a huge code corpus. Example: https://github.com/search?l==%22new+SequenceInputStream%22+-filename%3ASequenceInputStreamTest.java=code One problem with GitHub search is that often you get a lot of duplicate matches. In the example above I have filtered out `SequenceInputStreamTest.java` which showed up a lot. - PR: https://git.openjdk.org/jdk/pull/11249
Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v2]
On Sun, 20 Nov 2022 09:14:32 GMT, Markus KARG wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> allowing s2 to be null
>
> src/java.base/share/classes/java/io/SequenceInputStream.java line 82:
>
>> 80: * @param s2 the second input stream to read.
>> 81: */
>> 82: public SequenceInputStream(InputStream s1, InputStream s2) {
>
> BTW, what is your opinion @jaikiran and @AlanBateman: We could simplify the
> 2-arg constructor by calling `this(...)` instead of repeating the 1-arg
> constructor's implementation here. Is that a *preferred* or a *disliked*
> pattern in OpenJDK?
The updated code now changes the behaviour in the other direction:
Previously, if `s2` was null a NPE was thrown in `peekNextStream` when `s1` was
exhausted.
In the current code, `s2` is silently ignored if it is null.
A safer alternative that preserves the behaviour of nulls seems to be the
replace `List.of` with `Arrays.asList`.
These subtle changes in behaviour demonstrates the problem with even trivial
updates to legacy code...
-
PR: https://git.openjdk.org/jdk/pull/11249
