8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out #19871

vnkozlov · 2024-06-24T23:40:22Z

The timeout is cause by running the test with -Xcomp -XX:+VerifyOops -XX:+PatchALot.
The test continuously deoptimize and recompile java.lang.Throwable::<init> method.
-XX:+VerifyOops adds a lot of external addresses because it use ExternallAddress for error messages.
These messages are unique for each call to verify_oop() because they are constructed locally.
I reduced number of loop iteration by 10 to get reasonable execution time (2 mins instead of 20 mins) and got next data:

Without VerifyOops:
  External addresses table: 38 entries

With VerifyOops:
  External addresses table: 125922 entries

Looks like most of the time VM is spending to grow/reallocate big growable array added by JDK-8333819.
Before that change these addresses were recorded locally in nmethod's relocation info. When nmethod was deoptimized, its relocation info was discarded together with nmethod.

Only on x86 we declared message address as ExternalAddress. Aarch64 uses movptr() to store address as simple pointer. ARM uses own InlineAddress with RelocInfo::none type of relocation. I verified other platforms: none is using ExternalAddress.

I suggest to use AddressLiteral with RelocInfo::none for x86. With that the global table is small even with -XX:+VerifyOops:

  External addresses table: 42 entries

Tested tier1, run test with corresponding flags to verify that time is similar to before JDK-8333819

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out (Bug - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19871/head:pull/19871
$ git checkout pull/19871

Update a local copy of the PR:
$ git checkout pull/19871
$ git pull https://git.openjdk.org/jdk.git pull/19871/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19871

View PR using the GUI difftool:
$ git pr show -t 19871

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19871.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-24T23:40:54Z

👋 Welcome back kvn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-06-24T23:41:09Z

@vnkozlov This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out

Reviewed-by: thartmann, dlong

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 39 new commits pushed to the master branch:

3796fdf: 8328536: javac - crash on unknown type referenced in yield statement
07bc523: 8334670: SSLSocketOutputRecord buffer miscalculation
4ebb771: 8334769: Shenandoah: Move CodeCache_lock close to its use in ShenandoahConcurrentNMethodIterator
817edcb: 8331411: Shenandoah: Reconsider spinning duration in ShenandoahLock
bffc848: 8333755: NumberFormat integer only parsing breaks when format has suffix
b5d5896: 8335108: Build error after JDK-8333658 due to class templates
5883a20: 8334437: De-duplicate ProxyMethod list creation
8591eff: 8332103: since-checker - Add missing @ since tags to java.desktop
8374d16: 8335006: C2 SuperWord: add JMH benchmark VectorLoadToStoreForwarding.java
4ffc5e6: 8326705: Test CertMsgCheck.java fails to find alert certificate_required
... and 29 more: https://git.openjdk.org/jdk/compare/71a692ab435fdeea4ce8f8db7a55dd735c7c5016...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-06-24T23:41:39Z

@vnkozlov The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-06-24T23:44:32Z

Webrevs

01: Full - Incremental (6da56e22)
00: Full (dc586132)

dean-long · 2024-06-25T03:20:42Z

Where are these strings actually located? Do they need to be relocated if the CodeBuffer expands?

Can we assert that uses of ExternalAddress do not point inside the CodeBuffer/CodeBlob? You might catch additional cases.

vnkozlov · 2024-06-25T14:57:00Z

Where are these strings actually located? Do they need to be relocated if the CodeBuffer expands?

Thank you, @dean-long, for looking.
They are not located in CodeCache space, they are external. It is long path from code_string(ss.as_string()) call but at the end a string is duplicated in C heap:
codeBuffer.cpp#L1088

But structure which keep track of these strings is part of CodeBlob CodeBlob:: _dbg_strings which is not copied by CodeBuffer::expand(). expand() has similar code to CodeBuffer::copy_code_to() except coping _dbg_strings and _asm_remarks. So we do have the issue with expand - we may not free space allocated by strings and comments when expanded nmethod is deoptimized.

vnkozlov · 2024-06-25T19:51:02Z

Can we assert that uses of ExternalAddress do not point inside the CodeBuffer/CodeBlob? You might catch additional cases.

To clarify, these strings are externals and using ExternalAddress for them is correct. But we don't need to use relocation for them (no need to patch) because they don't move and pushptr() in _verify_oops loads whole 64-bit address into register - no relative addressing. That is why I use relocInfo::none for them.

vnkozlov · 2024-06-25T19:56:55Z

I did experiment to see if ExternalAddress reference address in CodeCache. There are few places where it can be changed but it is for different RFE.

dean-long · 2024-06-25T20:28:16Z

How will Leyden relocate these references if they are changed to relocInfo::none? Maybe these strings should be moved to the nmethod's _immutable_data?

vnkozlov · 2024-06-25T21:27:46Z

How will Leyden relocate these references if they are changed to relocInfo::none? Maybe these strings should be moved to the nmethod's _immutable_data?

Very nice idea. I was already think about moving oop_maps and relocation info from CodeBlob into _immutable_data. I will include these strings too. In a separate RFE.

Until then Leyden will not support VerifyOops.

vnkozlov · 2024-06-25T21:34:22Z

I may need to use section_word relocation for that and add additional section type for _immutable_data.
I would also need to fix it for other platforms which do not have relocation for these strings.
It is not simple change and it needs a separate RFE.

vnkozlov · 2024-06-25T22:12:05Z

I did more investigation and there is an other way to solve this.

I think I can use external_word_Relocation::spec_for_immediate() instead of relocInfo::none to avoid growing global table (because external_word_Relocation::_target is nullptr in this case) and still mark them as external addresses, so Leyden can see these strings and support VerifyOops.

I did local testing and it works. I will test it more and update PR. Thank you, @dean-long, for this discussion.

dean-long · 2024-06-25T23:36:18Z

I forgot about spec_for_immediate(). I think it will work. For relocating in Leyden, you may need to enhance
Relocation::pd_get_address_from_code() to recognize pushptr().

vnkozlov · 2024-06-26T00:44:07Z

Yes, I need add code similar too oop_Relocation and metadata_Relocation which uses 64-bit immediate because
pushptr() -> lea() + push() and lea() -> mov_literal64() for x64: macroAssembler_x86.cpp#L641

Actually I may not need add anything. For immediate values pd_get_address_from_code() calls pd_address_in_code() which is used already by oop_Relocation and metadata_Relocation.
It should work but I will check.

…fo::none

vnkozlov · 2024-06-26T02:31:02Z

I ran tier7 which uses -Xcomp -XX:+VerifyOops flags combination. It passed clean with last update with external_word_Relocation::spec_for_immediate().

TobiHartmann

Looks good as stop-the-gap solution. Do you think there are opportunities to improve the GrowableArray situation?

vnkozlov · 2024-06-26T17:30:08Z

Looks good as stop-the-gap solution. Do you think there are opportunities to improve the GrowableArray situation?

I have already RFE for that: https://bugs.openjdk.org/browse/JDK-8334691

TobiHartmann · 2024-06-26T18:54:54Z

Ah, perfect! Thanks.

vnkozlov · 2024-06-27T01:24:16Z

@dean-long, are you fine with latest version?

dean-long

Looks good.

vnkozlov · 2024-06-27T03:33:06Z

Thank you, Dean and Tobias, for reviews.

vnkozlov · 2024-06-27T03:33:21Z

/integrate

openjdk · 2024-06-27T03:34:05Z

Going to push as commit 6682305.
Since your change was applied there have been 39 commits pushed to the master branch:

3796fdf: 8328536: javac - crash on unknown type referenced in yield statement
07bc523: 8334670: SSLSocketOutputRecord buffer miscalculation
4ebb771: 8334769: Shenandoah: Move CodeCache_lock close to its use in ShenandoahConcurrentNMethodIterator
817edcb: 8331411: Shenandoah: Reconsider spinning duration in ShenandoahLock
bffc848: 8333755: NumberFormat integer only parsing breaks when format has suffix
b5d5896: 8335108: Build error after JDK-8333658 due to class templates
5883a20: 8334437: De-duplicate ProxyMethod list creation
8591eff: 8332103: since-checker - Add missing @ since tags to java.desktop
8374d16: 8335006: C2 SuperWord: add JMH benchmark VectorLoadToStoreForwarding.java
4ffc5e6: 8326705: Test CertMsgCheck.java fails to find alert certificate_required
... and 29 more: https://git.openjdk.org/jdk/compare/71a692ab435fdeea4ce8f8db7a55dd735c7c5016...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-27T03:34:10Z

@vnkozlov Pushed as commit 6682305.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out

dc58613

openjdk bot added the rfr Pull request is ready for review label Jun 24, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jun 24, 2024

Use external_word_Relocation::spec_for_immediate() instead of relocIn…

6da56e2

…fo::none

TobiHartmann approved these changes Jun 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 26, 2024

dean-long approved these changes Jun 27, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 27, 2024

openjdk bot closed this Jun 27, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 27, 2024

vnkozlov deleted the 8334779 branch June 27, 2024 03:35

8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out #19871

8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out #19871

Uh oh!

Conversation

vnkozlov commented Jun 24, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 24, 2024

Uh oh!

openjdk bot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 24, 2024

Uh oh!

mlbridge bot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dean-long commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 25, 2024

Uh oh!

dean-long commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 25, 2024

Uh oh!

dean-long commented Jun 25, 2024

Uh oh!

vnkozlov commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov commented Jun 26, 2024

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jun 26, 2024

Uh oh!

TobiHartmann commented Jun 26, 2024

Uh oh!

vnkozlov commented Jun 27, 2024

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jun 27, 2024

Uh oh!

vnkozlov commented Jun 27, 2024

Uh oh!

openjdk bot commented Jun 27, 2024

Uh oh!

openjdk bot commented Jun 27, 2024

Uh oh!

Uh oh!

vnkozlov commented Jun 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 24, 2024 •

edited

Loading

mlbridge bot commented Jun 24, 2024 •

edited

Loading

vnkozlov commented Jun 26, 2024 •

edited

Loading