Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v10]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: VM -> virtual machine; also fix misspelling - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/0f949d3c..adb1fbe3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16644&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16644&range=08-09 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Integrated: 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test
On Thu, 22 Feb 2024 09:49:31 GMT, Jaikiran Pai wrote: > Can I get a review of this change which proposes to remove the `SystemTest` > from among the `JSR166TestCase`? > > As noted in https://bugs.openjdk.org/browse/JDK-8278527 the > `SystemTest.nanoTime` test has been intermittently failing since a few years > now. Martin and Paul have investigated this failure in the past and they note > that this test is fragile and should be removed: > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14463658&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14463658 > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14508958&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508958 > > The commit in this PR removes the `SystemTest` that was enrolled into > `JSR166TestCase`. > > tier1 testing went fine with this change. This pull request has now been integrated. Changeset: 54f09d73 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/54f09d734584a71c648520664447f8395050adbe Stats: 76 lines in 2 files changed: 0 ins; 76 del; 0 mod 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test Reviewed-by: martin, lancea - PR: https://git.openjdk.org/jdk/pull/17960
Re: RFR: 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test
On Thu, 22 Feb 2024 09:49:31 GMT, Jaikiran Pai wrote: > Can I get a review of this change which proposes to remove the `SystemTest` > from among the `JSR166TestCase`? > > As noted in https://bugs.openjdk.org/browse/JDK-8278527 the > `SystemTest.nanoTime` test has been intermittently failing since a few years > now. Martin and Paul have investigated this failure in the past and they note > that this test is fragile and should be removed: > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14463658&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14463658 > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14508958&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508958 > > The commit in this PR removes the `SystemTest` that was enrolled into > `JSR166TestCase`. > > tier1 testing went fine with this change. Thank you Martin and Lance for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/17960#issuecomment-1960587846
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v4]
On Thu, 22 Feb 2024 22:50:03 GMT, Justin Lu wrote:
>> Please review this PR which handles an edge case pattern bug with
>> ChoiceFormat.
>>
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar|1.0#"
>> d.format(1) // unexpectedly returns ""
>>
>>
>> Not only does this lead to faulty formatting results, but breaks the
>> ChoiceFormat class invariant of duplicate limits.
>>
>> It would have been preferable if either an exception was thrown when the
>> ChoiceFormat was initially created, or the ChoiceFormat formatting 1
>> returned a value of "bar".
>>
>> For comparison,
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar"
>> d.format(1) // returns "bar"
>>
>>
>> After this change, the first code snippet now returns "bar" when formatting
>> 1 and discards the "baz" as the second code snippet does.
>>
>>
>> The specific fix for this bug is addressed with the following, on line 305,
>>
>> if (seg != Segment.FORMAT) {
>> // Discard incorrect portion and finish building cFmt
>> break;
>> }
>>
>> This prevents a limit/format entry from being built when the current parsing
>> mode has not yet processed the limit and relation. The previous
>> implementation would build an additional limit/format of 1 with an empty
>> string. The `break` allows for the discarding of the incorrect portion and
>> continuing. Alternatively an exception could have been thrown. For
>> consistency with the second code snippet, the former was picked.
>>
>> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
>> semi-related specification fix.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert enum change
LGTM, with a minor suggestion.
src/java.base/share/classes/java/text/ChoiceFormat.java line 342:
> 340: } else {
> 341: num = Double.parseDouble(str);
> 342: }
Could use enhanced switch?
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17883#pullrequestreview-1897207137
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1500096680
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 23:14:37 GMT, Brent Christian wrote:
>> I note that the adjective(s) (un)successful and the adverb(s)
>> (un)successfully are used at several places in these comments, it might
>> makes sense to use those terms here as well such that the documentation in
>> internally consistent in its use of success or failure of actions. In
>> particular, if this terminology is consistent with precedent in the official
>> JLS spec.
>>
>> However, I note that there are places where these terms are italicized and
>> places where they aren't. I am not sure I follow the convention for
>> italicization. In general, the first use (i.e. introduction) of a term that
>> the reader might want to pay attention to calls for italicization when
>> documents are read sequentially, such as in research papers. These javadoc
>> specs will usually not be read in sequentially. But considering that someone
>> does read them in order, I'd suggest italicizing only the first use of the
>> term or, if not, then perhaps none. Alternatively, you might want to
>> italicize all uses (but why?).
>
> Thanks for finding my misspelling, djelinski. 👍
The use of "(un)successful(ly)" in relation to `Reference.enqueue()` is quite
deliberate (and builds on the previous wording, "successful").
The intention was to use it consistently (is that not the case somewhere?). For
example, it's also used in the new **Memory Consistency Properties** section of
the `java.lang.ref` package docs ("The enqueueing of a reference...by a
successful call to `Reference.enqueue()`...").
A "successful call to `enqueue()`" is meant to be shorthand for:
"the reference has been enqueued, and the enqueuing was performed by the
`enqueue()` method (rather than by the garbage collector). Therefore there is a
_happens-before_ edge between the `enqueue()` method call and the dequeuing of
the Reference (whereas there would not be this _happens-before_ if the GC had
already enqueued the Reference at the time of the `enqueue()` call)."
The text emphasis with italics is to indicate this added significance of the
result of the `enqueue()` call -- ala `happens-before`.
I'm not aware of a similar scenario covered in the JLS, so AFAIK there is not
precedent to be consistent with in that regard.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1500073141
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 18:24:39 GMT, Y. Srinivas Ramakrishna wrote: >> or, better yet, `fails` > > I note that the adjective(s) (un)successful and the adverb(s) > (un)successfully are used at several places in these comments, it might makes > sense to use those terms here as well such that the documentation in > internally consistent in its use of success or failure of actions. In > particular, if this terminology is consistent with precedent in the official > JLS spec. > > However, I note that there are places where these terms are italicized and > places where they aren't. I am not sure I follow the convention for > italicization. In general, the first use (i.e. introduction) of a term that > the reader might want to pay attention to calls for italicization when > documents are read sequentially, such as in research papers. These javadoc > specs will usually not be read in sequentially. But considering that someone > does read them in order, I'd suggest italicizing only the first use of the > term or, if not, then perhaps none. Alternatively, you might want to > italicize all uses (but why?). Thanks for finding my misspelling, djelinski. 👍 - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1500054507
Re: RFR: JDK-8326530: Widen allowable error bound of Math.tan
On Thu, 22 Feb 2024 22:00:26 GMT, Joe Darcy wrote: > Widen acceptable error bound of Math.tan to accommodate the worst-case > observed error which is slightly outside of the allowable range. Looks fine. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17973#pullrequestreview-1897113120
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v4]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> The specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
revert enum change
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/c2b8db01..7d792777
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17883&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17883&range=02-03
Stats: 30 lines in 1 file changed: 0 ins; 13 del; 17 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v9]
> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 where > aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored the > in-house cache with WeakHashMap, and removed the Key class as it is no longer > needed (thus the original NPE will no longer be possible). Also with the new > JMH test case, it gains some performance improvement: > > (w/o fix) > > Benchmark Mode Cnt Score Error Units > LocaleCache.testForLanguageTag avgt 20 5781.275 ± 569.580 ns/op > LocaleCache.testLocaleOfavgt 20 62564.079 ± 406.697 ns/op > > (w/ fix) > Benchmark Mode Cnt Score Error Units > LocaleCache.testForLanguageTag avgt 20 4801.175 ± 371.830 ns/op > LocaleCache.testLocaleOfavgt 20 60394.652 ± 352.471 ns/op Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refining comments - Changes: - all: https://git.openjdk.org/jdk/pull/14404/files - new: https://git.openjdk.org/jdk/pull/14404/files/92cf07f4..7c1ca90e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14404&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14404&range=07-08 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14404/head:pull/14404 PR: https://git.openjdk.org/jdk/pull/14404
Integrated: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter
On Wed, 31 Jan 2024 22:24:14 GMT, Justin Lu wrote:
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319344)
> which adds MessageFormat pattern support for the following subformats:
> ListFormat, CompactNumberFormat, and DateTimeFormatter. This change is
> intended to provide pattern support for the more recently added JDK Format
> subclasses, as well as improving java.time formatting within i18n. The draft
> javadoc can be viewed here:
> https://cr.openjdk.org/~jlu/docs/api/java.base/java/text/MessageFormat.html.
> Please see the CSR for more in-depth behavioral changes, as well as
> limitations.
>
> The `FormatTypes`: dtf_date, dtf_time, dtf_datetime, pre-defined
> DateTimeFormatter(s), and list are added.
> The `FormatStyles`: compact_short, compact_long, or, and unit are added.
>
> For example, previously,
>
>
> Object[] args = {LocalDate.of(2023, 11, 16), LocalDate.of(2023, 11, 27)};
> MessageFormat.format("It was {0,date,full}, now it is {1,date,full}", args);
>
>
> would throw `Exception java.lang.IllegalArgumentException: Cannot format
> given Object as a Date`
>
> Now, a user can call
>
>
> MessageFormat.format("It was {0,dtf_date,full}, now it is {1,dtf_date,full}",
> args);
>
>
> which returns "It was Thursday, November 16, 2023, now it is Friday, November
> 17, 2023"
This pull request has now been integrated.
Changeset: 00ffc42c
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/00ffc42cef79d82b2f417c133a48bffec4c7e6b9
Stats: 1258 lines in 7 files changed: 929 ins; 197 del; 132 mod
8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and
DateTimeFormatter
Reviewed-by: naoto, rriggs
-
PR: https://git.openjdk.org/jdk/pull/17663
RFR: JDK-8326530: Widen allowable error bound of Math.tan
Widen acceptable error bound of Math.tan to accommodate the worst-case observed error which is slightly outside of the allowable range. - Commit messages: - JDK-8326530: Widen allowable error bound of Math.tan Changes: https://git.openjdk.org/jdk/pull/17973/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17973&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326530 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17973.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17973/head:pull/17973 PR: https://git.openjdk.org/jdk/pull/17973
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v3]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> This PR includes a lot of cleanup to the applyPattern implementation, the
> specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
improve variable name for StringBuilder
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/d57fa91a..c2b8db01
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17883&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17883&range=01-02
Stats: 15 lines in 1 file changed: 0 ins; 0 del; 15 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
RFR: JDK-8326525: com/sun/tools/attach/BasicTests.java does not verify AgentLoadException case
The change updates the test to throw an exception if expected AgentLoadException is not thrown - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/17971/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17971&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326525 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17971.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17971/head:pull/17971 PR: https://git.openjdk.org/jdk/pull/17971
Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v8]
> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 where > aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored the > in-house cache with WeakHashMap, and removed the Key class as it is no longer > needed (thus the original NPE will no longer be possible). Also with the new > JMH test case, it gains some performance improvement: > > (w/o fix) > > Benchmark Mode Cnt Score Error Units > LocaleCache.testForLanguageTag avgt 20 5781.275 ± 569.580 ns/op > LocaleCache.testLocaleOfavgt 20 62564.079 ± 406.697 ns/op > > (w/ fix) > Benchmark Mode Cnt Score Error Units > LocaleCache.testForLanguageTag avgt 20 4801.175 ± 371.830 ns/op > LocaleCache.testLocaleOfavgt 20 60394.652 ± 352.471 ns/op Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Comment refined. Removed the perf-test as the fix no longer offers perf improvement. - Changes: - all: https://git.openjdk.org/jdk/pull/14404/files - new: https://git.openjdk.org/jdk/pull/14404/files/32ec51f7..92cf07f4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14404&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14404&range=06-07 Stats: 76 lines in 2 files changed: 0 ins; 73 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14404/head:pull/14404 PR: https://git.openjdk.org/jdk/pull/14404
Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v7]
On Thu, 22 Feb 2024 00:14:23 GMT, Naoto Sato wrote: >> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 >> where aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored >> the in-house cache with WeakHashMap, and removed the Key class as it is no >> longer needed (thus the original NPE will no longer be possible). Also with >> the new JMH test case, it gains some performance improvement: >> >> (w/o fix) >> >> Benchmark Mode Cnt Score Error Units >> LocaleCache.testForLanguageTag avgt 20 5781.275 ± 569.580 ns/op >> LocaleCache.testLocaleOfavgt 20 62564.079 ± 406.697 ns/op >> >> (w/ fix) >> Benchmark Mode Cnt Score Error Units >> LocaleCache.testForLanguageTag avgt 20 4801.175 ± 371.830 ns/op >> LocaleCache.testLocaleOfavgt 20 60394.652 ± 352.471 ns/op > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 34 commits: > > - Use ReferencedKeySet.intern() > - Merge branch 'master' into JDK-8309622-Cache-BaseLocale > - Merge branch 'master' into JDK-8309622-Cache-BaseLocale > - Restored the test > - Merge branch 'master' into JDK-8309622-Cache-BaseLocale > - Merge branch 'master' of https://git.openjdk.org/jdk into > JDK-8309622-Cache-BaseLocale > - small cleanup > - Merge branch 'pull/14684' into JDK-8309622-Cache-BaseLocale > - Update ReferencedKeyTest.java > - Simple versions of create > - ... and 24 more: https://git.openjdk.org/jdk/compare/b419e951...32ec51f7 After a hiatus, I now got back to work on this. I think I reflected all the comments (and removed the performance bench because it no longer offers an improvement). - PR Comment: https://git.openjdk.org/jdk/pull/14404#issuecomment-1960073243
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v2]
On Thu, 22 Feb 2024 17:38:50 GMT, Justin Lu wrote:
>> src/java.base/share/classes/java/text/ChoiceFormat.java line 332:
>>
>>> 330:
>>> 331: // Used to explicitly define the segment mode while applying a
>>> pattern
>>> 332: private enum Segment {
>>
>> Do we need a new enum? Would introducing a new enum solve something that
>> cannot be done with the existing implementation?
>
> No, we don't _need_ a new enum, although I prefer it for readability.
>
> For example, I think the following is more clear:
> - `Segment.LIMIT.bldr.toString()`over `segments[1].toString();`.
> - `seg = Segment.LIMIT;` over `part = 0;`
>
> Can also no longer do something such as `part = 2;` even if unlikely.
>
> Although I can see the argument of not wanting an enum as `part` is either
> only `0` or `1`. Either is OK with me.
OK, now the new enum can be shared with multiple threads with this
implementation, but I guess it is OK as ChoiceFormat does not guarantee
synchronization.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1499732561
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v2]
On Thu, 22 Feb 2024 17:55:18 GMT, Justin Lu wrote:
>> Please review this PR which handles an edge case pattern bug with
>> ChoiceFormat.
>>
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar|1.0#"
>> d.format(1) // unexpectedly returns ""
>>
>>
>> Not only does this lead to faulty formatting results, but breaks the
>> ChoiceFormat class invariant of duplicate limits.
>>
>> It would have been preferable if either an exception was thrown when the
>> ChoiceFormat was initially created, or the ChoiceFormat formatting 1
>> returned a value of "bar".
>>
>> For comparison,
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar"
>> d.format(1) // returns "bar"
>>
>>
>> After this change, the first code snippet now returns "bar" when formatting
>> 1 and discards the "baz" as the second code snippet does.
>>
>>
>> This PR includes a lot of cleanup to the applyPattern implementation, the
>> specific fix for this bug is addressed with the following, on line 305,
>>
>> if (seg != Segment.FORMAT) {
>> // Discard incorrect portion and finish building cFmt
>> break;
>> }
>>
>> This prevents a limit/format entry from being built when the current parsing
>> mode has not yet processed the limit and relation. The previous
>> implementation would build an additional limit/format of 1 with an empty
>> string. The `break` allows for the discarding of the incorrect portion and
>> continuing. Alternatively an exception could have been thrown. For
>> consistency with the second code snippet, the former was picked.
>>
>> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
>> semi-related specification fix.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> null check at public level. spacing adjustment.
src/java.base/share/classes/java/text/ChoiceFormat.java line 335:
> 333: FORMAT(new StringBuilder());
> 334:
> 335: private final StringBuilder bldr;
Maybe needs a more meaningful name than `bldr`? At first look, I thought it was
intended for building a segment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1499732912
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Mon, 27 Nov 2023 22:41:25 GMT, Hans Boehm wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Cleaner thread dequeue happens-before running cleaning action > > src/java.base/share/classes/java/lang/ref/Reference.java line 568: > >> 566: * >> 567: * @apiNote >> 568: * Reference processing or finalization may occur whenever the >> virtual machine detects that no > > How about "detects that all needed data from the object is available > elsewhere, and no reference to that object will ever be stored ..." Otherwise > this seems needlessly mysterious to me. I find the additional suggested "detects that all needed data from the object is available elsewhere" more mysterious and confusing. The current wording seems clearer, as it sets the scene for, and motivates, when and why the `rechabilityFence()` might be needed or used. I may be missing the significance of the suggested "all needed data from the object is available elsewhere" at this point in the description. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1499668692
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 01:42:17 GMT, Brent Christian wrote: >> Classes in the `java.lang.ref` package would benefit from an update to bring >> the spec in line with how the VM already behaves. The changes would focus on >> _happens-before_ edges at some key points during reference processing. >> >> A couple key things we want to be able to say are: >> - `Reference.reachabilityFence(x)` _happens-before_ reference processing >> occurs for 'x'. >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the >> registered cleaning action. >> >> This will bring Cleaner in line (or close) with the memory visibility >> guarantees made for finalizers in [JLS >> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): >> _"There is a happens-before edge from the end of a constructor of an object >> to the start of a finalizer (§12.6) for that object."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Cleaner thread dequeue happens-before running cleaning action Looks good; just some casual remarks on verbage & font at a couple of places. - PR Review: https://git.openjdk.org/jdk/pull/16644#pullrequestreview-1896527410
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 12:05:31 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/java/lang/ref/Reference.java line 491:
>>
>>> 489: * If this reference is not registered with a queue, or was
>>> already enqueued
>>> 490: * (by the garbage collector, or a previous call to {@code
>>> enqueue}), this
>>> 491: * method is unnsuccessful and returns false.
>>
>> Suggestion:
>>
>> * method is unsuccessful and returns false.
>
> or, better yet, `fails`
I note that the adjective(s) (un)successful and the adverb(s) (un)successfully
are used at several places in these comments, it might makes sense to use those
terms here as well such that the documentation in internally consistent in its
use of success or failure of actions. In particular, if this terminology is
consistent with precedent in the official JLS spec.
However, I note that there are places where these terms are italicized and
places where they aren't. I am not sure I follow the convention for
italicization. In general, the first use (i.e. introduction) of a term that the
reader might want to pay attention to calls for italicization when documents
are read sequentially, such as in research papers. These javadoc specs will
usually not be read in sequentially. But considering that someone does read
them in order, I'd suggest italicizing only the first use of the term or, if
not, then perhaps none. Alternatively, you might want to italicize all uses
(but why?).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1499714011
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v4]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
wordsmithing
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17856/files
- new: https://git.openjdk.org/jdk/pull/17856/files/00f8179a..f1f1dc41
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17856&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17856&range=02-03
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17856.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
PR: https://git.openjdk.org/jdk/pull/17856
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v2]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> This PR includes a lot of cleanup to the applyPattern implementation, the
> specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
null check at public level. spacing adjustment.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/edfbe7bf..d57fa91a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17883&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17883&range=00-01
Stats: 5 lines in 1 file changed: 2 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern
On Tue, 20 Feb 2024 23:14:26 GMT, Naoto Sato wrote:
>> Please review this PR which handles an edge case pattern bug with
>> ChoiceFormat.
>>
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar|1.0#"
>> d.format(1) // unexpectedly returns ""
>>
>>
>> Not only does this lead to faulty formatting results, but breaks the
>> ChoiceFormat class invariant of duplicate limits.
>>
>> It would have been preferable if either an exception was thrown when the
>> ChoiceFormat was initially created, or the ChoiceFormat formatting 1
>> returned a value of "bar".
>>
>> For comparison,
>>
>> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
>> "0.0#foo|1.0#bar"
>> d.format(1) // returns "bar"
>>
>>
>> After this change, the first code snippet now returns "bar" when formatting
>> 1 and discards the "baz" as the second code snippet does.
>>
>>
>> This PR includes a lot of cleanup to the applyPattern implementation, the
>> specific fix for this bug is addressed with the following, on line 305,
>>
>> if (seg != Segment.FORMAT) {
>> // Discard incorrect portion and finish building cFmt
>> break;
>> }
>>
>> This prevents a limit/format entry from being built when the current parsing
>> mode has not yet processed the limit and relation. The previous
>> implementation would build an additional limit/format of 1 with an empty
>> string. The `break` allows for the discarding of the incorrect portion and
>> continuing. Alternatively an exception could have been thrown. For
>> consistency with the second code snippet, the former was picked.
>>
>> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
>> semi-related specification fix.
>
> src/java.base/share/classes/java/text/ChoiceFormat.java line 332:
>
>> 330:
>> 331: // Used to explicitly define the segment mode while applying a
>> pattern
>> 332: private enum Segment {
>
> Do we need a new enum? Would introducing a new enum solve something that
> cannot be done with the existing implementation?
No, we don't _need_ a new enum, although I prefer it for readability.
For example, I think the following is more clear:
- `Segment.LIMIT.bldr.toString()`over `segments[1].toString();`.
- `seg = Segment.LIMIT;` over `part = 0;`
Can also no longer do something such as `part = 2;` even if unlikely.
Although I can see the argument of not wanting an enum as `part` is either only
`0` or `1`. Either is OK with me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1499626656
Re: RFR: 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test
On Thu, 22 Feb 2024 09:49:31 GMT, Jaikiran Pai wrote: > Can I get a review of this change which proposes to remove the `SystemTest` > from among the `JSR166TestCase`? > > As noted in https://bugs.openjdk.org/browse/JDK-8278527 the > `SystemTest.nanoTime` test has been intermittently failing since a few years > now. Martin and Paul have investigated this failure in the past and they note > that this test is fragile and should be removed: > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14463658&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14463658 > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14508958&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508958 > > The commit in this PR removes the `SystemTest` that was enrolled into > `JSR166TestCase`. > > tier1 testing went fine with this change. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17960#pullrequestreview-1896400193
Re: RFR: 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test
On Thu, 22 Feb 2024 09:49:31 GMT, Jaikiran Pai wrote: > Can I get a review of this change which proposes to remove the `SystemTest` > from among the `JSR166TestCase`? > > As noted in https://bugs.openjdk.org/browse/JDK-8278527 the > `SystemTest.nanoTime` test has been intermittently failing since a few years > now. Martin and Paul have investigated this failure in the past and they note > that this test is fragile and should be removed: > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14463658&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14463658 > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14508958&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508958 > > The commit in this PR removes the `SystemTest` that was enrolled into > `JSR166TestCase`. > > tier1 testing went fine with this change. Marked as reviewed by martin (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17960#pullrequestreview-1896394436
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
> Currently assertEquals has in the failure case sometimes confusing output > like : > > java.lang.RuntimeException: VM output should contain exactly one RTM locking > statistics entry for method > compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >at jdk.test.lib.Asserts.fail(Asserts.java:634) >at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) > > (I don't think we really expected that for some reason 0 equals 1) > This should be improved. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: assertEquals adjust output - Changes: - all: https://git.openjdk.org/jdk/pull/17952/files - new: https://git.openjdk.org/jdk/pull/17952/files/09989d25..03cf3a82 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17952&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17952&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17952/head:pull/17952 PR: https://git.openjdk.org/jdk/pull/17952
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v2]
On Thu, 22 Feb 2024 15:23:56 GMT, Christoph Langer wrote: > I think it is a good idea to improve this. I was irritated by that output > more than once. > > Maybe a better message would be ... _"..." is not equal to "..."_ ? Thanks , I adjusted the output . - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1959752228
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v2]
On Thu, 22 Feb 2024 14:57:05 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust COPYRIGHT year info I think it is a good idea to improve this. I was irritated by that output more than once. Maybe a better message would be ... _"..." is not equal to "..."_ ? - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1959680638
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v2]
> Currently assertEquals has in the failure case sometimes confusing output > like : > > java.lang.RuntimeException: VM output should contain exactly one RTM locking > statistics entry for method > compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >at jdk.test.lib.Asserts.fail(Asserts.java:634) >at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) > > (I don't think we really expected that for some reason 0 equals 1) > This should be improved. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust COPYRIGHT year info - Changes: - all: https://git.openjdk.org/jdk/pull/17952/files - new: https://git.openjdk.org/jdk/pull/17952/files/d7b4de35..09989d25 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17952&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17952&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17952/head:pull/17952 PR: https://git.openjdk.org/jdk/pull/17952
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v6]
> Please review this PR which proposes that we officially deprecate the > following four methods in the `java.util.zip` package: > > * `Inflater.getTotalIn()` > * `Inflater.getTotalOut()` > * `Deflater.getTotalIn()` > * `Deflater.getTotalOut()` > > Since these legacy methods return `int`, they cannot safely return the number > of bytes processed without the risk of losing information about the > magnitude or even sign of the returned value. > > The corresponding methods `getBytesRead()` and `getBytesWritten()` methods > introduced in Java 5 return `long`, and should be used instead when obtaining > this information. > > Unrelated to the deprecation itself, the documentation currently does not > specify what these methods are expected to return when the number of > processed bytes is higher than `Integer.MAX_VALUE`. This PR aims to clarify > this in the API specification. > > Initally, this PR handles only `Inflater.getTotalIn()`. The other three > methods will be updated once the wordsmithing for this method stabilizes. Eirik Bjørsnøs has updated the pull request incrementally with one additional commit since the last revision: Use "highest order bits" instead of "highest bits" - Changes: - all: https://git.openjdk.org/jdk/pull/17919/files - new: https://git.openjdk.org/jdk/pull/17919/files/44323ab0..c683572c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17919&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17919&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17919.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17919/head:pull/17919 PR: https://git.openjdk.org/jdk/pull/17919
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v4]
On Tue, 20 Feb 2024 14:23:35 GMT, Alan Bateman wrote: > > Should these methods specify return values when the number of processed > > bytes exceed Integer.MAX_VALUE? > > I think they should, otherwise it's not clear if the return value is clamped > at Integer.MAX_VALUE. The wording can make it clear that the return value is > useless in cases where the total exceeds Integer.MAX_VALUE. Alan, Finding a good way to express this while being correct and succinct was surprisingly hard. What I have now is probably far from perfect, but at least something to serve as a starting point for review: https://github.com/openjdk/jdk/assets/300291/f258a7f2-c619-44d0-8b2a-78cf3b78aa25";> - PR Comment: https://git.openjdk.org/jdk/pull/17919#issuecomment-1959521558
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v5]
> Please review this PR which proposes that we officially deprecate the > following four methods in the `java.util.zip` package: > > * `Inflater.getTotalIn()` > * `Inflater.getTotalOut()` > * `Deflater.getTotalIn()` > * `Deflater.getTotalOut()` > > Since these legacy methods return `int`, they cannot safely return the number > of bytes processed without the risk of losing information about the > magnitude or even sign of the returned value. > > The corresponding methods `getBytesRead()` and `getBytesWritten()` methods > introduced in Java 5 return `long`, and should be used instead when obtaining > this information. > > Unrelated to the deprecation itself, the documentation currently does not > specify what these methods are expected to return when the number of > processed bytes is higher than `Integer.MAX_VALUE`. This PR aims to clarify > this in the API specification. > > Initally, this PR handles only `Inflater.getTotalIn()`. The other three > methods will be updated once the wordsmithing for this method stabilizes. Eirik Bjørsnøs has updated the pull request incrementally with two additional commits since the last revision: - Remove empty line - Add back a note to specify the long-standing behavior of discarding the highest 32 bits of the return value - Changes: - all: https://git.openjdk.org/jdk/pull/17919/files - new: https://git.openjdk.org/jdk/pull/17919/files/86695619..44323ab0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17919&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17919&range=03-04 Stats: 10 lines in 1 file changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17919.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17919/head:pull/17919 PR: https://git.openjdk.org/jdk/pull/17919
Re: RFR: JDK-8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library [v2]
On Wed, 21 Feb 2024 21:42:08 GMT, Alex Menkov wrote:
>> VirtualMachine.loadAgentPath/loadAgentLibrary can fail with
>> AgentLoadException in 2 cases:
>> - attach listener returns error; in the case the exception is thrown from
>> HotSpotVirtualMachine.processCompletionStatus (called from
>> HotSpotVirtualMachine.execute);
>> - attach listener returns success, but reply does not contain Agent_onAttach
>> return code ("return code: %d" message).
>>
>> before jdk21 if attach listener fails to load required library, it returned
>> error (case 1)
>> from jdk21 attach listener always returns success (case 2)
>> Both cases are ok, but for 2nd case HotSpotVirtualMachine.loadAgentLibrary
>> read only single line of the response message, so exception message didn't
>> contain error details.
>>
>> The fix updates HotSpotVirtualMachine.loadAgentLibrary to read the whole
>> response message.
>> Walking through the code, fixed some other minor things in attach listener
>> and attach API implementation (commented in the code)
>>
>> Testing:
>> - test/jdk/com/sun/tools;
>> - tier1,tier2,tier5-svc
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> uncaught error
src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line
113:
> 111: }
> 112: } else {
> 113: String msg = "Failed to load agent library";
Nit: The line 113 can be moved above the line 99, so the `msg` could be also
used at line 122 instead of a literal use of the string.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17954#discussion_r1499234830
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 01:42:17 GMT, Brent Christian wrote:
>> Classes in the `java.lang.ref` package would benefit from an update to bring
>> the spec in line with how the VM already behaves. The changes would focus on
>> _happens-before_ edges at some key points during reference processing.
>>
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the
>> registered cleaning action.
>>
>> This will bring Cleaner in line (or close) with the memory visibility
>> guarantees made for finalizers in [JLS
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Cleaner thread dequeue happens-before running cleaning action
src/java.base/share/classes/java/lang/ref/Reference.java line 491:
> 489: * If this reference is not registered with a queue, or was already
> enqueued
> 490: * (by the garbage collector, or a previous call to {@code
> enqueue}), this
> 491: * method is unnsuccessful and returns false.
Suggestion:
* method is unsuccessful and returns false.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1499140107
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 12:04:05 GMT, Daniel Jeliński wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Cleaner thread dequeue happens-before running cleaning action
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 491:
>
>> 489: * If this reference is not registered with a queue, or was already
>> enqueued
>> 490: * (by the garbage collector, or a previous call to {@code
>> enqueue}), this
>> 491: * method is unnsuccessful and returns false.
>
> Suggestion:
>
> * method is unsuccessful and returns false.
or, better yet, `fails`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1499141654
Re: RFR: 8325754: Dead AbstractQueuedSynchronizer$ConditionNodes survive minor garbage collections
On Wed, 21 Feb 2024 15:01:15 GMT, Viktor Klang wrote: > More aggressively breaking chains in order to prevent nodes promoted to older > generations standing in the way for collecting younger nodes. I decided that > it was most efficient to add this logic to the else-branch of updating the > firstWaiter and lastWaiter. > > There's a race with unlinkCancelledWaiters() but according to @DougLea it > should be a benign one. > > There's a performance impact of this, but as it is a plain write, and one to > null at that, it should be acceptable. If the race with unlinking non-waiting nodes is benign then this should be okay. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17950#pullrequestreview-1895672384
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Wed, 21 Feb 2024 18:26:18 GMT, Aleksei Efimov wrote: >>> Currently, it is hard to distinguish what part of the test responsible for >>> [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) testing, and >>> which part is for >>> [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). I would prefer >>> to add a new test for the current fix instead: that could be done as >>> additional test mode, OR even better to add a completely new test. >> >> Hm, I think the test was overpurposed already. Creating another test file >> with duplicating code does not sound too good IMHO. Maybe it is acceptable >> without the renaming? >> >> Another question: Do we need a CSR/Release note as there is some behavioral >> change involved (although it should always have been like with this change)? > >> Hm, I think the test was overpurposed already. > > This test was added by JDK-8314063 fix, and I do not think it was change > after that. > >> Creating another test file with duplicating code does not sound too good >> IMHO. Maybe it is acceptable without the renaming? > > I think it is acceptable. Currently, it is hard to separate the test cases > for `a)` the original > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) fix (the opened > socket is not closed properly when connection timeout occurs) and `b)` the > current fix [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) > (unconnected sockets are not supported by SocketFactory). It would be great > to have this distinction in the modified test. I drafted a CSR. @AlekseiEfimov, would be nice if you could review it. As for the test, I had a closer look now and I find it hard to separate testing of [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) from [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). Furthermore, most of the entries test things that hadn't been addressed by any of these two bugs at all. So, [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) is only tested in lines 72, 73, 76 and 77 The original problem of this issue [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) is touched in line 71 and 73. That means, most of the other test invocations test some generic behavior which was never erroneous so far. I could, however, give each line its own test id and annotate the bugs accordingly. Do you think that makes sense? - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1959271452
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern
On Thu, 15 Feb 2024 19:44:42 GMT, Justin Lu wrote:
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> This PR includes a lot of cleanup to the applyPattern implementation, the
> specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
src/java.base/share/classes/java/text/ChoiceFormat.java line 268:
> 266: for (int i = 0; i < newPattern.length(); ++i) {
> 267: char ch = newPattern.charAt(i);
> 268: switch(ch) {
Suggestion:
switch (ch) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1499095669
Integrated: 8326461: tools/jlink/CheckExecutable.java fails as .debuginfo files are not executable
On Thu, 22 Feb 2024 07:23:04 GMT, SendaoYan wrote: > Before JDK-8325342(commit id:0bcece995840777db660811e4b20bb018e90439b), all > the files in build/linux-x86_64-server-release/images/jdk/bin are executable: > >  > > > After JDK-8325342, all the *.debuginfo files in > build/linux-x86_64-server-release/images/jdk/bin are not executable: > >  > > > This PR only modifies the testcase to adapt to the modification of the > corresponding build script, ignoring the check of debuginfo file executable > permissions, and the risk is low This pull request has now been integrated. Changeset: cc1e216e Author:SendaoYan Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/cc1e216eb9e4c817f6744ec76d62f21f4bd14489 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8326461: tools/jlink/CheckExecutable.java fails as .debuginfo files are not executable Reviewed-by: shade, alanb - PR: https://git.openjdk.org/jdk/pull/17958
RFR: 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test
Can I get a review of this change which proposes to remove the `SystemTest` from among the `JSR166TestCase`? As noted in https://bugs.openjdk.org/browse/JDK-8278527 the `SystemTest.nanoTime` test has been intermittently failing since a few years now. Martin and Paul have investigated this failure in the past and they note that this test is fragile and should be removed: https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14463658&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14463658 https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14508958&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508958 The commit in this PR removes the `SystemTest` that was enrolled into `JSR166TestCase`. tier1 testing went fine with this change. - Commit messages: - 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test Changes: https://git.openjdk.org/jdk/pull/17960/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17960&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8278527 Stats: 76 lines in 2 files changed: 0 ins; 76 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17960.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17960/head:pull/17960 PR: https://git.openjdk.org/jdk/pull/17960
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 01:42:17 GMT, Brent Christian wrote: >> Classes in the `java.lang.ref` package would benefit from an update to bring >> the spec in line with how the VM already behaves. The changes would focus on >> _happens-before_ edges at some key points during reference processing. >> >> A couple key things we want to be able to say are: >> - `Reference.reachabilityFence(x)` _happens-before_ reference processing >> occurs for 'x'. >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the >> registered cleaning action. >> >> This will bring Cleaner in line (or close) with the memory visibility >> guarantees made for finalizers in [JLS >> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): >> _"There is a happens-before edge from the end of a constructor of an object >> to the start of a finalizer (§12.6) for that object."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Cleaner thread dequeue happens-before running cleaning action @bchristi-git Made a suggestion, but beyond that this looks great to me. Thanks for putting this together! - PR Comment: https://git.openjdk.org/jdk/pull/16644#issuecomment-1959006509
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 01:42:17 GMT, Brent Christian wrote: >> Classes in the `java.lang.ref` package would benefit from an update to bring >> the spec in line with how the VM already behaves. The changes would focus on >> _happens-before_ edges at some key points during reference processing. >> >> A couple key things we want to be able to say are: >> - `Reference.reachabilityFence(x)` _happens-before_ reference processing >> occurs for 'x'. >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the >> registered cleaning action. >> >> This will bring Cleaner in line (or close) with the memory visibility >> guarantees made for finalizers in [JLS >> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): >> _"There is a happens-before edge from the end of a constructor of an object >> to the start of a finalizer (§12.6) for that object."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Cleaner thread dequeue happens-before running cleaning action src/java.base/share/classes/java/lang/ref/Reference.java line 550: > 548: * strongly > reachable. > 549: * This reachability is assured regardless of any optimizing > transformations > 550: * the VM may perform that might otherwise allow the object to become since "the virtual machine" is spelt out later in the documentation I think we should do the same here. Suggestion: * the virtual machine may perform that might otherwise allow the object to become - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1498900890
Re: RFR: 8326461: tools/jlink/CheckExecutable.java fail after JDK-8325342
On Thu, 22 Feb 2024 07:23:04 GMT, SendaoYan wrote: > Before JDK-8325342(commit id:0bcece995840777db660811e4b20bb018e90439b), all > the files in build/linux-x86_64-server-release/images/jdk/bin are executable: > >  > > > After JDK-8325342, all the *.debuginfo files in > build/linux-x86_64-server-release/images/jdk/bin are not executable: > >  > > > This PR only modifies the testcase to adapt to the modification of the > corresponding build script, ignoring the check of debuginfo file executable > permissions, and the risk is low Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17958#pullrequestreview-1895258187
Re: RFR: 8326461: tools/jlink/CheckExecutable.java fail after JDK-8325342
On Thu, 22 Feb 2024 07:23:04 GMT, SendaoYan wrote: > Before JDK-8325342(commit id:0bcece995840777db660811e4b20bb018e90439b), all > the files in build/linux-x86_64-server-release/images/jdk/bin are executable: > >  > > > After JDK-8325342, all the *.debuginfo files in > build/linux-x86_64-server-release/images/jdk/bin are not executable: > >  > > > This PR only modifies the testcase to adapt to the modification of the > corresponding build script, ignoring the check of debuginfo file executable > permissions, and the risk is low Looks fine to me. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17958#pullrequestreview-1895241663
Re: RFR: 8326461: tools/jlink/CheckExecutable.java fail after JDK-8325342
On Thu, 22 Feb 2024 07:23:04 GMT, SendaoYan wrote: > Before JDK-8325342(commit id:0bcece995840777db660811e4b20bb018e90439b), all > the files in build/linux-x86_64-server-release/images/jdk/bin are executable: > >  > > > After JDK-8325342, all the *.debuginfo files in > build/linux-x86_64-server-release/images/jdk/bin are not executable: > >  > > > This PR only modifies the testcase to adapt to the modification of the > corresponding build script, ignoring the check of debuginfo file executable > permissions, and the risk is low JDK-8326412 changed the executable bit on the debuginfo files but forgot to update this test. - PR Comment: https://git.openjdk.org/jdk/pull/17958#issuecomment-1958947943
