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

Cleaning for Concurrent Scavenger initialization #15610

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

dmitripivkine
Copy link
Contributor

  • Concurrent Scavenger can be run at any platform where it is enabled,
    there is no extra check required
  • There is no need to enforce usage Software Range Check Barruer on
    platforms where HW is not available
  • add extensions->softwareRangeCheckReadBarrierForced to indicate
    -XXgc:softwareRangeCheckReadBarrier is requested explicitly, stop use
    extensions->softwareRangeCheckReadBarrier for this purpose

Signed-off-by: Dmitri Pivkine Dmitri_Pivkine@ca.ibm.com

@dmitripivkine
Copy link
Contributor Author

fixing #15590

@pshipton pshipton requested a review from amicic July 25, 2022 16:38
@amicic
Copy link
Contributor

amicic commented Jul 26, 2022

Introduction of Forced flag and the change of logic seems to be right, but it would be more consistent with the rest of status/control flags related to CS to have the flag at OMR side, since all those flags are already in OMR.

If doing that (touching OMR code), I'd also consider getting rid of one of these 2 flags: softwareRangeCheckReadBarrier or concurrentScavengerHWSupport - they seem to be just a complement of each other. There are also related wrappers to be considered for removal. Not forcing this - think if it makes sense and which one would make more sense to leave and which to remove, if at all.

@dmitripivkine
Copy link
Contributor Author

Introduction of Forced flag and the change of logic seems to be right, but it would be more consistent with the rest of status/control flags related to CS to have the flag at OMR side, since all those flags are already in OMR.

I thought about this. I can do it if you insist. The reason I put flag at OpenJ9 side is despite this flag has similar name it relates with Java command line option only. It has nothing to deal with from OMR side.

If doing that (touching OMR code), I'd also consider getting rid of one of these 2 flags: softwareRangeCheckReadBarrier or concurrentScavengerHWSupport - they seem to be just a complement of each other. There are also related wrappers to be considered for removal. Not forcing this - think if it makes sense and which one would make more sense to leave and which to remove, if at all.

I thought about this too. The hidden condition is included to these flags is CS Active (CS Active plus Software Range Check Read Barrier and CS Active plus HW Support) so, we can not say softwareRangeCheckReadBarrier = ! concurrentScavengerHWSupport without check of third condition. There is why I left it as-is. Again, if you insist, I can modify this logic too.
Please let me know what do you think.

@amicic
Copy link
Contributor

amicic commented Jul 26, 2022

still think the flag belongs to omr side.
the second part about removing is not that important to change, but you could add a comment for one of the flags it's a complement of the other

- Concurrent Scavenger can be run at any platform where it is enabled,
there is no extra check required
- There is no need to enforce usage Software Range Check Barruer on
platforms where HW is not available
- use extensions->softwareRangeCheckReadBarrierForced to indicate
-XXgc:softwareRangeCheckReadBarrier is requested explicitly, stop use
extensions->softwareRangeCheckReadBarrier for this purpose


Signed-off-by: Dmitri Pivkine <Dmitri_Pivkine@ca.ibm.com>
@dmitripivkine
Copy link
Contributor Author

Introduce flag at OMR side: eclipse/omr#6625

@dmitripivkine
Copy link
Contributor Author

Jenkins test sanity xLinux,zLinux jdk11

@amicic amicic merged commit d4f0a0c into eclipse-openj9:master Jul 27, 2022
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.

None yet

2 participants