Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Wed, 10 Jul 2024 05:48:23 GMT, David Holmes wrote: >> I'm not so sure this is in fact a bug. If we are throwing with a cause, but >> we can't actually throw and so will do vm_exit, then the exception of >> interest is the cause not the more generic exception that would otherwise >> contain the cause. >> >> Though I have to wonder why there is not an original `_throw` for the >> "cause" exception, that would have triggered the special_exception handling >> anyway? > > Though I see this is inconsistent with `Exceptions::_throw_msg_cause` Okay I think I see how the logic works. If we were going to abort we would never reach `_throw_cause` as the initial `_throw` would have exited. But for the `!thread->can_call_Java()` case the original `_throw` would replace the intended real exception with the dummy `VM_exception()`, which is then "caught" and we try to replace with a more specific exception to be thrown via `throw_cause`, which will again replace whichever exception is requested with the dummy `VM_exception()` - so the end result is we will throw the dummy regardless of whether the cause or wrapping exception is specified. So your fix here makes sense. - PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1671680471
Re: RFR: 8335637: Add explicit non-null return value expectations to Object.toString() [v4]
On Wed, 10 Jul 2024 05:02:36 GMT, Joe Darcy wrote:
>> Make well-behaved implementation expectations of Object.{toString, hashCode}
>> explicit.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Narrow scope of the change.
Reducing this to the clarification of toString is good.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20063#pullrequestreview-2168127871
Re: RFR: 8334057: JLinkReproducibleTest.java support receive test.tool.vm.opts
On Wed, 12 Jun 2024 02:00:41 GMT, SendaoYan wrote: > Hi all, > Currently, the testcase `test/jdk/tools/jlink/JLinkReproducibleTest.java` > doesn't receive jvm options from jtreg. > I think it's necessory to receive jvm options from jtreg. > The change has been verified, only change the testacase, the risk is low. Hi, can anyone take look this PR. - PR Comment: https://git.openjdk.org/jdk/pull/19669#issuecomment-2219642848
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Wed, 10 Jul 2024 05:46:31 GMT, David Holmes wrote:
>> src/hotspot/share/utilities/exceptions.cpp line 208:
>>
>>> 206: Handle h_loader, Handle
>>> h_protection_domain) {
>>> 207: // Check for special boot-strapping/compiler-thread handling
>>> 208: if (special_exception(thread, file, line, h_cause)) return;
>>
>> This fixes a long standing bug where `special_exception` is being queried
>> with the *cause* of the exception being thrown instead of the *name* of the
>> exception being thrown.
>
> I'm not so sure this is in fact a bug. If we are throwing with a cause, but
> we can't actually throw and so will do vm_exit, then the exception of
> interest is the cause not the more generic exception that would otherwise
> contain the cause.
>
> Though I have to wonder why there is not an original `_throw` for the "cause"
> exception, that would have triggered the special_exception handling anyway?
Though I see this is inconsistent with `Exceptions::_throw_msg_cause`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1671653968
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Mon, 8 Jul 2024 19:09:47 GMT, Doug Simon wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed TestTranslatedException
>
> src/hotspot/share/utilities/exceptions.cpp line 208:
>
>> 206: Handle h_loader, Handle
>> h_protection_domain) {
>> 207: // Check for special boot-strapping/compiler-thread handling
>> 208: if (special_exception(thread, file, line, h_cause)) return;
>
> This fixes a long standing bug where `special_exception` is being queried
> with the *cause* of the exception being thrown instead of the *name* of the
> exception being thrown.
I'm not so sure this is in fact a bug. If we are throwing with a cause, but we
can't actually throw and so will do vm_exit, then the exception of interest is
the cause not the more generic exception that would otherwise contain the cause.
Though I have to wonder why there is not an original `_throw` for the "cause"
exception, that would have triggered the special_exception handling anyway?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1671652583
Re: RFR: 8335637: Add explicit non-null return value expectations to Object.toString() [v4]
On Wed, 10 Jul 2024 05:02:36 GMT, Joe Darcy wrote:
>> Make well-behaved implementation expectations of Object.{toString, hashCode}
>> explicit.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Narrow scope of the change.
I'd like to get portions of this change in JDK 23 as well as 24. As the quality
of implementation discussion on cyclic data structures, etc. looks like it
needs more discussion before reaching consensus, I've filed JDK-8336043 for the
broader quality of implementation discussion in JDK 24.
-
PR Comment: https://git.openjdk.org/jdk/pull/20063#issuecomment-2219563751
Re: RFR: 8335637: Add explicit non-null return value expectations to Object.toString() [v4]
On Wed, 10 Jul 2024 05:02:36 GMT, Joe Darcy wrote:
>> Make well-behaved implementation expectations of Object.{toString, hashCode}
>> explicit.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Narrow scope of the change.
The proposed reduction of the scope of the change to just note that
`toString()` should return a non-null value looks good to me.
-
Marked as reviewed by jpai (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20063#pullrequestreview-2168038159
Re: RFR: 8335637: Add explicit non-null return value expectations to Object.toString() [v4]
> Make well-behaved implementation expectations of Object.{toString, hashCode}
> explicit.
Joe Darcy has updated the pull request incrementally with one additional commit
since the last revision:
Narrow scope of the change.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20063/files
- new: https://git.openjdk.org/jdk/pull/20063/files/66679fb8..ee007c3e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20063&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20063&range=02-03
Stats: 13 lines in 1 file changed: 0 ins; 13 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/20063.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20063/head:pull/20063
PR: https://git.openjdk.org/jdk/pull/20063
Re: RFR: 8333396: Use StringBuilder internally for java.text.Format.* formatting [v19]
> ### Performance regression of DecimalFormat.format
> From the output of perf, we can see the hottest regions contain atomic
> instructions. But when run with JDK 11, there is no such problem. The reason
> is the removed biased locking.
> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
> contains many synchronized methods.
> So I added support for some new methods that accept StringBuilder which is
> lock-free.
>
> ### Benchmark testcase
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> private DecimalFormat format;
>
> @Setup(Level.Trial)
> public void setup() {
> format = new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testNewAndFormat() throws InterruptedException {
> new DecimalFormat("#0.0").format(9524234.1236457);
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testFormatOnly() throws InterruptedException {
> format.format(9524234.1236457);
> }
> }
>
>
> ### Test result
> Current JDK before optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> Current JDK after optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> ### JDK 11
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
lingjun-cg has updated the pull request incrementally with one additional
commit since the last revision:
896: Use StringBuilder internally for java.text.Format.* formatting
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19513/files
- new: https://git.openjdk.org/jdk/pull/19513/files/5fda8747..6573e413
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=18
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=17-18
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19513.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19513/head:pull/19513
PR: https://git.openjdk.org/jdk/pull/19513
Re: RFR: 8325679: Optimize ArrayList subList sort [v4]
On Mon, 19 Feb 2024 10:05:24 GMT, Attila Szegedi wrote: >> Somewhat surprisingly, `ArrayList$Sublist.sort()` is not specialized and >> will thus fall back to slower default method of `List.sort()` instead of >> sorting a range of the array in-place in its backing root `ArrayList`. >> >> This doesn't change observable behavior, so haven't added tests, and `tier1` >> tests still all pass except for >> `test/jdk/java/util/Locale/LocaleProvidersFormat.java` which also currently >> fails on master too on the machine I tested on. > > Attila Szegedi has updated the pull request incrementally with one additional > commit since the last revision: > > Remove test Can you bump the copyright year of `ArrayList.java`? Since `ListDefaults` covers your change, can you add your bugid here https://github.com/openjdk/jdk/blob/1472124489c841642996ae984e21c533ffec8091/test/jdk/java/util/List/ListDefaults.java#L53 and bump the copyright year as well? - PR Review: https://git.openjdk.org/jdk/pull/17818#pullrequestreview-2167684628
Re: RFR: 8335905: CompoundElement API cleanup [v2]
> `CompoundElement` already inherits `Iterable` and its `forEach(Consumer)`, > thus we can remove `Iterable elements()` and `forEachElements(Consumer)`. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Two usages in tier3 - Changes: - all: https://git.openjdk.org/jdk/pull/20103/files - new: https://git.openjdk.org/jdk/pull/20103/files/ea069e81..9caa520d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20103&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20103&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20103.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20103/head:pull/20103 PR: https://git.openjdk.org/jdk/pull/20103
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v6]
On Tue, 9 Jul 2024 22:49:30 GMT, Liam Miller-Cushon wrote:
>> This change overrides mutator methods in the implementation returned by
>> `Map.of().entrySet()` to throw `UnsupportedOperationException`.
>
> Liam Miller-Cushon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update copyright year
>
> and add the bug number to the modified test
Marked as reviewed by liach (Committer).
test/jdk/java/util/Collection/MOAT.java line 541:
> 539: */
> 540: private static void testCollMutatorsAlwaysThrow(Collection
> c) {
> 541: testCollMutatorsAlwaysThrow(c, ABSENT_VALUE);
Suggestion:
testCollMutatorsAlwaysThrow(c, ABSENT_VALUE);
-
PR Review: https://git.openjdk.org/jdk/pull/18522#pullrequestreview-2167636479
PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1671325840
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v7]
> This change overrides mutator methods in the implementation returned by > `Map.of().entrySet()` to throw `UnsupportedOperationException`. Liam Miller-Cushon has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/util/Collection/MOAT.java Co-authored-by: Chen Liang - Changes: - all: https://git.openjdk.org/jdk/pull/18522/files - new: https://git.openjdk.org/jdk/pull/18522/files/31dc4f5e..3acac325 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18522.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18522/head:pull/18522 PR: https://git.openjdk.org/jdk/pull/18522
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v6]
> This change overrides mutator methods in the implementation returned by > `Map.of().entrySet()` to throw `UnsupportedOperationException`. Liam Miller-Cushon has updated the pull request incrementally with one additional commit since the last revision: Update copyright year and add the bug number to the modified test - Changes: - all: https://git.openjdk.org/jdk/pull/18522/files - new: https://git.openjdk.org/jdk/pull/18522/files/8327a8e1..31dc4f5e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=04-05 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18522.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18522/head:pull/18522 PR: https://git.openjdk.org/jdk/pull/18522
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v5]
On Tue, 9 Jul 2024 22:43:05 GMT, Chen Liang wrote: > All three files can have their copyright year updated. Done - PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-2218861123
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v5]
On Tue, 9 Jul 2024 22:40:41 GMT, Liam Miller-Cushon wrote: >> This change overrides mutator methods in the implementation returned by >> `Map.of().entrySet()` to throw `UnsupportedOperationException`. > > Liam Miller-Cushon has updated the pull request incrementally with one > additional commit since the last revision: > > Update unmodifiable map javadoc All three files can have their copyright year updated. - PR Review: https://git.openjdk.org/jdk/pull/18522#pullrequestreview-2167632144
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException
On Tue, 9 Jul 2024 22:23:57 GMT, Chen Liang wrote: > Would you mind changing "Calling any mutator method on the Map will ..." to > something like "... on the map or any derived view collection will ..." to > emphasize our new consistency? (Our internal conversation agreed on this) Done! Kevin made a similar note [in the CSR](https://bugs.openjdk.org/browse/JDK-8329284?focusedId=14688403&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14688403). I have updated the CSR to mention the javadoc change. - PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-2218846679
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v5]
> This change overrides mutator methods in the implementation returned by > `Map.of().entrySet()` to throw `UnsupportedOperationException`. Liam Miller-Cushon has updated the pull request incrementally with one additional commit since the last revision: Update unmodifiable map javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/18522/files - new: https://git.openjdk.org/jdk/pull/18522/files/4c9a511f..8327a8e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18522.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18522/head:pull/18522 PR: https://git.openjdk.org/jdk/pull/18522
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException
On Fri, 26 Apr 2024 18:20:18 GMT, Liam Miller-Cushon wrote: >> This change overrides mutator methods in the implementation returned by >> `Map.of().entrySet()` to throw `UnsupportedOperationException`. > > Please keep this open @cushon Given we have specified in `Collection` that unmodifiability extends to all derived collection views: https://github.com/openjdk/jdk/blob/4c9a511f75174a3f3ae575fae2677c198c607b52/src/java.base/share/classes/java/util/Collection.java#L159-L160 Would you mind changing "Calling any mutator method on the Map will ..." to something like "... on the map or any derived view collection will ..." to emphasize our new consistency? (Our internal conversation agreed on this) https://github.com/openjdk/jdk/blob/4c9a511f75174a3f3ae575fae2677c198c607b52/src/java.base/share/classes/java/util/Map.java#L125-L126 - PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-2218833548
RFR: 8335905: CompoundElement API cleanup
`CompoundElement` already inherits `Iterable` and its `forEach(Consumer)`, thus we can remove `Iterable elements()` and `forEachElements(Consumer)`. - Commit messages: - Missed tests - 8335905: CompoundElement API cleanup Changes: https://git.openjdk.org/jdk/pull/20103/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20103&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335905 Stats: 83 lines in 32 files changed: 2 ins; 11 del; 70 mod Patch: https://git.openjdk.org/jdk/pull/20103.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20103/head:pull/20103 PR: https://git.openjdk.org/jdk/pull/20103
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v4]
On Tue, 9 Jul 2024 16:12:58 GMT, Vanitha B P wrote: >> Created jtreg test case for >> [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue. >> >> The JpackageTest created tests that the child process started from the app >> launched by jpackage launcher is not automatically terminated when the the >> launcher is terminated. > > Vanitha B P has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed the review comment Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19536#pullrequestreview-2167519424
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v3]
On Mon, 8 Jul 2024 16:21:16 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Add JVMCI symbol exports
> - Revert "More graceful JVMCI VM option interaction"
>
>This reverts commit 2814350370cf142e130fe1d38610c646039f976d.
This is really great work, Axel! I've been reading this code for a while, and
have done one pass looking through the PR with a few comments.
src/hotspot/share/opto/library_call.cpp line 4620:
> 4618: Node *unlocked_val = _gvn.MakeConX(markWord::unlocked_value);
> 4619: Node *chk_unlocked = _gvn.transform(new
> CmpXNode(lmasked_header, unlocked_val));
> 4620: Node *test_not_unlocked = _gvn.transform(new
> BoolNode(chk_unlocked, BoolTest::ne));
I don't really know what this does. Someone from the c2 compiler group should
look at this.
src/hotspot/share/runtime/arguments.cpp line 1830:
> 1828: FLAG_SET_CMDLINE(LockingMode, LM_LIGHTWEIGHT);
> 1829: warning("UseObjectMonitorTable requires LM_LIGHTWEIGHT");
> 1830: }
Maybe we want this to have the opposite sense - turn off UseObjectMonitorTable
if not LM_LIGHTWEIGHT?
src/hotspot/share/runtime/javaThread.inline.hpp line 258:
> 256: }
> 257:
> 258: _om_cache.clear();
This could be shorter, ie: if (UseObjectMonitorTable) _om_cache.clear();
I think the not having an assert was to make the caller unconditional, which is
good.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 393:
> 391:
> 392: ObjectMonitor* LightweightSynchronizer::get_or_insert_monitor(oop
> object, JavaThread* current, const ObjectSynchronizer::InflateCause cause,
> bool try_read) {
> 393: assert(LockingMode == LM_LIGHTWEIGHT, "must be");
This assert should be assert(UseObjectMonitorTable not LM_LIGHTWEIGHT).
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 732:
> 730:
> 731: markWord mark = object->mark();
> 732: assert(!mark.is_unlocked(), "must be unlocked");
"must be locked" makes more sense.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 763:
> 761: assert(mark.has_monitor(), "must be");
> 762: // The monitor exists
> 763: ObjectMonitor* monitor = ObjectSynchronizer::read_monitor(current,
> object, mark);
This looks in the table for the monitor in U
Re: RFR: 8335182: Consolidate and streamline String concat code shapes [v3]
> This PR attains a speed-up on some microbenchmarks by simplifying how we > build up the MH combinator tree shape > (only use prependers with prefix, always add a suffix to the newArray > combinator), then simplifying/inlining some of the code in the helper > functions. > > Many simple concatenation expressions stay performance neutral, while the win > comes from enabling C2 to better optimize more complex shapes > (concat13String, concatMix4String, concatConst6String see relatively large > improvements): > > > NameCnt Base Error Test > Error Unit Change > StringConcat.concat13String 50 66.380 ± 0.18953.017 ± > 0.241 ns/op 1.25x (p = 0.000*) > StringConcat.concat4String 50 13.620 ± 0.05312.382 ± > 0.089 ns/op 1.10x (p = 0.000*) > StringConcat.concat6String 50 17.168 ± 0.07016.046 ± > 0.064 ns/op 1.07x (p = 0.000*) > StringConcat.concatConst2String 509.820 ± 0.081 8.158 ± > 0.041 ns/op 1.20x (p = 0.000*) > StringConcat.concatConst4String 50 14.938 ± 0.12412.800 ± > 0.049 ns/op 1.17x (p = 0.000*) > StringConcat.concatConst6Object 50 56.115 ± 0.28854.046 ± > 0.214 ns/op 1.04x (p = 0.000*) > StringConcat.concatConst6String 50 19.032 ± 0.07316.213 ± > 0.093 ns/op 1.17x (p = 0.000*) > StringConcat.concatConstBoolByte 508.486 ± 0.066 8.556 ± > 0.050 ns/op 0.99x (p = 0.004*) > StringConcat.concatConstInt 508.942 ± 0.052 7.677 ± > 0.029 ns/op 1.16x (p = 0.000*) > StringConcat.concatConstIntConstInt 50 12.883 ± 0.07012.431 ± > 0.070 ns/op 1.04x (p = 0.000*) > StringConcat.concatConstString 507.523 ± 0.050 7.486 ± > 0.044 ns/op 1.00x (p = 0.058 ) > StringConcat.concatConstStringConstInt 50 11.961 ± 0.03211.699 ± > 0.049 ns/op 1.02x (p = 0.000*) > StringConcat.concatEmptyConstInt 507.778 ± 0.038 7.723 ± > 0.037 ns/op 1.01x (p = 0.000*) > StringConcat.concatEmptyConstString 503.506 ± 0.026 3.505 ± > 0.015 ns/op 1.00x (p = 0.930 ) > StringConcat.concatEmptyLeft 503.573 ± 0.075 3.518 ± > 0.057 ns/op 1.02x (p = 0.044 ) > StringConcat.concatEmptyRight503.713 ± 0.049 3.622 ± > 0.053 ns/op 1.02x (p = 0.000*) > StringConcat.concatMethodConstString 507.418 ± 0.030 7.478 ± > 0.066 ns/op 0.99x (p = 0.005*) > StringConcat.concatMix4String... Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Excess blankspace - Changes: - all: https://git.openjdk.org/jdk/pull/19927/files - new: https://git.openjdk.org/jdk/pull/19927/files/7c50d7dd..58fe4b12 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19927&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19927&range=01-02 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19927.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19927/head:pull/19927 PR: https://git.openjdk.org/jdk/pull/19927
Re: RFR: 8335668: NumberFormat integer only parsing should throw exception for edge case [v2]
On Tue, 9 Jul 2024 19:41:29 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reflect review
>
> test/jdk/java/text/Format/NumberFormat/LenientParseTest.java line 146:
>
>> 144: @EnabledIfSystemProperty(named = "user.language", matches = "en")
>> 145: public void compactIntegerParseOnlyFractionOnlyTest() {
>> 146: var fmt = NumberFormat.getIntegerInstance();
>
> Should it get a compact number instance, instead of integer?
Thanks for catching, I would hope the compact test actually tests a compact
format; fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20101#discussion_r1671197396
Re: RFR: 8335668: NumberFormat integer only parsing should throw exception for edge case [v2]
> Please review this PR which corrects a case in NumberFormat integer only
> parsing.
>
> [JDK-8333755](https://bugs.openjdk.org/browse/JDK-8333755) fixed integer only
> parsing when the value has a suffix, although it caused incorrect behavior
> for the following case: when the parsed string does not contain an integer
> portion, and the format has integer only parsing, parsing should fail,
> instead of 0 being returned. For example,
>
>
> var fmt = NumberFormat.getIntegerInstance();
> fmt.parse(".5", new ParsePosition(0)); // should return null, not 0
>
>
> The changes to the _badParseStrings_ data provider in _StrictParseTest.java_
> are needed since those cases _should_ fail in different indexes depending on
> if integer parsing is enabled. Thus, they were updated to ensure they fail
> for both integer and non-integer parsing with the same errorIndex.
>
> In the fix itself, I also updated the initial value of `intIndex` to -1 from
> 0, to provide better clarity.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
reflect review
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20101/files
- new: https://git.openjdk.org/jdk/pull/20101/files/522b8381..a11d3468
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20101&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20101&range=00-01
Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/20101.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20101/head:pull/20101
PR: https://git.openjdk.org/jdk/pull/20101
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v23]
> I have implemented the Zimmermann's square root algorithm, available in works > [here](https://inria.hal.science/inria-00072854/en/) and > [here](https://www.researchgate.net/publication/220532560_A_proof_of_GMP_square_root). > > The algorithm is proved to be asymptotically faster than the Newton's Method, > even for small numbers. To get an idea of how much the Newton's Method is > slow, consult my article [here](https://arxiv.org/abs/2406.07751), in which > I compare Newton's Method with a version of classical square root algorithm > that I implemented. After implementing Zimmermann's algorithm, it turns out > that it is faster than my algorithm even for small numbers. fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Added comment on normalization in MutableBigInteger.sqrtRemZimmermann() - Changes: - all: https://git.openjdk.org/jdk/pull/19710/files - new: https://git.openjdk.org/jdk/pull/19710/files/9aa7fdca..95be919e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19710&range=22 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19710&range=21-22 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19710/head:pull/19710 PR: https://git.openjdk.org/jdk/pull/19710
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v22]
> I have implemented the Zimmermann's square root algorithm, available in works > [here](https://inria.hal.science/inria-00072854/en/) and > [here](https://www.researchgate.net/publication/220532560_A_proof_of_GMP_square_root). > > The algorithm is proved to be asymptotically faster than the Newton's Method, > even for small numbers. To get an idea of how much the Newton's Method is > slow, consult my article [here](https://arxiv.org/abs/2406.07751), in which > I compare Newton's Method with a version of classical square root algorithm > that I implemented. After implementing Zimmermann's algorithm, it turns out > that it is faster than my algorithm even for small numbers. fabioromano1 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 53 additional commits since the last revision: - Merge branch 'openjdk:master' into patchSqrt - Ensure normalized value in MutableBigInteger initialization with ints - Merge branch 'openjdk:master' into patchSqrt - Added documentation - An optimization - Reverted changes in BigDecimal - Merge branch 'openjdk:master' into patchSqrt - Added "throw" to throw the ArithmeticException created - Correct BigDecimal.sqrt() - Simplified computing square root of BigDecimal using BigInteger.sqrt() - ... and 43 more: https://git.openjdk.org/jdk/compare/aa8fe60e...9aa7fdca - Changes: - all: https://git.openjdk.org/jdk/pull/19710/files - new: https://git.openjdk.org/jdk/pull/19710/files/b28f034b..9aa7fdca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19710&range=21 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19710&range=20-21 Stats: 11590 lines in 439 files changed: 7143 ins; 2562 del; 1885 mod Patch: https://git.openjdk.org/jdk/pull/19710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19710/head:pull/19710 PR: https://git.openjdk.org/jdk/pull/19710
Re: RFR: 8335182: Consolidate and streamline String concat code shapes [v2]
On Fri, 28 Jun 2024 12:39:32 GMT, Claes Redestad wrote: >> This PR attains a speed-up on some microbenchmarks by simplifying how we >> build up the MH combinator tree shape >> (only use prependers with prefix, always add a suffix to the newArray >> combinator), then simplifying/inlining some of the code in the helper >> functions. >> >> Many simple concatenation expressions stay performance neutral, while the >> win comes from enabling C2 to better optimize more complex shapes >> (concat13String, concatMix4String, concatConst6String see relatively large >> improvements): >> >> >> NameCnt Base Error Test >> Error Unit Change >> StringConcat.concat13String 50 66.380 ± 0.18953.017 ± >> 0.241 ns/op 1.25x (p = 0.000*) >> StringConcat.concat4String 50 13.620 ± 0.05312.382 ± >> 0.089 ns/op 1.10x (p = 0.000*) >> StringConcat.concat6String 50 17.168 ± 0.07016.046 ± >> 0.064 ns/op 1.07x (p = 0.000*) >> StringConcat.concatConst2String 509.820 ± 0.081 8.158 ± >> 0.041 ns/op 1.20x (p = 0.000*) >> StringConcat.concatConst4String 50 14.938 ± 0.12412.800 ± >> 0.049 ns/op 1.17x (p = 0.000*) >> StringConcat.concatConst6Object 50 56.115 ± 0.28854.046 ± >> 0.214 ns/op 1.04x (p = 0.000*) >> StringConcat.concatConst6String 50 19.032 ± 0.07316.213 ± >> 0.093 ns/op 1.17x (p = 0.000*) >> StringConcat.concatConstBoolByte 508.486 ± 0.066 8.556 ± >> 0.050 ns/op 0.99x (p = 0.004*) >> StringConcat.concatConstInt 508.942 ± 0.052 7.677 ± >> 0.029 ns/op 1.16x (p = 0.000*) >> StringConcat.concatConstIntConstInt 50 12.883 ± 0.07012.431 ± >> 0.070 ns/op 1.04x (p = 0.000*) >> StringConcat.concatConstString 507.523 ± 0.050 7.486 ± >> 0.044 ns/op 1.00x (p = 0.058 ) >> StringConcat.concatConstStringConstInt 50 11.961 ± 0.03211.699 ± >> 0.049 ns/op 1.02x (p = 0.000*) >> StringConcat.concatEmptyConstInt 507.778 ± 0.038 7.723 ± >> 0.037 ns/op 1.01x (p = 0.000*) >> StringConcat.concatEmptyConstString 503.506 ± 0.026 3.505 ± >> 0.015 ns/op 1.00x (p = 0.930 ) >> StringConcat.concatEmptyLeft 503.573 ± 0.075 3.518 ± >> 0.057 ns/op 1.02x (p = 0.044 ) >> StringConcat.concatEmptyRight503.713 ± 0.049 3.622 ± >> 0.053 ns/op 1.02x (p = 0.000*) >> StringConcat.concatMethodConstString 507.418 ± 0.030 7.478 ± >> 0.066 ns/op 0.99x (p... > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Copyrights test/micro/org/openjdk/bench/java/lang/StringConcatStartup.java line 182: > 180: concat = "" + "S" + "S" + c + d + z + l + d + z + f + b + d > + z + S + f; > 181: concat = "" + b + d + z + d + i + z + d + b + d + "S" + c + > f + d; > 182: concat = "" + d + s + f + c + i + "S" + b + b + S + i + s + > d + "S" + f; Suggestion: concat = "" + d + s + f + c + i + "S" + b + b + S + i + s + d + "S" + f; test/micro/org/openjdk/bench/java/lang/StringConcatStartup.java line 191: > 189: concat = "" + z + S + S + "S" + S + S + z + b + S + z + b + > f + s + l; > 190: concat = "" + s + z + d + "S" + z + l + f + z + s + z + d + > l + s + l; > 191: concat = "" + l + d + i + s + i + c + i + f + b + f + s + b > + s + s; Suggestion: concat = "" + l + d + i + s + i + c + i + f + b + f + s + b + s + s; test/micro/org/openjdk/bench/java/lang/StringConcatStartup.java line 193: > 191: concat = "" + l + d + i + s + i + c + i + f + b + f + s + b > + s + s; > 192: concat = "" + z + "S" + S + "S" + "S" + i + "S" + s + d + z > + l; > 193: concat = "" + i + S + S + "S" + f + "S" + "S" + z + S + z + > b + z + c + b; Suggestion: concat = "" + i + S + S + "S" + f + "S" + "S" + z + S + z + b + z + c + b; test/micro/org/openjdk/bench/java/lang/StringConcatStartup.java line 223: > 221: concat = "" + "S" + f + S + i + i + i + "S" + i + i + l + c > + l + S + S + z + b + i + c + f + S; > 222: concat = "" + c + z + S + S + b + i + c; > 223: concat = "" + S + s + S + c; Suggestion: concat = "" + S + s + S + c; test/micro/org/openjdk/bench/java/lang/StringConcatStartup.java line 310: > 308: concat = "" + "S" + "S" + c + d + z + l + d + z + f + b + d > + z + S + f; > 309: concat = "" + b + d + z + d + i + z + d + b + d + "S" + c + > f + d; > 310: concat = "" + d + s + f + c + i + "S" + b + b + S + i + s + > d + "S" + f; Suggestion: concat = "" + d + s + f + c + i + "S" + b + b + S + i + s + d + "S" + f; test/m
Re: RFR: 8335668: NumberFormat integer only parsing should throw exception for edge case
On Tue, 9 Jul 2024 18:42:25 GMT, Justin Lu wrote:
> Please review this PR which corrects a case in NumberFormat integer only
> parsing.
>
> [JDK-8333755](https://bugs.openjdk.org/browse/JDK-8333755) fixed integer only
> parsing when the value has a suffix, although it caused incorrect behavior
> for the following case: when the parsed string does not contain an integer
> portion, and the format has integer only parsing, parsing should fail,
> instead of 0 being returned. For example,
>
>
> var fmt = NumberFormat.getIntegerInstance();
> fmt.parse(".5", new ParsePosition(0)); // should return null, not 0
>
>
> The changes to the _badParseStrings_ data provider in _StrictParseTest.java_
> are needed since those cases _should_ fail in different indexes depending on
> if integer parsing is enabled. Thus, they were updated to ensure they fail
> for both integer and non-integer parsing with the same errorIndex.
>
> In the fix itself, I also updated the initial value of `intIndex` to -1 from
> 0, to provide better clarity.
The fix looks good. A couple of comments on tests.
test/jdk/java/text/Format/NumberFormat/LenientParseTest.java line 146:
> 144: @EnabledIfSystemProperty(named = "user.language", matches = "en")
> 145: public void compactIntegerParseOnlyFractionOnlyTest() {
> 146: var fmt = NumberFormat.getIntegerInstance();
Should it get a compact number instance, instead of integer?
test/jdk/java/text/Format/NumberFormat/StrictParseTest.java line 221:
> 219: @EnabledIfSystemProperty(named = "user.language", matches = "en")
> 220: public void compactIntegerParseOnlyFractionOnlyTest() {
> 221: var fmt = NumberFormat.getIntegerInstance();
same here
-
PR Review: https://git.openjdk.org/jdk/pull/20101#pullrequestreview-2167273474
PR Review Comment: https://git.openjdk.org/jdk/pull/20101#discussion_r1671085244
PR Review Comment: https://git.openjdk.org/jdk/pull/20101#discussion_r1671086182
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException [v3]
On Tue, 9 Jul 2024 19:19:41 GMT, Liam Miller-Cushon wrote: >> There is `testImmutableCollection`/`testImmutableSet` that takes an >> arbitrary nonexistent item for insertion/removal: >> https://github.com/openjdk/jdk/blob/598af2e51464be089b64da4024e62865c2c6ec72/test/jdk/java/util/Collection/MOAT.java#L665 >> I think a refactor of a generic `testCollMutatorsAlwaysThrow(Collection >> c, T t)` and delegating the original Integer version to call >> `testCollMutatorsAlwaysThrow(c, ABSENT_VALUE)` would not be invasive. > > Thanks! Done. > > That pointed out that the mutators on `keySet()` and `values()` were not > throwing UOE, so I have tentatively updated the PR to also fix that. Thank you for this more comprehensive update! Definitely an improvement to bring the UOE behavior to all 3 of these view collections. - PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1671043314
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException [v4]
> This change overrides mutator methods in the implementation returned by > `Map.of().entrySet()` to throw `UnsupportedOperationException`. Liam Miller-Cushon 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 eight additional commits since the last revision: - Also throw UOE for mutators on keySet() and values() and add more test coverage to MOAT. - Merge remote-tracking branch 'origin/master' into JDK-8328821-make-clear-consistent - Merge remote-tracking branch 'origin/master' into JDK-8328821-make-clear-consistent - Check m.entrySet().hashCode() in MOAT - Merge remote-tracking branch 'origin/master' into JDK-8328821-make-clear-consistent - Use AbstractImmutableSet - Throw UOE for all Map.of().entrySet() mutator methods - 8328821: Make the ImmutableCollections clear() call consistent Without overriding clear(), a call to it in an empty map would just return, as iterator.hasNext() would be false. However if calling Map.of().clear() throws an exception. To make the behavior of Map.of().entrySet().clear() consistent, we need to have an implementation of clear() for the entry set that throws as well. - Changes: - all: https://git.openjdk.org/jdk/pull/18522/files - new: https://git.openjdk.org/jdk/pull/18522/files/598af2e5..4c9a511f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18522&range=02-03 Stats: 30615 lines in 772 files changed: 17927 ins; 10197 del; 2491 mod Patch: https://git.openjdk.org/jdk/pull/18522.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18522/head:pull/18522 PR: https://git.openjdk.org/jdk/pull/18522
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException [v3]
On Tue, 9 Jul 2024 17:43:32 GMT, Chen Liang wrote: >> `testCollMutatorsAlwaysThrow` expects a `Collection` (not e.g. a >> `Collection>`). MOAT could be refactored to handle >> that case. Do you think that's worth it, or have thoughts about what the >> cleanest way to do that would be? > > There is `testImmutableCollection`/`testImmutableSet` that takes an arbitrary > nonexistent item for insertion/removal: > https://github.com/openjdk/jdk/blob/598af2e51464be089b64da4024e62865c2c6ec72/test/jdk/java/util/Collection/MOAT.java#L665 > I think a refactor of a generic `testCollMutatorsAlwaysThrow(Collection c, > T t)` and delegating the original Integer version to call > `testCollMutatorsAlwaysThrow(c, ABSENT_VALUE)` would not be invasive. Thanks! Done. That pointed out that the mutators on `keySet()` and `values()` were not throwing UOE, so I have tentatively updated the PR to also fix that. - PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1671039819
Re: More useful structured concurrency stack traces
Probably best to bring this to loom-dev as there have been some exploration into but where we decided not to expose any APIs at this time. -Alan On 09/07/2024 19:50, Louis Wasserman wrote: My understanding of the structured concurrency APIs now in preview is that when a subtask is forked, exceptions thrown in that stack trace will have stack traces going up to the beginning of that subtask, not e.g. up the structured concurrency task tree. (My tests suggest this is the case for simple virtual threads without structured concurrency.) Most concurrency frameworks on the JVM that I’ve encountered share the property that stack traces for exceptions don’t trace through the entire causal chain – and, not unrelatedly, that developers struggle to debug concurrent applications, especially with stack traces from production and not full debuggers attached. In some cases, like chained CompletableFutures, this seems necessary to ensure that executing what amounts to a loop does not result in stack traces that grow linearly with the number of chained futures. But when structured concurrency is involved, it seems more plausible to me that the most useful possible stack traces would go up the tree of tasks – that is, whenever a task was forked, the stack trace would look roughly as if it were a normal/sequential/direct invocation of the task. This could conceivably cause stack overflows where they didn’t happen before, but only for code that violates the expectations we have around normal sequential code: you can’t recurse unboundedly; use iteration instead. I’m curious if there are ways we could make the upcoming structured concurrency APIs give those stack traces all the way up the tree, or provide hooks to enable you to do that yourself. Last year’s JVMLS talk on Continuations Under the Covers demonstrated how stacks were redesigned in ways that frequently and efficiently snapshot the stack itself – not just the trace, but the thing that includes all the variables in use. There’s a linked list of StackChunks, and all but maybe the top of the stack has those elements frozen, etc, and the top of the stack gets frozen when the thread is yielded. Without certainty about how stack traces are managed in the JVM today, I would imagine you could possibly do something similar – you’d add a way to cheaply snapshot a reference to the current stack trace that can be traversed later. If you’re willing to hold on to all the references currently on the stack – which might be acceptable for the structured concurrency case in particular, where you might be able to assume you’ll return to the parent task and its stack at some point – you might be able to do this by simply wrapping the existing StackChunks. Then, each `fork` or `StructuredTaskScope` creation might snapshot the current call stack, and you’d stitch together the stack traces later…somewhere. That part is a little more open ended: would you add a new variant of `fillInStackTrace`? Would it only apply to exceptions that bubbled up to the task scope? Or would we be adding new semantics to what happens when you throw an exception or walk the stack in general? The most plausible vision I have at this point is an API that spawns a virtual thread which receives a stack trace of some sort – or perhaps snapshots the current stack trace – and prepends that trace to all stack traces within the virtual thread’s execution. I suppose this is doable today if you’re willing to pay the performance cost of explicitly getting the current stack trace every time you fork a task or start a scope. That is kind of antithetical to the point of virtual threads – making forking tasks very efficient – but it’s something you might be willing to turn on during testing. Right now, my inspiration for this question is attempting to improve the stack trace situation with Kotlin coroutines, where Google production apps have complained about the difficulty of debugging with the current stack traces. But this is something I'd expect to apply equally well to all JVM languages: the ability to snapshot and string together stack trace causal chains like this in production could significantly improve the experience of debugging concurrent code. -- Louis Wasserman
Re: RFR: 8335642: Hide Transform implementation for Class-File API [v2]
> Removes ClassFile API transformation implementation details accidentally > leaked to public API. Users don't have access to classes necessary to > correctly implement these transform resolutions. In addition, removed > improper `canWriteDirect` and made `ClassFileBuilder::transform` chain > returns. > > Replaces #19928. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: return tag required - Changes: - all: https://git.openjdk.org/jdk/pull/20102/files - new: https://git.openjdk.org/jdk/pull/20102/files/9b40fb76..a36dea5c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20102&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20102&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20102.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20102/head:pull/20102 PR: https://git.openjdk.org/jdk/pull/20102
Re: RFR: 8325679: Optimize ArrayList subList sort [v4]
On Mon, 19 Feb 2024 10:05:24 GMT, Attila Szegedi wrote: >> Somewhat surprisingly, `ArrayList$Sublist.sort()` is not specialized and >> will thus fall back to slower default method of `List.sort()` instead of >> sorting a range of the array in-place in its backing root `ArrayList`. >> >> This doesn't change observable behavior, so haven't added tests, and `tier1` >> tests still all pass except for >> `test/jdk/java/util/Locale/LocaleProvidersFormat.java` which also currently >> fails on master too on the machine I tested on. > > Attila Szegedi has updated the pull request incrementally with one additional > commit since the last revision: > > Remove test Keep it open, please. - PR Comment: https://git.openjdk.org/jdk/pull/17818#issuecomment-2218421921
More useful structured concurrency stack traces
My understanding of the structured concurrency APIs now in preview is that when a subtask is forked, exceptions thrown in that stack trace will have stack traces going up to the beginning of that subtask, not e.g. up the structured concurrency task tree. (My tests suggest this is the case for simple virtual threads without structured concurrency.) Most concurrency frameworks on the JVM that I’ve encountered share the property that stack traces for exceptions don’t trace through the entire causal chain – and, not unrelatedly, that developers struggle to debug concurrent applications, especially with stack traces from production and not full debuggers attached. In some cases, like chained CompletableFutures, this seems necessary to ensure that executing what amounts to a loop does not result in stack traces that grow linearly with the number of chained futures. But when structured concurrency is involved, it seems more plausible to me that the most useful possible stack traces would go up the tree of tasks – that is, whenever a task was forked, the stack trace would look roughly as if it were a normal/sequential/direct invocation of the task. This could conceivably cause stack overflows where they didn’t happen before, but only for code that violates the expectations we have around normal sequential code: you can’t recurse unboundedly; use iteration instead. I’m curious if there are ways we could make the upcoming structured concurrency APIs give those stack traces all the way up the tree, or provide hooks to enable you to do that yourself. Last year’s JVMLS talk on Continuations Under the Covers demonstrated how stacks were redesigned in ways that frequently and efficiently snapshot the stack itself – not just the trace, but the thing that includes all the variables in use. There’s a linked list of StackChunks, and all but maybe the top of the stack has those elements frozen, etc, and the top of the stack gets frozen when the thread is yielded. Without certainty about how stack traces are managed in the JVM today, I would imagine you could possibly do something similar – you’d add a way to cheaply snapshot a reference to the current stack trace that can be traversed later. If you’re willing to hold on to all the references currently on the stack – which might be acceptable for the structured concurrency case in particular, where you might be able to assume you’ll return to the parent task and its stack at some point – you might be able to do this by simply wrapping the existing StackChunks. Then, each `fork` or `StructuredTaskScope` creation might snapshot the current call stack, and you’d stitch together the stack traces later…somewhere. That part is a little more open ended: would you add a new variant of `fillInStackTrace`? Would it only apply to exceptions that bubbled up to the task scope? Or would we be adding new semantics to what happens when you throw an exception or walk the stack in general? The most plausible vision I have at this point is an API that spawns a virtual thread which receives a stack trace of some sort – or perhaps snapshots the current stack trace – and prepends that trace to all stack traces within the virtual thread’s execution. I suppose this is doable today if you’re willing to pay the performance cost of explicitly getting the current stack trace every time you fork a task or start a scope. That is kind of antithetical to the point of virtual threads – making forking tasks very efficient – but it’s something you might be willing to turn on during testing. Right now, my inspiration for this question is attempting to improve the stack trace situation with Kotlin coroutines, where Google production apps have complained about the difficulty of debugging with the current stack traces. But this is something I'd expect to apply equally well to all JVM languages: the ability to snapshot and string together stack trace causal chains like this in production could significantly improve the experience of debugging concurrent code. -- Louis Wasserman
RFR: 8335642: Hide Transform implementation for Class-File API
Removes ClassFile API transformation implementation details accidentally leaked to public API. Users don't have access to classes necessary to correctly implement these transform resolutions. In addition, removed improper `canWriteDirect` and made `ClassFileBuilder::transform` chain returns. Replaces #19928. - Commit messages: - 8335642: Hide Transform implementation for Class-File API Changes: https://git.openjdk.org/jdk/pull/20102/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20102&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335642 Stats: 169 lines in 10 files changed: 28 ins; 100 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/20102.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20102/head:pull/20102 PR: https://git.openjdk.org/jdk/pull/20102
RFR: 8335668: NumberFormat integer only parsing should throw exception for edge case
Please review this PR which corrects a case in NumberFormat integer only
parsing.
[JDK-8333755](https://bugs.openjdk.org/browse/JDK-8333755) fixed integer only
parsing when the value has a suffix, although it caused incorrect behavior for
the following case: when the parsed string does not contain an integer portion,
and the format has integer only parsing, parsing should fail, instead of 0
being returned. For example,
var fmt = NumberFormat.getIntegerInstance();
fmt.parse(".5", new ParsePosition(0)); // should return null, not 0
The changes to the _badParseStrings_ data provider in _StrictParseTest.java_
are needed since those cases _should_ fail in different indexes depending on if
integer parsing is enabled. Thus, they were updated to ensure they fail for
both integer and non-integer parsing with the same errorIndex.
In the fix itself, I also updated the initial value of `intIndex` to -1 from 0,
to provide better clarity.
-
Commit messages:
- clean up + add more general test cases
- init
Changes: https://git.openjdk.org/jdk/pull/20101/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20101&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8335668
Stats: 65 lines in 3 files changed: 54 ins; 0 del; 11 mod
Patch: https://git.openjdk.org/jdk/pull/20101.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20101/head:pull/20101
PR: https://git.openjdk.org/jdk/pull/20101
Re: Read-only view of ByteArrayInputStream content
Le 2024-07-09 à 20 h 14, Archie Cobbs a écrit :
Gotcha - so in other words, you want a way to effectively "unwrap" the
original byte[] array so you can access the whole thing at one time
(random access), as opposed to just accessing it in online fashion as
a stream of bytes.
Indeed, I wanted to "unwrap" the original byte[] array. But the goal was
not that much for random access (I could get the same with
readAllBytes()), but rather to avoid unnecessary array copies.
Basically, the BLOB API seems clearly designed to allow the
implementation to stream the data on demand if it wants to (which is
good), but as a side effect it doesn't provide a way for the caller to
guarantee avoidance of copying the entire array (if the implementation
happens to not stream the data on demand).
Right. The wish to "unwrap" the ByteArrayInputStream original array come
from the empirical observation that many of the JDBC drivers that we are
using do not stream. Therefore, our code was like:
while (resultSet.next()) { // Potentially millions of rows
try (InputStream in = resultSet.getBinaryStream(blobColumn)) {
if (in instanceof ByteArrayInputStream) {
unwrap the original array without copy
} else {
slower path with streaming
}
}
}
For the "unwrap the array" part, a read-only ByteBuffer would be fine.
Hence the proposal for adding a ByteArrayInputStream.asByteBuffer() method.
Martin
Re: RFR: 8333396: Use StringBuilder internally for java.text.Format.* formatting [v18]
On Tue, 9 Jul 2024 02:06:34 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Use StringBuilder internally for java.text.Format.* formatting
Thanks for the changes. Let's work to get the CSR in good shape now.
(I have left comments directly on the CSR).
-
Marked as reviewed by jlu (Committer).
PR Review: https://git.openjdk.org/jdk/pull/19513#pullrequestreview-2167093930
RFR: 8335935: Chained builders not sending transformed models to next transforms
Please review the fix for a major transformation bug within the ClassFile API, where certain kinds of buffered elements produced by one transform is not sent to the next, and the "chained" (`andThen` transformation chains) builders returning the wrong builder for call chains. This is intended to be backported to JDK 23, as this has significant impact on user experience with ClassFile API transformations. The backport won't be clean as we already renamed `ClassFile.transform` to `transformClass`, which will be reverted in the backport. - Commit messages: - JDK-8335935: Chained builders not sending transformed models to next transforms Changes: https://git.openjdk.org/jdk/pull/20100/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20100&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335935 Stats: 293 lines in 10 files changed: 169 ins; 97 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/20100.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20100/head:pull/20100 PR: https://git.openjdk.org/jdk/pull/20100
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v21]
On Tue, 9 Jul 2024 18:14:03 GMT, Raffaello Giulietti wrote: > Everything not obvious that departs from the paper by Bertot, Magaud and > Zimmermann should be commented. Unfortunately, I can't say precisely what and > to which extent until I see a first version. @rgiulietti Well. Regarding the method `sqrtRemZimmermann()`, the base case is different fundamentally because in the paper there's no base case at all and the implementation is left to the reader. For the recursive step, apart from the considerations above, the rest of the implementation is based on the pseudocode in the papers, and every difference is based on considerations that are present in the two papers. - PR Comment: https://git.openjdk.org/jdk/pull/19710#issuecomment-2218373924
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v21]
On Tue, 2 Jul 2024 01:44:43 GMT, fabioromano1 wrote: >> I have implemented the Zimmermann's square root algorithm, available in >> works [here](https://inria.hal.science/inria-00072854/en/) and >> [here](https://www.researchgate.net/publication/220532560_A_proof_of_GMP_square_root). >> >> The algorithm is proved to be asymptotically faster than the Newton's >> Method, even for small numbers. To get an idea of how much the Newton's >> Method is slow, consult my article >> [here](https://arxiv.org/abs/2406.07751), in which I compare Newton's Method >> with a version of classical square root algorithm that I implemented. After >> implementing Zimmermann's algorithm, it turns out that it is faster than my >> algorithm even for small numbers. > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Ensure normalized value in MutableBigInteger initialization with ints Everything not obvious that departs from the paper by Bertot, Magaud and Zimmermann should be commented. Unfortunately, I can't say precisely what and to which extent until I see a first version. - PR Comment: https://git.openjdk.org/jdk/pull/19710#issuecomment-2218355974
Re: Read-only view of ByteArrayInputStream content
Hi Martin, On Tue, Jul 9, 2024 at 12:31 PM Martin Desruisseaux < martin.desruisse...@geomatys.com> wrote: > I was using a custom OutputStream implementation for getting the value of > that field, avoiding any form of copy. > Gotcha - so in other words, you want a way to effectively "unwrap" the original byte[] array so you can access the whole thing at one time (random access), as opposed to just accessing it in online fashion as a stream of bytes. It seems that the root of this dilemma is really the java.sql API. For example, you would think there should be a method java.sql.Blob.asByteBuffer() (and Clob.asCharBuffer()). Maybe adding such methods would be a more precise way to fix this. In looking at this, I noticed that Blob.getBytes() does not specify whether, when the entire array is asked for, a copy must (or must not) be returned. So you can't rely on that method for any particular behavior either. Basically, the BLOB API seems clearly designed to allow the implementation to stream the data on demand if it wants to (which is good), but as a side effect it doesn't provide a way for the caller to guarantee avoidance of copying the entire array (if the implementation happens to not stream the data on demand). -Archie -- Archie L. Cobbs
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v21]
On Tue, 9 Jul 2024 18:04:43 GMT, Raffaello Giulietti wrote: > These helpful considerations, and others that are not obvious when comparing > with the paper, should really be part of comments in the code. As mentioned, > this helps with reviewing and for maintenance. @rgiulietti I will add the above informations as comments in the code. If there are other parts that need to be clarified, please let me know. - PR Comment: https://git.openjdk.org/jdk/pull/19710#issuecomment-2218346785
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v21]
On Tue, 2 Jul 2024 01:44:43 GMT, fabioromano1 wrote: >> I have implemented the Zimmermann's square root algorithm, available in >> works [here](https://inria.hal.science/inria-00072854/en/) and >> [here](https://www.researchgate.net/publication/220532560_A_proof_of_GMP_square_root). >> >> The algorithm is proved to be asymptotically faster than the Newton's >> Method, even for small numbers. To get an idea of how much the Newton's >> Method is slow, consult my article >> [here](https://arxiv.org/abs/2406.07751), in which I compare Newton's Method >> with a version of classical square root algorithm that I implemented. After >> implementing Zimmermann's algorithm, it turns out that it is faster than my >> algorithm even for small numbers. > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Ensure normalized value in MutableBigInteger initialization with ints These helpful considerations, and others that are not obvious when comparing with the paper, should really be part of comments in the code. As mentioned, this helps with reviewing and for maintenance. - PR Comment: https://git.openjdk.org/jdk/pull/19710#issuecomment-2218338467
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v21]
On Tue, 9 Jul 2024 17:17:48 GMT, Raffaello Giulietti
wrote:
> Let's agree that the reference for this PR is the
> [algorithm](https://inria.hal.science/inria-00072113/document) described by
> Bertot, Magaud and Zimmermann.
>
> From a first reading, the algorithm implemented here appears different in
> many aspects from the one in the paper, making a direct comparison between
> the twos rather hard. For example, the paper normalizes the input to an even
> number of words ("limbs"). AFAIU, this doesn't seem to happen here. There
> might be good reasons to depart from what is described in the paper, but
> there's no discussion.
>
> To ease the reviewing process, and for the benefit of future maintainers,
> every departure from the paper should be discussed and mathematically
> justified to some extent in comments.
@rgiulietti The input is normalized in the method called `sqrtRemZimmermann`,
and is normalized to a number of words which is a multiple of 4, and of course
a multiple of 4 is even. For speed, the normalization is performed only
"logically",
which means that the contents of the input are not changed, but only its
logical length, which is stored in the variable `len`.
The reason why the length is normalized to a mutiple of 4 is that, in this way,
the input can be always split in blocks of words without twiddling with bits.
I am completely available for further clarifications regarding the algorithm
code.
-
PR Comment: https://git.openjdk.org/jdk/pull/19710#issuecomment-2218328258
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException [v3]
On Tue, 9 Jul 2024 17:11:22 GMT, Liam Miller-Cushon wrote:
>> test/jdk/java/util/Map/MapFactories.java line 505:
>>
>>> 503:
>>> 504: @Test(expectedExceptions=UnsupportedOperationException.class)
>>> 505: public void immutableEntrySetAddAllDisallowed() {
>>
>> Looking back at MOAT, do you think we should add these into MOAT?
>> https://github.com/openjdk/jdk/blob/598af2e51464be089b64da4024e62865c2c6ec72/test/jdk/java/util/Collection/MOAT.java#L594-L619
>>
>> We just need to add calls to `testMapMutatorsAlwaysThrow` and
>> `testEmptyMapMutatorsAlwaysThrow` to check
>> `test(Empty)CollMutatorsAlwaysThrow(map.entrySet());`,
>> `test(Empty)CollMutatorsAlwaysThrow(map.keySet());`, and
>> `test(Empty)CollMutatorsAlwaysThrow(map.values());`
>
> `testCollMutatorsAlwaysThrow` expects a `Collection` (not e.g. a
> `Collection>`). MOAT could be refactored to handle
> that case. Do you think that's worth it, or have thoughts about what the
> cleanest way to do that would be?
There is `testImmutableCollection`/`testImmutableSet` that takes an arbitrary
nonexistent item for insertion/removal:
https://github.com/openjdk/jdk/blob/598af2e51464be089b64da4024e62865c2c6ec72/test/jdk/java/util/Collection/MOAT.java#L665
I think a refactor of a generic `testCollMutatorsAlwaysThrow(Collection c, T
t)` and delegating the original Integer version to call
`testCollMutatorsAlwaysThrow(c, ABSENT_VALUE)` would not be invasive.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1670934363
Re: Read-only view of ByteArrayInputStream content
Hello Archie, thanks for the reply.
Le 2024-07-09 à 18 h 17, Archie Cobbs a écrit :
The difference in the old vs. new behavior is the use of a 128k
temporary transfer buffer. So if I understand this correctly the
performance problem you are addressing (in terms of blown cache) is
not proportional to the size of the original BLOB, but to at most 128K
No, in the previous JDK implementation, no 128 kb transfer buffer was
used. The implementation before commit 5cacf21 (September 2023) was as
below:
public synchronized long transferTo(OutputStream out) throws IOException {
int len = count - pos;
out.write(buf, pos, len);
pos = count;
return len;
}
In above code, buf is a protected field of ByteArrayInputStream with a
value specified by user at construction time, so the array can have any
size. I was using a custom OutputStream implementation for getting the
value of that field, avoiding any form of copy. But this trick does not
work anymore since a transfer buffer has been introduced in commit
b0d1450 (December 2023). A read-only ByteBuffer would make possible to
continue to avoid a copy.
Martin
Re: RFR: 8334755: Asymptotically faster implementation of square root algorithm [v21]
On Tue, 2 Jul 2024 01:44:43 GMT, fabioromano1 wrote:
>> I have implemented the Zimmermann's square root algorithm, available in
>> works [here](https://inria.hal.science/inria-00072854/en/) and
>> [here](https://www.researchgate.net/publication/220532560_A_proof_of_GMP_square_root).
>>
>> The algorithm is proved to be asymptotically faster than the Newton's
>> Method, even for small numbers. To get an idea of how much the Newton's
>> Method is slow, consult my article
>> [here](https://arxiv.org/abs/2406.07751), in which I compare Newton's Method
>> with a version of classical square root algorithm that I implemented. After
>> implementing Zimmermann's algorithm, it turns out that it is faster than my
>> algorithm even for small numbers.
>
> fabioromano1 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Ensure normalized value in MutableBigInteger initialization with ints
Let's agree that the reference for this PR is the
[algorithm](https://inria.hal.science/inria-00072113/document) described by
Bertot, Magaud and Zimmermann.
>From a first reading, the algorithm implemented here appears different in many
>aspects from the one in the paper, making a direct comparison between the twos
>rather hard.
For example, the paper normalizes the input to an even number of words
("limbs"). AFAIU, this doesn't seem to happen here. There might be good reasons
to depart from what is described in the paper, but there's no discussion.
To ease the reviewing process, and for the benefit of future maintainers, every
departure from the paper should be discussed and mathematically justified to
some extent in comments.
-
PR Comment: https://git.openjdk.org/jdk/pull/19710#issuecomment-2218267026
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException [v3]
On Mon, 8 Jul 2024 20:39:38 GMT, Chen Liang wrote:
>> Liam Miller-Cushon 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 six
>> additional commits since the last revision:
>>
>> - Merge remote-tracking branch 'origin/master' into
>> JDK-8328821-make-clear-consistent
>> - Check m.entrySet().hashCode() in MOAT
>> - Merge remote-tracking branch 'origin/master' into
>> JDK-8328821-make-clear-consistent
>> - Use AbstractImmutableSet
>> - Throw UOE for all Map.of().entrySet() mutator methods
>> - 8328821: Make the ImmutableCollections clear() call consistent
>>
>>Without overriding clear(), a call to it in an empty map would
>>just return, as iterator.hasNext() would be false. However if
>>calling Map.of().clear() throws an exception. To make the
>>behavior of Map.of().entrySet().clear() consistent, we need to
>>have an implementation of clear() for the entry set that throws
>>as well.
>
> test/jdk/java/util/Map/MapFactories.java line 505:
>
>> 503:
>> 504: @Test(expectedExceptions=UnsupportedOperationException.class)
>> 505: public void immutableEntrySetAddAllDisallowed() {
>
> Looking back at MOAT, do you think we should add these into MOAT?
> https://github.com/openjdk/jdk/blob/598af2e51464be089b64da4024e62865c2c6ec72/test/jdk/java/util/Collection/MOAT.java#L594-L619
>
> We just need to add calls to `testMapMutatorsAlwaysThrow` and
> `testEmptyMapMutatorsAlwaysThrow` to check
> `test(Empty)CollMutatorsAlwaysThrow(map.entrySet());`,
> `test(Empty)CollMutatorsAlwaysThrow(map.keySet());`, and
> `test(Empty)CollMutatorsAlwaysThrow(map.values());`
`testCollMutatorsAlwaysThrow` expects a `Collection` (not e.g. a
`Collection>`). MOAT could be refactored to handle that
case. Do you think that's worth it, or have thoughts about what the cleanest
way to do that would be?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1670899976
Re: Read-only view of ByteArrayInputStream content
On Tue, Jul 9, 2024 at 10:51 AM Martin Desruisseaux <
martin.desruisse...@geomatys.com> wrote:
> JDK-22 has modified ByteArrayInputStream.transferTo(OutputStream)
> implementation for performing a defensive copy of the byte array when the
> destination is not trusted (JDK-8321053). However, I was using the previous
> implementation as a trick for reading the array without copy. The reason is
> because we have millions of ByteArrayInputStream instances when reading
> BLOBs from a database (e.g., geometries from a spatial database), and some
> popular JDBC drivers implement ResultSet.getBinaryStream(int) by reading
> all data in a byte[] array and wrapping the array in a
> ByteArrayInputStream. As a cleaner replacement for my old trick, would it
> be possible to add something like the following method in
> ByteArrayInputStream?
>
> /**
> * Returns the remaining content of this input stream as a read-only buffer.
> * The sequence of remaining bytes in the buffer is the same as the sequence
> * of bytes that would be returned by {@link #readAllBytes()}.
> *
> * @return the remaining content of this input stream, as a read-only buffer
> */
> public synchronized ByteBuffer asByteBuffer() {
> return ByteBuffer.wrap(buf, pos, count - pos).slice().asReadOnlyBuffer();
> }
>
> The call to slice() is for blocking callers from accessing the bytes
> before the current stream position. I assumed that it could be a security
> issue. If this proposal is acceptable, I can create a pull request.
>
A few random thoughts...
First, let's consider what you are optimizing for. The difference in the
old vs. new behavior is the use of a 128k temporary transfer buffer. So if
I understand this correctly the performance problem you are addressing (in
terms of blown cache) is not proportional to the size of the original BLOB,
but to at most 128K (in both the old and new cases, you have to scan the
entire BLOB, so that's a wash). Does that 128K create a noticeable
difference?
Anyway, IMHO more interoperability between the different Java "ways of
doing things" is good. E.g., converting between Instant and Date, etc. This
falls into that category. In fact, in the past I've wished for the reverse
- i.e., ByteBuffer.asOutputStream() - and ended up creating a simple
wrapper class that does that. So perhaps these two additions could go
together - just a thought.
Minor nit - the Javadoc would need to clarify that operations on the
returned ByteBuffer have no effect on the original OutputStream.
-Archie
--
Archie L. Cobbs
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v4]
> Created jtreg test case for > [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue. > > The JpackageTest created tests that the child process started from the app > launched by jpackage launcher is not automatically terminated when the the > launcher is terminated. Vanitha B P has updated the pull request incrementally with one additional commit since the last revision: Addressed the review comment - Changes: - all: https://git.openjdk.org/jdk/pull/19536/files - new: https://git.openjdk.org/jdk/pull/19536/files/b3275577..964742d9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19536&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19536&range=02-03 Stats: 8 lines in 1 file changed: 1 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19536.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19536/head:pull/19536 PR: https://git.openjdk.org/jdk/pull/19536
RFR: 8336012: Fix usages of jtreg-reserved properties
Please review this trivial test-only change to resolve a name clash with a jtreg-reserved property by removing the third option to control a test verbosity level via the `test.verbose` system property. The fix assumes that the system property was only used in a manual scenario, like `make test TEST=...` and a `-Dtest.verbose=99` equivalent. For such scenarios, the other two names (for example `-DPrivateInvokeTest.verbose=99`) are still supported. - Commit messages: - JDK-8336012: Fix usages of jtreg-reserved properties Changes: https://git.openjdk.org/jdk/pull/20099/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20099&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336012 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20099.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20099/head:pull/20099 PR: https://git.openjdk.org/jdk/pull/20099
Read-only view of ByteArrayInputStream content
Hello
JDK-22 has modified ByteArrayInputStream.transferTo(OutputStream)
implementation for performing a defensive copy of the byte array when
the destination is not trusted (JDK-8321053). However, I was using the
previous implementation as a trick for reading the array without copy.
The reason is because we have millions of ByteArrayInputStream instances
when reading BLOBs from a database (e.g., geometries from a spatial
database), and some popular JDBC drivers implement
ResultSet.getBinaryStream(int) by reading all data in a byte[] array and
wrapping the array in a ByteArrayInputStream. As a cleaner replacement
for my old trick, would it be possible to add something like the
following method in ByteArrayInputStream?
/** * Returns the remaining content of this input stream as a
read-only buffer. * The sequence of remaining bytes in the buffer is
the same as the sequence * of bytes that would be returned by {@link
#readAllBytes()}. * * @return the remaining content of this input
stream, as a read-only buffer */ public synchronized ByteBuffer
asByteBuffer() { return ByteBuffer.wrap(buf, pos, count -
pos).slice().asReadOnlyBuffer(); }
The call to slice() is for blocking callers from accessing the bytes
before the current stream position. I assumed that it could be a
security issue. If this proposal is acceptable, I can create a pull request.
Thanks,
Martin
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v3]
On Tue, 9 Jul 2024 14:26:11 GMT, Alexey Semenyuk wrote:
>> Vanitha B P has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressed the review comments based on the inputs
>
> test/jdk/tools/jpackage/apps/ChildProcessAppLauncher.java line 39:
>
>> 37: System.out.println("Calc id=" + process.pid());
>> 38: System.exit(0);
>> 39: }
>
> The better alternative would be:
> ```String calcPath = Path.of(System.getenv("SystemRoot"), "system32",
> "calc.exe").toString();```
>
> This way NPE will be thrown if `SystemRoot` env variable is not set instead
> of silent exit, and `FS` field is not needed.
Agree, i will make the changes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1670697556
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 14:42:42 GMT, Doug Simon wrote:
>> test/jdk/jdk/internal/vm/TestTranslatedException.java line 167:
>>
>>> 165: private static void assertThrowableEquals(Throwable originalIn,
>>> Throwable decodedIn) {
>>> 166: Throwable original = originalIn;
>>> 167: Throwable decoded = decodedIn;
>>
>> What is the purpose of this renaming?
>
> So that the printing down the bottom of this message shows the complete
> throwable, not just the cause on which the comparison failed.
Thanks! I missed the reassign in the folded unchanged code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670683400
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 13:46:46 GMT, Doug Simon wrote: >> This PR addresses intermittent failures in jtreg GC stress tests. The >> failures occur under these conditions: >> 1. Using a libgraal build with assertions enabled as the top tier JIT >> compiler. Such a libgraal build will cause a VM exit if an assertion or >> GraalError occurs in a compiler thread (as this catches more errors in >> testing). >> 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) >> to a routine that performs a HotSpot heap allocation that fails. >> 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as >> described in 1. >> >> An OOME thrown in these specific conditions should not exit the VM as it not >> related to an OOME in the app or test. Instead, the failure should be >> treated as a bailout and the libgraal compiler should continue. >> >> To accomplish this, libgraal needs to be able to distinguish a GraalError >> caused by an OOME. This PR modifies the exception translation code to make >> this possible. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fixed TestTranslatedException LGTM - Marked as reviewed by yzheng (Committer). PR Review: https://git.openjdk.org/jdk/pull/20083#pullrequestreview-2166581323
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 14:37:47 GMT, Yudi Zheng wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed TestTranslatedException
>
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 782:
>
>> 780: while (true) {
>> 781: // Trigger an OutOfMemoryError
>> 782: objArrayOop next = oopFactory::new_objectArray(0x7FFF,
>> CHECK_NULL);
>
> Shall we check for pending exception and break here?
The `CHECK_NULL` macro effectively does that.
> test/jdk/jdk/internal/vm/TestTranslatedException.java line 167:
>
>> 165: private static void assertThrowableEquals(Throwable originalIn,
>> Throwable decodedIn) {
>> 166: Throwable original = originalIn;
>> 167: Throwable decoded = decodedIn;
>
> What is the purpose of this renaming?
So that the printing down the bottom of this message shows the complete
throwable, not just the cause on which the comparison failed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670656254
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670654917
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 13:46:46 GMT, Doug Simon wrote:
>> This PR addresses intermittent failures in jtreg GC stress tests. The
>> failures occur under these conditions:
>> 1. Using a libgraal build with assertions enabled as the top tier JIT
>> compiler. Such a libgraal build will cause a VM exit if an assertion or
>> GraalError occurs in a compiler thread (as this catches more errors in
>> testing).
>> 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`)
>> to a routine that performs a HotSpot heap allocation that fails.
>> 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as
>> described in 1.
>>
>> An OOME thrown in these specific conditions should not exit the VM as it not
>> related to an OOME in the app or test. Instead, the failure should be
>> treated as a bailout and the libgraal compiler should continue.
>>
>> To accomplish this, libgraal needs to be able to distinguish a GraalError
>> caused by an OOME. This PR modifies the exception translation code to make
>> this possible.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed TestTranslatedException
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 782:
> 780: while (true) {
> 781: // Trigger an OutOfMemoryError
> 782: objArrayOop next = oopFactory::new_objectArray(0x7FFF,
> CHECK_NULL);
Shall we check for pending exception and break here?
test/jdk/jdk/internal/vm/TestTranslatedException.java line 167:
> 165: private static void assertThrowableEquals(Throwable originalIn,
> Throwable decodedIn) {
> 166: Throwable original = originalIn;
> 167: Throwable decoded = decodedIn;
What is the purpose of this renaming?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670646934
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670607742
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v3]
On Tue, 9 Jul 2024 10:49:06 GMT, Vanitha B P wrote:
>> Created jtreg test case for
>> [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue.
>>
>> The JpackageTest created tests that the child process started from the app
>> launched by jpackage launcher is not automatically terminated when the the
>> launcher is terminated.
>
> Vanitha B P has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressed the review comments based on the inputs
test/jdk/tools/jpackage/apps/ChildProcessAppLauncher.java line 39:
> 37: System.out.println("Calc id=" + process.pid());
> 38: System.exit(0);
> 39: }
The better alternative would be:
```String calcPath = Path.of(System.getenv("SystemRoot"), "system32",
"calc.exe").toString();```
This way NPE will be thrown if `SystemRoot` env variable is not set instead of
silent exit, and `FS` field is not needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19536#discussion_r1670627437
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
> This PR addresses intermittent failures in jtreg GC stress tests. The > failures occur under these conditions: > 1. Using a libgraal build with assertions enabled as the top tier JIT > compiler. Such a libgraal build will cause a VM exit if an assertion or > GraalError occurs in a compiler thread (as this catches more errors in > testing). > 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) > to a routine that performs a HotSpot heap allocation that fails. > 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as > described in 1. > > An OOME thrown in these specific conditions should not exit the VM as it not > related to an OOME in the app or test. Instead, the failure should be treated > as a bailout and the libgraal compiler should continue. > > To accomplish this, libgraal needs to be able to distinguish a GraalError > caused by an OOME. This PR modifies the exception translation code to make > this possible. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fixed TestTranslatedException - Changes: - all: https://git.openjdk.org/jdk/pull/20083/files - new: https://git.openjdk.org/jdk/pull/20083/files/ff544be3..aa32491c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20083&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20083&range=00-01 Stats: 19 lines in 2 files changed: 12 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20083/head:pull/20083 PR: https://git.openjdk.org/jdk/pull/20083
Re: RFR: 8335820: java/lang/invoke/LFCaching/LFSingleThreadCachingTest.java fails due to IllegalArgumentException: hash must be nonzero [v2]
On Mon, 8 Jul 2024 14:14:02 GMT, Adam Sotona wrote: >> Class-File API constant pool implementation requires non-zero entry hash >> code. >> Unfortunately current implementation computes zero hash code for specific CP >> entries. >> >> This patch removes invalid and obsolete `AbstractPoolEntry::phiMix` >> calculation and assures all pool entries have non-zero hash. A regression >> test of the actual zero-hash `IntegerEntry` has been added. >> >> All pre-computed hash codes in `BoundAttribute::standardAttribute` are >> updated. >> >> The patch has no performance effect measurable by any of the actual >> Class-File API benchmarks. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > fixed BootstrapMethodEntryImpl::computeHashCode Marked as reviewed by liach (Committer). - PR Review: https://git.openjdk.org/jdk/pull/20074#pullrequestreview-2166140052
RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long)
This patch intrinsifies `Math.max(long, long)` and `Math.min(long, long)` in
order to help improve vectorization performance.
Currently vectorization does not kick in for loops containing either of these
calls because of the following error:
VLoop::check_preconditions: failed: control flow in loop not allowed
The control flow is due to the java implementation for these methods, e.g.
public static long max(long a, long b) {
return (a >= b) ? a : b;
}
This patch intrinsifies the calls to replace the CmpL + Bool nodes for
MaxL/MinL nodes respectively.
By doing this, vectorization no longer finds the control flow and so it can
carry out the vectorization.
E.g.
SuperWord::transform_loop:
Loop: N518/N126 counted [int,int),+4 (1025 iters) main has_sfpt
strip_mined
518 CountedLoop === 518 246 126 [[ 513 517 518 242 521 522 422 210 ]] inner
stride: 4 main of N518 strip mined !orig=[419],[247],[216],[193] !jvms:
Test::test @ bci:14 (line 21)
Applying the same changes to `ReductionPerf` as in
https://github.com/openjdk/jdk/pull/13056, we can compare the results before
and after. Before the patch, on darwin/aarch64 (M1):
==
Test summary
==
TEST TOTAL PASS FAIL ERROR
jtreg:test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java
1 1 0 0
==
TEST SUCCESS
long min 1155
long max 1173
After the patch, on darwin/aarch64 (M1):
==
Test summary
==
TEST TOTAL PASS FAIL ERROR
jtreg:test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java
1 1 0 0
==
TEST SUCCESS
long min 1042
long max 1042
This patch does not add an platform-specific backend implementations for the
MaxL/MinL nodes.
Therefore, it still relies on the macro expansion to transform those into
CMoveL.
I've run tier1 and hotspot compiler tests on darwin/aarch64 and got these
results:
==
Test summary
==
TEST TOTAL PASS FAIL ERROR
jtreg:test/hotspot/jtreg:tier1 2500 2500 0 0
>> jtreg:test/jdk:tier1 2413 2412 1 0 <<
jtreg:test/langtools:tier1 4556 4556 0 0
jtreg:test/jaxp:tier1 0 0 0 0
jtreg:test/lib-test:tier13333 0 0
==
The failure I got is
[CODETOOLS-7903745](https://bugs.openjdk.org/browse/CODETOOLS-7903745) so
unrelated to these changes.
-
Commit messages:
- 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long)
Changes: https://git.openjdk.org/jdk/pull/20098/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20098&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8307513
Stats: 32 lines in 5 files changed: 32 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/20098.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20098/head:pull/20098
PR: https://git.openjdk.org/jdk/pull/20098
Re: RFR: 8325525: Create jtreg test case for JDK-8325203 [v3]
> Created jtreg test case for > [JDK-8325203](https://bugs.openjdk.org/browse/JDK-8325203) issue. > > The JpackageTest created tests that the child process started from the app > launched by jpackage launcher is not automatically terminated when the the > launcher is terminated. Vanitha B P has updated the pull request incrementally with one additional commit since the last revision: Addressed the review comments based on the inputs - Changes: - all: https://git.openjdk.org/jdk/pull/19536/files - new: https://git.openjdk.org/jdk/pull/19536/files/4d22961a..b3275577 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19536&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19536&range=01-02 Stats: 270 lines in 4 files changed: 129 ins; 141 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19536.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19536/head:pull/19536 PR: https://git.openjdk.org/jdk/pull/19536
Integrated: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:19:21 GMT, Severin Gehwolf wrote: > Please review this simple test fix to exclude the test from being run on an > Alpine Linux system. Apparently default Alpine Linux systems don't have > cgroups set up by default the way other distros do. More info on the bug. I > propose to not run the test on musl systems. This pull request has now been integrated. Changeset: f3ff4f74 Author:Severin Gehwolf URL: https://git.openjdk.org/jdk/commit/f3ff4f7427c3c3f5cb2a115a61462bb9d28de1cd Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux Reviewed-by: stuefe, mbaesken - PR: https://git.openjdk.org/jdk/pull/20076
Re: RFR: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:19:21 GMT, Severin Gehwolf wrote: > Please review this simple test fix to exclude the test from being run on an > Alpine Linux system. Apparently default Alpine Linux systems don't have > cgroups set up by default the way other distros do. More info on the bug. I > propose to not run the test on musl systems. Thanks for the reviews! The docs build failure in GHA is infra related: Run # If runner architecture is x64 set JAVA_HOME_17_X64 otherwise set to JAVA_HOME_17_arm64 [build.sh][INFO] CYGWIN_OR_MSYS=0 [build.sh][INFO] JAVA_HOME: /usr/lib/jvm/temurin-17-jdk-amd64 [build.sh][INFO] Downloading https://archive.apache.org/dist/ant/binaries/apache-ant-1.10.8-bin.zip to /home/runner/work/jdk/jdk/jtreg/src/make/../build/deps/apache-ant-1.10.8-bin.zip Error: sh][ERROR] wget exited with exit code 4 Error: Process completed with exit code 1. - PR Comment: https://git.openjdk.org/jdk/pull/20076#issuecomment-2217257118
Re: RFR: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
On Mon, 8 Jul 2024 14:19:21 GMT, Severin Gehwolf wrote: > Please review this simple test fix to exclude the test from being run on an > Alpine Linux system. Apparently default Alpine Linux systems don't have > cgroups set up by default the way other distros do. More info on the bug. I > propose to not run the test on musl systems. Marked as reviewed by mbaesken (Reviewer). The issue is gone in our tests, with your patch added. - PR Review: https://git.openjdk.org/jdk/pull/20076#pullrequestreview-2165630718 PR Comment: https://git.openjdk.org/jdk/pull/20076#issuecomment-2217120494
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation
On Mon, 8 Jul 2024 19:01:05 GMT, Doug Simon wrote:
> This PR addresses intermittent failures in jtreg GC stress tests. The
> failures occur under these conditions:
> 1. Using a libgraal build with assertions enabled as the top tier JIT
> compiler. Such a libgraal build will cause a VM exit if an assertion or
> GraalError occurs in a compiler thread (as this catches more errors in
> testing).
> 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`)
> to a routine that performs a HotSpot heap allocation that fails.
> 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as
> described in 1.
>
> An OOME thrown in these specific conditions should not exit the VM as it not
> related to an OOME in the app or test. Instead, the failure should be treated
> as a bailout and the libgraal compiler should continue.
>
> To accomplish this, libgraal needs to be able to distinguish a GraalError
> caused by an OOME. This PR modifies the exception translation code to make
> this possible.
src/hotspot/share/utilities/exceptions.cpp line 114:
> 112: #endif // ASSERT
> 113:
> 114: if (h_exception.is_null() && !thread->can_call_java()) {
There is no reason to replace an existing exception object with a dummy
exception object in the case where the current thread cannot call into Java.
Since the exception object already exists, no Java call is necessary.
This change is necessary to allow the libgraal exception translation mechanism
to know that an OOME is being translated.
src/hotspot/share/utilities/exceptions.cpp line 208:
> 206: Handle h_loader, Handle
> h_protection_domain) {
> 207: // Check for special boot-strapping/compiler-thread handling
> 208: if (special_exception(thread, file, line, h_cause)) return;
This fixes a long standing bug where `special_exception` is being queried with
the *cause* of the exception being thrown instead of the *name* of the
exception being thrown.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1669153819
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1669148553
RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation
This PR addresses intermittent failures in jtreg GC stress tests. The failures occur under these conditions: 1. Using a libgraal build with assertions enabled as the top tier JIT compiler. Such a libgraal build will cause a VM exit if an assertion or GraalError occurs in a compiler thread (as this catches more errors in testing). 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) to a routine that performs a HotSpot heap allocation that fails. 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as described in 1. An OOME thrown in these specific conditions should not exit the VM as it not related to an OOME in the app or test. Instead, the failure should be treated as a bailout and the libgraal compiler should continue. To accomplish this, libgraal needs to be able to distinguish a GraalError caused by an OOME. This PR modifies the exception translation code to make this possible. - Commit messages: - improved exception translation between HotSpot and libgraal heaps Changes: https://git.openjdk.org/jdk/pull/20083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20083&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335553 Stats: 84 lines in 5 files changed: 50 ins; 22 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/20083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20083/head:pull/20083 PR: https://git.openjdk.org/jdk/pull/20083
