Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8316756: C2 EA fails with "missing memory path" when encountering unsafe_arraycopy stub call #17347

Closed
wants to merge 4 commits into from

Conversation

tobiasholenstein
Copy link
Contributor

@tobiasholenstein tobiasholenstein commented Jan 10, 2024

Before #5259 the graph of the following program looked like this during Escape Analysis:

static int test() {
    MyClass obj = new MyClass(); // Non-escaping to trigger Escape Analysis
    UNSAFE.copyMemory(null, SRC_BASE, null, DST_BASE, 4);
    obj.x = 42;
    return obj.x;
}

With MemBarCPUOrder:
working

Setting RC_NARROW_MEM flag in LibraryCallKit::inline_unsafe_copyMemory() removes the 428 MergeMem node. Without a MergeMem node after 432 StoreB EA failes in Phase 2 of ConnectionGraph::split_unique_types(...) when trying to push the allocation's users on the appropriate worklist - 429 CallLeafNoFP is not an expected user of 428 StoreB. Therefore the assert "EA: missing memory path" is hit.
Without MemBarCPUOrdera and after setting RC_NARROW_MEM:
failing

Proposed Fix

Dropping the RC_NARROW_MEM flag in LibraryCallKit::inline_unsafe_copyMemory() causes the introduction of a MergeMem between StoreB and CallLeafNoFP, so the corresponding code in EA doesn't encounter a CallLeafNoFP anymore:
fixed

Testing: Tier1-4 passed


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-8316756: C2 EA fails with "missing memory path" when encountering unsafe_arraycopy stub call (Bug - P3)

Reviewers

Contributors

  • Vladimir Kozlov <kvn@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17347

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 10, 2024

👋 Welcome back tholenstein! 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
Copy link

openjdk bot commented Jan 10, 2024

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

  • hotspot-compiler

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.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 10, 2024
@tobiasholenstein tobiasholenstein marked this pull request as ready for review January 11, 2024 16:09
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 11, 2024
@mlbridge
Copy link

mlbridge bot commented Jan 11, 2024

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reviewed by @iwanowww.

I have too many question about this.

Based on code and test UNSAFE.copyMemory() copies "native" memory which should not affect anything. #5259 sets RC_NARROW_MEM exactly for that as I understand.

Flag setting (StoreB nodes) in JavaThread::_doing_unsafe_access is also not affected but it is volatile field and these stores should be staying where they are. They can't go up or down. And it should be accomplished by some kind of barriers between StoreB and unsafe_Arraycopy call. But the call's memory edge should not point to StoreB - it is incorrect since it does not affect that field in this case. Call's memory should point to root memory in this case.

Operating on fields of new MyClass object could be moved around and object can be eliminated since it does not escape.

@vnkozlov
Copy link
Contributor

vnkozlov commented Jan 11, 2024

JavaThread::_doing_unsafe_access field is checked by runtime when we SEGV to find that it happens in unsafe_arraycopy code. Again, unsafe_arraycopy does not affect this field.

@vnkozlov
Copy link
Contributor

The result of unsafe_arraycopy should not affect memory too.

@iwanowww
Copy link
Contributor

iwanowww commented Jan 11, 2024

Yes, proposed fix undoes some optimizations JDK-8269119 introduced: RC_NARROW_MEM was introduced to optimally represent memory effects of native-to-native memory copy. The whole off-heap memory state is tracked by a single raw memory slice, so it qualifies to be treated as operating on narrow memory. The IR shape as it is now looks fine. C2 models all non-heap memory operations as raw accesses, but they are serialized on a single memory alias (raw memory).

IMO the bug is in EA code which doesn't properly handle calls with narrow memory effects.

@iwanowww
Copy link
Contributor

iwanowww commented Jan 11, 2024

Flag setting (StoreB nodes) in JavaThread::_doing_unsafe_access is also not affected but it is volatile field and these stores should be staying where they are. They can't go up or down.

In a private discussion with @tobiasholenstein, I proposed to move those stores into the corresponding stub. It would complicate the implementation a bit (platform-specific vs cross-platform implementation), but simplify things on IR level.

@vnkozlov
Copy link
Contributor

all non-heap memory operations as raw accesses.

Right. StoreB is also RAW access. My previous comment is incorrect - StoreB can be memory for unsafe_arraycopy and such it can preserve the order of execution.

I agree with moving Stores into stub. C2 don't need to know about them.

@tobiasholenstein
Copy link
Contributor Author

Flag setting (StoreB nodes) in JavaThread::_doing_unsafe_access is also not affected but it is volatile field and these stores should be staying where they are. They can't go up or down.

In a private discussion with @tobiasholenstein, I proposed to move those stores into the corresponding stub. It would complicate the implementation a bit (platform-specific vs cross-platform implementation), but simplify things on IR level.

So you think we should go for that solution instead of this fix?

@vnkozlov
Copy link
Contributor

Flag setting (StoreB nodes) in JavaThread::_doing_unsafe_access is also not affected but it is volatile field and these stores should be staying where they are. They can't go up or down.

In a private discussion with @tobiasholenstein, I proposed to move those stores into the corresponding stub. It would complicate the implementation a bit (platform-specific vs cross-platform implementation), but simplify things on IR level.

So you think we should go for that solution instead of this fix?

Yes. You may still need to fix EA to recognize RAW memory for unsafe_arraycopy.

@vnkozlov
Copy link
Contributor

I think EA expect MergeMem node before all calls as memory edge. Even if we move StoreB inside stub we will have runtime call node. I made test more complicated to see how it will react to such runtime calls:

    static int[] test() {
         int[] src = new int[4];
         int[] dst = new int[4];
         MyClass obj = new MyClass();
         UNSAFE.copyMemory(src, 0, dst, 0, 4);
         obj.x = 42;
         dst[1] = obj.x;
         return dst;
    }

and it hit same assert:

 119  Proj  === 117  [[ 15 654 640 644 178 178 178 178 ]] #2  Memory: @rawptr:BotPTR, idx=Raw; !jvms: UnsafeArrayCopy::test @ bci:8 (line 25)
 640  CallLeafNoFP  === 181 1 119 8 1 (62 91 30 1 ) [[ 641 643 ]] # unsafe_arraycopy void ( NotNull *+bot, NotNull *+bot, long, half ) !jvms: Unsafe::copyMemory @ bci:29 (line 806) UnsafeArrayCopy::test @ bci:26 (line 26)

@vnkozlov
Copy link
Contributor

I am investigating how to handle it in EA. I understand that it is not easy for you.

@vnkozlov
Copy link
Contributor

@tobiasholenstein I suggest to file separate REF to move StoreB which updates JavaThread::_doing_unsafe_access into stub and work on it.

For this issue we need to fix EA. I will work on patch.

@vnkozlov
Copy link
Contributor

I added 8316756.patch for EA fix to bug report. Please, also add my test case (with local arrays and object) to your test.

@tobiasholenstein
Copy link
Contributor Author

/contributor add @vnkozlov

@openjdk
Copy link

openjdk bot commented Jan 15, 2024

@tobiasholenstein
Contributor Vladimir Kozlov <kvn@openjdk.org> successfully added.

@tobiasholenstein
Copy link
Contributor Author

Flag setting (StoreB nodes) in JavaThread::_doing_unsafe_access is also not affected but it is volatile field and these stores should be staying where they are. They can't go up or down.

In a private discussion with @tobiasholenstein, I proposed to move those stores into the corresponding stub. It would complicate the implementation a bit (platform-specific vs cross-platform implementation), but simplify things on IR level.

So you think we should go for that solution instead of this fix?

Yes. You may still need to fix EA to recognize RAW memory for unsafe_arraycopy.

I applied your patch for EA and added the test. Thanks!

tier1-4 pass

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.

@vnkozlov
Copy link
Contributor

Did you file RFE for StoreB move?

@openjdk
Copy link

openjdk bot commented Jan 15, 2024

@tobiasholenstein 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:

8316756: C2 EA fails with "missing memory path" when encountering unsafe_arraycopy stub call

Co-authored-by: Vladimir Kozlov <kvn@openjdk.org>
Reviewed-by: kvn, thartmann, chagedorn

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 97 new commits pushed to the master branch:

  • ee4d9aa: 8323659: LinkedTransferQueue add and put methods call overridable offer
  • 5045839: 8323635: Test gc/g1/TestHumongousAllocConcurrentStart.java fails with -XX:TieredStopAtLevel=3
  • 44a9392: 8323780: Serial: Remove unused _full_collections_completed
  • 5906240: 8323716: Only print ZGC Phase Switch events in hs_err files when running with ZGC
  • e01f6da: 8320175: [BACKOUT] 8316533: C2 compilation fails with assert(verify(phase)) failed: missing Value() optimization
  • 8abaf11: 8323715: Serial: Move genMemoryPools to serial folder
  • 6720499: 8323738: Serial: Remove unreachable methods in Generation
  • 36f4b34: 8323122: AArch64: Increase itable stub size estimate
  • b363472: 8318227: RISC-V: C2 ConvHF2F
  • edc0ebb: 8323745: Missing comma in copyright header in TestScope
  • ... and 87 more: https://git.openjdk.org/jdk/compare/88378ed0584c7eb0849b6fc1e361fd8ea0698caf...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 openjdk bot added the ready Pull request is ready to be integrated label Jan 15, 2024
Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please run hs-comp-stress and hs-precheckin-comp as well before integration.

@@ -4006,6 +4006,13 @@ void ConnectionGraph::split_unique_types(GrowableArray<Node *> &alloc_worklist,
if (n == nullptr) {
continue;
}
} else if (n->is_CallLeaf()) {
// Runtime calls with narrow memory input (no MergeMem node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we somehow assert here that we have a call with an intended narrow memory input? Directly asserting that this is an unsafe arraycopy might be too specific. But maybe we can add the following sanity check?

n->as_CallLeaf()->adr_type()->is_rawptr()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it does not always need to be raw memory. But maybe we still want to assert that we have an unsafe arraycopy in this case? If we ever have more valid cases, the assert could easily be adjusted to allow them.

But given how close we are to RDP 2, I suggest to go with this general fix and follow up with an RFE to add that assert if you all agree with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we somehow assert here that we have a call with an intended narrow memory input? Directly asserting that this is an unsafe arraycopy might be too specific. But maybe we can add the following sanity check?

n->as_CallLeaf()->adr_type()->is_rawptr()

@vnkozlov what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is not Narrow memory you will get MergeMem node as Call's memory input which we put onmergemem_worklist and not processing it or its users in this part of code. We have assert(mergemem_worklist.contains(m->as_MergeMem()) instead here.

You can add assert(!n->in(TypeFunc::Memory)->as_MergeMem(), "only narrow memory expected here"); if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we could add this assert as well for expecting a narrow memory input in general. What are your thoughts about explicitly asserting for an unsafe arraycopy when visiting this call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There is another case (DTrace runtime call) with narrow memory: parseHelper.cpp#L54

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we need to integrate this P3 until tomorrow (Thursday), I'd suggest to integrate as-is and add the idea of adding an assert to the follow-up RFE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There is another case (DTrace runtime call) with narrow memory: parseHelper.cpp#L54

That's right. We would need to assert this case as well. But as Tobias suggested, let's move on without any additional assertions for now.

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good to me, too.

@tobiasholenstein
Copy link
Contributor Author

Did you file RFE for StoreB move?

Yes, I filed https://bugs.openjdk.org/browse/JDK-8323813

@tobiasholenstein
Copy link
Contributor Author

Thanks @chhagedorn , @vnkozlov and @TobiHartmann for the reviews! And thanks @iwanowww for the discussion!

/integrate

@openjdk
Copy link

openjdk bot commented Jan 17, 2024

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 17, 2024
@openjdk openjdk bot closed this Jan 17, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 17, 2024
@openjdk
Copy link

openjdk bot commented Jan 17, 2024

@tobiasholenstein Pushed as commit b891721.

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

@TobiHartmann
Copy link
Member

/backport jdk22

@openjdk
Copy link

openjdk bot commented Jan 17, 2024

@TobiHartmann the backport was successfully created on the branch backport-TobiHartmann-b8917214 in my personal fork of openjdk/jdk22. To create a pull request with this backport targeting openjdk/jdk22:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit b8917214 from the openjdk/jdk repository.

The commit being backported was authored by Tobias Holenstein on 17 Jan 2024 and was reviewed by Vladimir Kozlov, Tobias Hartmann and Christian Hagedorn.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22:

$ git fetch https://github.com/openjdk-bots/jdk22.git backport-TobiHartmann-b8917214:backport-TobiHartmann-b8917214
$ git checkout backport-TobiHartmann-b8917214
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22.git backport-TobiHartmann-b8917214

⚠️ @TobiHartmann You are not yet a collaborator in my fork openjdk-bots/jdk22. An invite will be sent out and you need to accept it before you can proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
5 participants