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

Revert "Preserve ymm/zmm registers on x" #15999

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

BradleyWood
Copy link
Member

@BradleyWood BradleyWood commented Sep 27, 2022

This reverts PR #14632. A 10%+ performance
regression (issue #15716) reported on JDK 8
was isolated to #14632. Reverting this PR
should not have any function change, it
was only intended to support the Vector
API JEPs which have not yet been enabled
on x86 in OpenJ9. A previous attempt at
disabling just the extended vector register
preservation did not recoup the performance,
but only completely reverting this PR
restored the regression. This suggests
there is something with the way the helpers
have been restructured that is causing the
performance degradation. This PR will be
reverted while the cause is investigated.

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 28, 2022

This will require an explanation for why this is necessary. Can you add the rationale to the commit message please?

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 28, 2022

Jenkins test sanity xlinux,osx,win jdk17

In the meantime, I'll launch PR testing.

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 28, 2022

For the record lest it get lost in the next push, JDK17 builds and sanity testing passed on all platforms.

@BradleyWood
Copy link
Member Author

This will require an explanation for why this is necessary. Can you add the rationale to the commit message please?

I have updated the commit message.

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 28, 2022

Can you also add: "A previous attempt at disabling just the extended vector register preservation did not recoup the performance, but only completely reverting this PR restored the regression. This suggests there is something with the way the helpers have been restructured that is causing the performance degradation. This PR will be reverted while the cause is investigated."

@pshipton
Copy link
Member

Is this something we'll want in the 0.35 release?

@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 1991, 2022 IBM Corp. and others
* Copyright (c) 1991, 2021 IBM Corp. and others
Copy link
Member

Choose a reason for hiding this comment

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

This copyright shouldn't be reverted to 2021, the file is modified by other commits in 2022.

@@ -1,4 +1,4 @@
dnl Copyright (c) 1991, 2022 IBM Corp. and others
dnl Copyright (c) 1991, 2021 IBM Corp. and others
Copy link
Member

Choose a reason for hiding this comment

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

This copyright shouldn't be reverted to 2021, the file is modified by other commits in 2022.

This reverts PR eclipse-openj9#14632. A 10%+ performance
regression (issue eclipse-openj9#15716) reported on JDK 8
was isolated to eclipse-openj9#14632. Reverting this PR
should not have any function change, it
was only intended to support the Vector
API JEPs which have not yet been enabled
on x86 in OpenJ9. A previous attempt at
disabling just the extended vector register
preservation did not recoup the performance,
but only completely reverting this PR
restored the regression. This suggests
there is something with the way the helpers
have been restructured that is causing the
performance degradation. This PR will be
reverted while the cause is investigated.
Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

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

Recent pushes have only been to commit messages or comments. No need to re-run CI.

@0xdaryl 0xdaryl merged commit efe6833 into eclipse-openj9:master Sep 29, 2022
@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 29, 2022

Is this something we'll want in the 0.35 release?

Yes, if it is not too late and this PR has a chance to soak in at least one test cycle.

@BradleyWood : could you prepare the 0.35 PR and have it ready for us to merge please?

BradleyWood added a commit to BradleyWood/openj9 that referenced this pull request Nov 17, 2022
…rt-14632"

This reverts commit efe6833, reversing
changes made to 1d1b4f8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants