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

8263125: During deoptimization vectors should reassign scalarized payload after all objects are reallocated. #2924

Closed
wants to merge 3 commits into from

Conversation

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Mar 10, 2021

Currently during deoptimization Vector's payload field values are restored during Vector reallocation:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L155

But for scalar-replaced values this is not correct because payload box object could be re-allocated after allocation of this vector. Scalar-replaced payload should be restored during regular fields reassignment (Deoptimization::reassign_fields() change).

I renamed incorrect eliminate_* names for methods which restore/reallocate objects and locks.

I added checks for EliminateAutoBox and EnableVectorAggressiveReboxing optimizations which can replace allocations with scalar objects independent from Escape Analysis.

I added prints for unexpected StackValue values (stackValue.cpp) and for Vector debug info location type (location.cpp).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263125: During deoptimization vectors should reassign scalarized payload after all objects are reallocated.

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2924/head:pull/2924
$ git checkout pull/2924

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 10, 2021

👋 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 openjdk bot added the rfr label Mar 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@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.

@openjdk openjdk bot added the hotspot label Mar 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 10, 2021

Webrevs

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 10, 2021

/label add hotspot-compiler

@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@vnkozlov
The hotspot-compiler label was successfully added.

Copy link

@iwanowww iwanowww left a comment

I vividly remember I fixed the very same problem before in Panama.

More specifically, why doesn't VectorSupport::allocate_vector_payload handle the case?

Handle VectorSupport::allocate_vector_payload(InstanceKlass* ik, frame* fr, RegisterMap* reg_map, ScopeValue* payload, TRAPS) {
  if (payload->is_location() &&
      payload->as_LocationValue()->location().type() == Location::vector) {
    // Vector value in an aligned adjacent tuple (1, 2, 4, 8, or 16 slots).
    Location location = payload->as_LocationValue()->location();
    return allocate_vector_payload_helper(ik, fr, reg_map, location, THREAD); // safepoint
  } else {
    // Scalar-replaced boxed vector representation.
    StackValue* value = StackValue::create_stack_value(fr, reg_map, payload);
    return value->get_obj();
  }
}

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 10, 2021

Here is the order of scalarized objects in this case:

        # jdk.incubator.vector.ShortVector::rearrangeTemplate @ bci:14 (line 2117) L[0]=#ScObj0 L[1]=_ L[2]=#ScObj1 L[3]=#ScObj2 L[4]=_ L[5]=_ STK[0]=#Ptr0x00007f83a4e99850 STK[1]=#Ptr0x00007f83bd684760 STK[2]=#Ptr0x00007f83bca050f0 STK[3]=#4 STK[4]=#ScObj0 STK[5]=#ScObj1
        # ScObj0 jdk/incubator/vector/Short64Vector={ [payload :0]=rsp + #40 }
        # ScObj1 jdk/incubator/vector/Short64Vector$Short64Shuffle={ [payload :0]=#ScObj3 }
        # ScObj2 jdk/incubator/vector/Short64Vector$Short64Mask={ [payload :0]=rsp + #32 }
        # ScObj3 byte[4]={ [0]=rsp + #48 , [1]=rsp + #52 , [2]=rsp + #56 , [3]=RBP }

ScObj1 Short64Shuffle vector will be processed before ScObj3 byte[4] which it references.
VectorSupport::allocate_vector_payload() called only during ScObj1 Short64Shuffle reallocation when ScObj3 byte[4] is not reallocated yet. As result value->get_obj() return NULL. And that is the bug.

I forgot to say that the bug is triggered only with iterative EA I am working on. Without it byte[] array is not scalarized and everything works.

I also thought about calling modified VectorSupport::allocate_vector_payload() from Deoptimization::reassign_fields() to keep checks in one place but it would require more code changes then suggested fix (which is mostly asserts and debug print).

@iwanowww
Copy link

@iwanowww iwanowww commented Mar 11, 2021

Thanks for the clarifications, Vladimir.

I agree that VectorSupport::allocate_vector_payload is not the right place to handle the problematic case.

Some cleanup suggestions: now you can remove StackValue::create_stack_value()-related code fromVectorSupport::allocate_vector_payload(), replace ScopeValue* payload argument with Location location, and turn
location.type() == Location::vector check into an assert.

@iwanowww
Copy link

@iwanowww iwanowww commented Mar 11, 2021

I renamed incorrect eliminate_* names for methods which restore/reallocate objects and locks

Fully agree that eliminate_allocations/eliminate_locks are misleading, but restore_* still look a bit confusing to me.
What do you think about rematerialize_objects/rematerialize_scalarized_objects/relock_objects/restore_eliminated_locks?

@openjdk
Copy link

@openjdk openjdk bot commented Mar 11, 2021

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

8263125: During deoptimization vectors should reassign scalarized payload after all objects are reallocated.

Reviewed-by: vlivanov, rrich

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:

  • ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
  • e25ad73: 8263430: Uninitialized Method* variables after JDK-8233913
  • 9f6b1d7: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection
  • ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
  • cf1c021: 8263480: ProblemList two jpackage tests on Windows
  • f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
  • b92abac: 8263433: Shenandoah: Don't expect forwarded objects in set_concurrent_mark_in_progress()
  • 15dacca: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
  • 7ed46bd: 8241716: Jpackage functionality to let users choose whether to create shortcuts
  • 3820ab9: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/b7f0b3fc8b556b352fd7593ca674ab8e562c709a...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.

Copy link
Contributor

@reinrich reinrich left a comment

Hello Vladimir,

your change looks good to me. You might want to add the refactoring Vladimir
suggested.

May I ask why there is a special case to reallocate a vectors payload at all. In
other words: why is the method VectorSupport::allocate_vector_payload_helper()
needed? Is it for support of VectorMask?

Thanks, Richard.

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 12, 2021

I renamed incorrect eliminate_* names for methods which restore/reallocate objects and locks

Fully agree that eliminate_allocations/eliminate_locks are misleading, but restore_* still look a bit confusing to me.
What do you think about rematerialize_objects/rematerialize_scalarized_objects/relock_objects/restore_eliminated_locks?

I selected rematerialize_objects and restore_eliminated_locks.

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 12, 2021

Thanks for the clarifications, Vladimir.

I agree that VectorSupport::allocate_vector_payload is not the right place to handle the problematic case.

Some cleanup suggestions: now you can remove StackValue::create_stack_value()-related code fromVectorSupport::allocate_vector_payload(), replace ScopeValue* payload argument with Location location, and turn
location.type() == Location::vector check into an assert.

I removed create_stack_value() code from VectorSupport::allocate_vector_payload() and added asserts for all possible cases I found.

@iwanowww
Copy link

@iwanowww iwanowww commented Mar 12, 2021

May I ask why there is a special case to reallocate a vectors payload at all. In
other words: why is the method VectorSupport::allocate_vector_payload_helper()
needed? Is it for support of VectorMask?

(I assume you are asking about VectorSupport::allocate_vector_payload().)

Special handling is needed because Vector/VectorMask is represented as a single value in scalarized form, but in the boxed form it's a pair of instances: Vector/VectorMask + primitive array (holding the payload). reassign_fields doesn't handle such case and that's why it is special-cased.

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 12, 2021

Hello Vladimir,

your change looks good to me. You might want to add the refactoring Vladimir
suggested.

May I ask why there is a special case to reallocate a vectors payload at all. In
other words: why is the method VectorSupport::allocate_vector_payload_helper()
needed? Is it for support of VectorMask?

Thanks, Richard.

@iwanowww may know correct answer.

As I understand it is because vectors need to re-allocate additional payload array to store vectors element:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L129
create_stack_value() does not handle new allocations and arrays. It would require a lot changes.

I think it could be done if during debug info generation we describe element's storage as Scalarized array. But it would not help VectorMask as you said.

@reinrich
Copy link
Contributor

@reinrich reinrich commented Mar 12, 2021

May I ask why there is a special case to reallocate a vectors payload at all. In
other words: why is the method VectorSupport::allocate_vector_payload_helper()
needed? Is it for support of VectorMask?

(I assume you are asking about VectorSupport::allocate_vector_payload().)

Special handling is needed because Vector/VectorMask is represented as a single value in scalarized form

And can't the payload for deoptimization be represented as a scalarized array in that case too? Maybe not because of the special handling a VectorMask requires.

I'm probably not familiar enough with the vector implementation, so sorry for bothering.

IIUC @vnkozlov says this could be done but not for VectorMask.

I think it could be done if during debug info generation we describe element's storage as Scalarized array. But it would not help VectorMask as you said.

@reinrich
Copy link
Contributor

@reinrich reinrich commented Mar 12, 2021

I think it could be done if during debug info generation we describe element's storage as Scalarized array. But it would not help VectorMask as you said.

I see. Thanks for answering. Change still looks good to me.

@iwanowww
Copy link

@iwanowww iwanowww commented Mar 12, 2021

And can't the payload for deoptimization be represented as a scalarized array in that case too? Maybe not because of the special handling a VectorMask requires.

Vector is special in a very similar way: debug info contains only vector value location. Custom logic is needed to turn it into scalarized array representation. And then there's still a step required to allocate the corresponding typed vector box which wraps the payload.

I'm not saying it is not possible to represent current on-heap shape solely in debug info, but it would require significant refactorings/enhancements of existing machinery. And would complicate possible changes of the on-heap vector/mask shape. It is not something "cut in stone" and can evolve over time.

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 12, 2021

And can't the payload for deoptimization be represented as a scalarized array in that case too? Maybe not because of the special handling a VectorMask requires.

Vector is special in a very similar way: debug info contains only vector value location. Custom logic is needed to turn it into scalarized array representation. And then there's still a step required to allocate the corresponding typed vector box which wraps the payload.

I'm not saying it is not possible to represent current on-heap shape solely in debug info, but it would require significant refactorings/enhancements of existing machinery. And would complicate possible changes of the on-heap vector/mask shape. It is not something "cut in stone" and can evolve over time.

Yes, I said it too. It can be done but code would be much larger and more complicated.
And, as Vladimir I. correctly pointed, VectorAPI is new code which can be evolved. To have specialized code for it makes it easy to do experiments.

Thank you @iwanowww and @reinrich for reviews.

@vnkozlov
Copy link
Contributor Author

@vnkozlov vnkozlov commented Mar 12, 2021

/integrate

@openjdk openjdk bot closed this Mar 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@vnkozlov Since your change was applied there have been 43 commits pushed to the master branch:

  • 65421fa: 8213177: GlobalCounter::CSContext could be an enum class
  • a9b156d: 8258414: OldObjectSample events too expensive
  • 0bbe064: 8263354: Accumulated C2 code cleanups
  • aa33443: 8262454: Handshake timeout improvements, single target, kill unfinished thread
  • ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
  • e25ad73: 8263430: Uninitialized Method* variables after JDK-8233913
  • 9f6b1d7: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection
  • ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
  • cf1c021: 8263480: ProblemList two jpackage tests on Windows
  • f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
  • ... and 33 more: https://git.openjdk.java.net/jdk/compare/b7f0b3fc8b556b352fd7593ca674ab8e562c709a...master

Your commit was automatically rebased without conflicts.

Pushed as commit a6e056f.

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

@vnkozlov vnkozlov deleted the JDK-8263125 branch Mar 12, 2021
@reinrich
Copy link
Contributor

@reinrich reinrich commented Mar 12, 2021

I see. Thanks for elaborating on my question!

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 12, 2021

Mailing list message from Reingruber, Richard on hotspot-dev:

I see. Thanks again for answering my questions.

Richard.

-----Original Message-----
From: hotspot-dev <hotspot-dev-retn at openjdk.java.net> On Behalf Of Vladimir Ivanov
Sent: Freitag, 12. M?rz 2021 12:33
To: hotspot-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR: 8263125: During deoptimization vectors should reassign scalarized payload after all objects are reallocated. [v2]

On Fri, 12 Mar 2021 10:54:44 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

And can't the payload for deoptimization be represented as a scalarized array in that case too? Maybe not because of the special handling a VectorMask requires.

Vector is special in a very similar way: debug info contains only vector value location. Custom logic is needed to turn it into scalarized array representation. And then there's still a step required to allocate the corresponding typed vector box which wraps the payload.

I'm not saying it is not possible to represent current on-heap shape solely in debug info, but it would require significant refactorings/enhancements of existing machinery. And would complicate possible changes of the on-heap vector/mask shape. It is not something "cut in stone" and can evolve over time.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2924

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants