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

8276905: Use appropriate macosx_version_minimum value while compiling metal shaders #6346

Closed
wants to merge 4 commits into from

Conversation

jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Nov 11, 2021

When JDK is built on BigSur with Xcode12.5 and used to run jtreg tests in macOS10.15 with Xcode <=12.4 we are getting compilation error. We dont set any deployment target when when we use xcrun to create .air file and this issue looks similar to https://developer.apple.com/forums/thread/105719. Also https://developer.apple.com/forums/thread/105719 states that even if they set app deployment target they need to explicitly set deployment in "xcrun metal".

Made same change to use -mmacosx-version-min=10.12.00 in our make file. Whether we should keep 10.12.00 or 10.13/14 is open to discussion for metal pipeline.

SwingSet2, J2DDemo & JCK/jtreg runs in CI are green. Also Vitaly(Submitter) has confirmed that patch resolves the issue.


Progress

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

Issue

  • JDK-8276905: Use appropriate macosx_version_minimum value while compiling metal shaders

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6346/head:pull/6346
$ git checkout pull/6346

Update a local copy of the PR:
$ git checkout pull/6346
$ git pull https://git.openjdk.java.net/jdk pull/6346/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6346

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6346.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 11, 2021

👋 Welcome back jdv! 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 Pull request is ready for review label Nov 11, 2021
@openjdk
Copy link

openjdk bot commented Nov 11, 2021

@jayathirthrao The following labels will be automatically applied to this pull request:

  • build
  • client

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

@openjdk openjdk bot added build build-dev@openjdk.org client client-libs-dev@openjdk.org labels Nov 11, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 11, 2021

Webrevs

Copy link
Member

@magicus magicus left a comment

We should not hard-code version numbers like that.

Fortunately for you, there already exists a variable $(MACOSX_VERSION_MIN) that you can use.

@magicus
Copy link
Member

magicus commented Nov 11, 2021

Also, if you did not create this patch yourself, please make sure to use /contributor to give proper credits. Or maybe you mean that Vitaly submitted the bug report, not the patch?

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 11, 2021

We should not hard-code version numbers like that.

Fortunately for you, there already exists a variable $(MACOSX_VERSION_MIN) that you can use.

Thanks for the review i have updated code to use the Macro.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 11, 2021

Also, if you did not create this patch yourself, please make sure to use /contributor to give proper credits. Or maybe you mean that Vitaly submitted the bug report, not the patch?

By Submitter i meant submitter of bug in JBS. I was not able to verify the patch in our CI, so i shared the patch with Vitaly so that he can verify the reproducibility of the issue.

@openjdk
Copy link

openjdk bot commented Nov 11, 2021

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

8276905: Use appropriate macosx_version_minimum value while compiling metal shaders

Reviewed-by: ihse, kcr, erikj, prr

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

  • 7fc344d: 8277028: Use service type documentation as fallback for @provides
  • 35a831d: 8272170: Missing memory barrier when checking active state for regions
  • 02f7900: 8276932: G1: Annotate methods with override explicitly in g1CollectedHeap.hpp
  • fdcd16a: 8277048: Tiny improvements to the specification text for java.util.Properties.load
  • b231f5b: 8276921: G1: Remove redundant failed evacuation regions calculation in RemoveSelfForwardPtrHRClosure
  • ca2efb7: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend
  • 296780c: 8276983: Small fixes to DumpAllocStat::print_stats
  • 8c5f030: 8276453: Undefined behavior in C1 LIR_OprDesc causes SEGV in fastdebug build
  • 176d21d: 8276824: refactor Thread::is_JavaThread_protected
  • 74f3e69: 8277071: [BACKOUT] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
  • ... and 84 more: https://git.openjdk.java.net/jdk/compare/fc0fe256793b33430c1247e0c091150a091da3c4...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 Nov 11, 2021
COMMAND := $(METAL) -c -std=osx-metal2.0 -o $(SHADERS_AIR) $(SHADERS_SRC), \
COMMAND := $(METAL) -c -std=osx-metal2.0 \
-mmacosx-version-min=$(MACOSX_VERSION_MIN) \
-o $(SHADERS_AIR) $(SHADERS_SRC), \
Copy link
Contributor

@prrace prrace Nov 12, 2021

Choose a reason for hiding this comment

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

No .. we decided metal requires macos 10.14 as a minimum. In fact it won't run (by policy) on earlier releases so settting it to what the broader JDK defines as a minimum is not appropriate right now.
And I've no idea what restrictions we'd be placing on metal saying it can only use 10.12 features.
Nor do I know what the xcode 12.4 used to build JDK 17 defaults to and how much a change to either 10.12 or 10.14 might require in re-testing.

So -
we should use 10.14
what's the actual impact of that on a current build using xcode 12.4 - does it change anything we use ?

Copy link
Contributor

@prrace prrace Nov 12, 2021

Choose a reason for hiding this comment

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

bear in mind "impact" might mean metal can't use some new faster code, not just runtime errors

Copy link
Member Author

@jayathirthrao jayathirthrao Nov 15, 2021

Choose a reason for hiding this comment

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

Hi Phil,

Thanks for the review. I also had doubts using 10.12. And yes as you mentioned we dont use Metal pipeline for versions < macOS10.14.

We dont use any Metal API which is specific to >macOS10.14(We had such usage when Lanai was under development but they were removed subsequently). So i dont see any performance impact of making xcrun to use 10.14 as minimum version.

Thanks,
Jay

Copy link
Member

@magicus magicus left a comment

I'm withdrawing my approval until the issues raised by Phil has been resolved.

It sounds like this change need more thorough discussion about what problems you are experience, and what are acceptable solutions to it.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 12, 2021
@erikj79
Copy link
Member

erikj79 commented Nov 12, 2021

The JDK default of 10.12 is an old and rather conservative choice. I basically picked it as that was the oldest I could get away with at the time. This value is meant to be bumped whenever a need for it arises. Given that the oldest Macos version still getting Apple support is 10.15, I see no issue with raising this to at least 10.14, if that is what any part of the JDK requires. The whole point of defining that value is that it should reflect our requirements.

So I propose, bump the number in configure (and jib-profiles.js) and use it in the client makefiles so we fix the problem.

@magicus
Copy link
Member

magicus commented Nov 12, 2021

If we leave -mmacosx-version-min unspecified, will metal pick another value by default silently? And if so, might we be actually lowering the min version even if specifying 10.14?

In any case, it seems that any change here will need thorough testing both on Oracle CI systems and to make sure it solves @jayathirthrao's problem.

@erikj79
Copy link
Member

erikj79 commented Nov 12, 2021

If we leave -mmacosx-version-min unspecified, will metal pick another value by default silently? And if so, might we be actually lowering the min version even if specifying 10.14?

I'm not sure how Xcode picks the default target, but in my experience, it's some combination of the SDK bundled with Xcode and the current system the build is running on. I would assume this tool behaves the same way.

At Oracle we are currently using Xcode 12.4 and Macos 10.15.4, so specifying 10.14 would most likely be a step down from the assumed current default of 10.15. As far as Oracle is concerned, we should be fine with going with 10.15 as that's the oldest version that we will officially support for JDK 18 anyway. I have generally tried to be a bit more conservative with this version requirement though.

I agree that testing is required for such a change.

@prrace
Copy link
Contributor

prrace commented Nov 12, 2021

My understanding is that the flag here affects ONLY the metal compiler - for compiling metal shaders.
And if you don't specify -Dsun.java2d.metal=true (since metal is off by defau�lt) its a 100% no-op for the rest of the JDK.
And already, if you specify Dsun.java2d.metal=true and you are on 10.13 or lower, we do not honour the request so we haven't changed what platforms will work at all if we do it this way. So our effective deployment target for metal is already 10.14

And I also would not be surprised if someone wants to backport this to 17u, in which case a config change would have the effect of making 17u no longer run on macos 12 .. which I guess will happen sometime during the life of the LTS but right now ??

So making it a metal-specific change is what I think we should do FOR NOW and we can have a follow-on fix that aligns both of these .. maybe that is a subsequent JDK 18 fix, or perhaps it should be an early JDK 19 fix ?

@erikj79
Copy link
Member

erikj79 commented Nov 12, 2021

My understanding is that the flag here affects ONLY the metal compiler - for compiling metal shaders. And if you don't specify -Dsun.java2d.metal=true (since metal is off by defau�lt) its a 100% no-op for the rest of the JDK. And already, if you specify Dsun.java2d.metal=true and you are on 10.13 or lower, we do not honour the request so we haven't changed what platforms will work at all if we do it this way. So our effective deployment target for metal is already 10.14

This explanation certainly makes a good case for giving this particular tool invocation special treatment. If we don't want to bump the general deployment target version, then this makes sense.

And I also would not be surprised if someone wants to backport this to 17u, in which case a config change would have the effect of making 17u no longer run on macos 12 .. which I guess will happen sometime during the life of the LTS but right now ??

I would be fine with backporting a general deployment target version change to 10.14 to 17u LTS. Apple are very aggressive with dropping support for older OS versions. We don't really have any obligations to maintain compatibility with legacy versions of macos. We just haven't actively dropped compatibility (which is different from what any particular distributor officially supports) unless there has been some technical limitation before. But, as you have now explained, we already have a runtime guard for this optional feature, so we aren't actually forced to change the global deployment target.

So making it a metal-specific change is what I think we should do FOR NOW and we can have a follow-on fix that aligns both of these .. maybe that is a subsequent JDK 18 fix, or perhaps it should be an early JDK 19 fix ?

I'm ok with a metal specific change here, targeting 10.14, but it needs a comment referencing the global value and explaining that there is a runtime guard around this. Also note that on aarch64 the global value is 11.00.00, and in that case we should use the global value.

@mlbridge
Copy link

mlbridge bot commented Nov 12, 2021

Mailing list message from Kingsley O on client-libs-dev:

Please remove me from your mailing list - thanks

On Thu, Nov 11, 2021 at 3:32 PM Jayathirth D V <jdv at openjdk.java.net> wrote:

@prrace
Copy link
Contributor

prrace commented Nov 12, 2021

" Also note that on aarch64 the global value is 11.00.00, and in that case we should use the global value."

I have no idea what happens if you specify 10.14 to the metal compiler on aarch64
Something else to check although probably the safest option is to make it 11.0 on that architecture

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 15, 2021

" Also note that on aarch64 the global value is 11.00.00, and in that case we should use the global value."

I have no idea what happens if you specify 10.14 to the metal compiler on aarch64 Something else to check although probably the safest option is to make it 11.0 on that architecture

Sure. I will make it to use 11.0 on aarch64 and 10.14 on x64 and add comment mentioning that we have runtime guard to ignore metal pipeline for <10.14 versions. With current 10.12 minimum version patch there is CI run on aarch64 it has not thrown any error but i am not sure how that works.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 15, 2021

" Also note that on aarch64 the global value is 11.00.00, and in that case we should use the global value."

I have no idea what happens if you specify 10.14 to the metal compiler on aarch64 Something else to check although probably the safest option is to make it 11.0 on that architecture

A question popped up while doing this. By making 11.0 as minimum macosx version on aarch64, are we not restricting metal to be used only on >=11.0 on aarch 64?

@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 15, 2021

The JDK itself has a minimum version of 11.0 on macOS aarch64 systems (because apple only supports it on macOS >= 11), so it probably doesn't matter much. You might wish to file a low-priority follow-up issue to change the runtime check to be consistent.

@erikj79
Copy link
Member

erikj79 commented Nov 15, 2021

A question popped up while doing this. By making 11.0 as minimum macosx version on aarch64, are we not restricting metal to be used only on >=11.0 on aarch 64?

Correct, but there are no older versions of Macos on aarch64, so this is what we want. I think the correct behavior here is to use $(MACOSX_VERSION_MIN) on aarch64 and only override this with an explicit 10.14 for x86_64 and explaining why that override is done in a comment.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 15, 2021

The JDK itself has a minimum version of 11.0 on macOS aarch64 systems (because apple only supports it on macOS >= 11), so it probably doesn't matter much. You might wish to file a low-priority follow-up issue to change the runtime check to be consistent.

Thanks Kevin for the clarification and for also clearing up the follow up question i wanted to ask about runtime check update.

Copy link
Member

@erikj79 erikj79 left a comment

Looks good!

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 15, 2021
Copy link
Member

@magicus magicus left a comment

Looks good to me also. Please let Phil have a say as well before integrating.

@magicus
Copy link
Member

magicus commented Nov 15, 2021

Also please update the title of the PR and the JBS issue (they must be the same) to something about that we're changing metal min version, rather than the obscure error that was the result of not having the min version...

@jayathirthrao jayathirthrao changed the title 8276905: Function frag_col has a deployment target which is incompatible with this OS 8276905: Use appropriate macosx_version_minimum value while compiling metal shaders Nov 15, 2021
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Nov 15, 2021
@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 15, 2021

Looks good to me also. Please let Phil have a say as well before integrating.

Thanks Magnus for the review. Yes i have already Re-Requested review from Phil on latest patch.
Also i am waiting on Vitaly(JBS submitter) to verify the latest patch.

prrace
prrace approved these changes Nov 15, 2021
Copy link
Contributor

@prrace prrace left a comment

This should be the most compatible solution.

@jayathirthrao
Copy link
Member Author

jayathirthrao commented Nov 16, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Nov 16, 2021

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

  • 7906eb0: 8277119: Add asserts in GenericTaskQueueSet methods
  • 1c45c8a: 8274757: Cleanup unnecessary calls to Throwable.initCause() in java.management module
  • c06df25: 8274662: Replace 'while' cycles with iterator with enhanced-for in jdk.hotspot.agent
  • 9629627: 8274163: Use String.equals instead of String.compareTo in jdk.jcmd
  • 0bc2683: 8274190: Use String.equals instead of String.compareTo in jdk.internal.jvmstat
  • a9cb8bd: 8274168: Avoid String.compareTo == 0 to check String equality in java.management
  • 20f3872: 8274261: Use enhanced-for instead of plain 'for' in jdk.jcmd
  • b8d33a2: 8277029: JMM GetDiagnosticXXXInfo APIs should verify output array sizes
  • 1d79cfd: 8276229: Stop allowing implicit updates in G1BlockOffsetTable
  • 7719a74: 8277172: Remove stray comment mentioning instr_size_for_decode_klass_not_null on x64
  • ... and 103 more: https://git.openjdk.java.net/jdk/compare/fc0fe256793b33430c1247e0c091150a091da3c4...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 16, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 16, 2021
@openjdk
Copy link

openjdk bot commented Nov 16, 2021

@jayathirthrao Pushed as commit 9a9a157.

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

@jayathirthrao jayathirthrao deleted the JDK-8276905 branch Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org client client-libs-dev@openjdk.org integrated Pull request has been integrated
5 participants