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

8251854: [macosx] Java forces the use of discrete GPU #1139

Closed
wants to merge 1 commit into from

Conversation

mrserb
Copy link
Member

@mrserb mrserb commented Nov 10, 2020

This is a review request for the bug particularly fixed some time ago:
https://mail.openjdk.java.net/pipermail/2d-dev/2015-May/005425.html

In that review request it was found that the old fix does not work well in all cases, see:
https://mail.openjdk.java.net/pipermail/2d-dev/2015-August/005611.html

The current fix updates an embedded plist.info, so the java will not require
discrete graphics by default, same as for any other applications.

The discrete card will be used:

  1. If macOS decided to enable it for some reason
  2. If the java app sets/uses a full-screen window
  3. If the user disable "automatic graphics switching" in the system preferences
  4. If an external monitor is connected to the laptop

In other cases, the integrated graphics will be used, which should be fine in most cases since this graphic is used/tested in the mbp 13/etc. This is not only about rendering performance but also about startup performance, on my current new laptop mbp 16 + Cataline 10.15.7 the switching discrete/integrated causes unexpected delays up to 10 seconds.

Note that the new "metal" pipeline also does not require discrete graphics.

The documentation for NSSupportsAutomaticGraphicsSwitching:
https://developer.apple.com/documentation/bundleresources/information_property_list/nssupportsautomaticgraphicsswitching

I'll create a release note after approval.

Performance numbers:
https://mail.openjdk.java.net/pipermail/2d-dev/2020-August/010979.html
Old review request:
https://mail.openjdk.java.net/pipermail/2d-dev/2020-August/010973.html


Progress

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

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

  • JDK-8251854: [macosx] Java forces the use of discrete GPU

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 10, 2020

👋 Welcome back serb! 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 Nov 10, 2020

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

  • build

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 build build-dev@openjdk.org label Nov 10, 2020
@mrserb
Copy link
Member Author

mrserb commented Nov 10, 2020

/label add awt 2d

@mrserb
Copy link
Member Author

mrserb commented Nov 10, 2020

/label add build

@openjdk openjdk bot added awt client-libs-dev@openjdk.org 2d client-libs-dev@openjdk.org labels Nov 10, 2020
@openjdk
Copy link

openjdk bot commented Nov 10, 2020

@mrserb
The awt label was successfully added.

The 2d label was successfully added.

@openjdk
Copy link

openjdk bot commented Nov 10, 2020

@mrserb The build label was already applied.

@mrserb mrserb marked this pull request as ready for review Nov 10, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 10, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 10, 2020

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 10, 2020

I'm still somewhat unsure whether we want to do this in all cases. As mentioned offline, the discrete GPU will be unused (might as well not be there) for almost all Java applications when using a single screen. When the MacBook Pro is running on battery, this seems like a good thing, but when it is plugged in, it seems like we wasting the discrete GPU. It's too bad Apple doesn't provide a way for an application to hint whether they would like to use the discrete GPU if available.

I want to run some performance tests with JavaFX, since it will likely impact JavaFX applications.

Copy link
Member

@erikj79 erikj79 left a comment

Change looks ok from a build point of view, but I can't comment on the validity and implications of using this key.

@openjdk
Copy link

openjdk bot commented Nov 10, 2020

@mrserb This change is no longer ready for integration - check the PR body for details.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 10, 2020

I ran a 3D lighting test that is designed to be a GPU stress test. It's a worst case, to be sure, but it take 10 times as long to render with the integrated GPU as it does with the discrete GPU:

attenuation.LightingSample: 500 large quads
discrete GPU: 23.5 fps
integrated GPU: 2.34 fps

In a more realistic example of drawing a large number of 2D vectors, it runs 35% slower with the integrated GPU:

Vector charting test, oval clip
discrete GPU: 41.1 fps
integrated GPU: 26.6 fps

I see similar results in the performance numbers you listed above.

An application developer who packages up their JDK, for example, using jpackage, can make the decision for themselves. Application developers who rely on the JDK as delivered will get whatever we choose as the default. So we need to be sure that the benefit of doing this justifies the performance hit.

@mrserb
Copy link
Member Author

mrserb commented Nov 10, 2020

The difference between the two is that, if the integrated card is default then it is possible to force the discrete card if needed, but it is not possible to force the integrated card if discrete is the default. In the end, it is a laptop, it will work longer on an integrated card.

@victordyakov
Copy link

victordyakov commented Nov 17, 2020

@kevinrushforth @prrace could you please review?

@prrace
Copy link
Contributor

prrace commented Nov 17, 2020

@kevinrushforth @prrace could you please review?

As we discussed yesterday, it is reviewed but not ready to be approved.
We are waiting for a reply from Apple.

@mrserb
Copy link
Member Author

mrserb commented Nov 17, 2020

@kevinrushforth @prrace could you please review?

As we discussed yesterday, it is reviewed but not ready to be approved.
We are waiting for a reply from Apple.

@prrace
We are waiting for it for three months already. If it is possible then not sure why other applications did not found any possible ways to force one specific GPU. What we are wating for?

@victordyakov
Copy link

victordyakov commented Dec 9, 2020

@mrserb @prrace what is a back up plan in case we will not have a reply from Apple? (as we do not have it for 4! months). Definitely we need a plan B

@prrace
Copy link
Contributor

prrace commented Dec 12, 2020

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Dec 12, 2020
@openjdk
Copy link

openjdk bot commented Dec 12, 2020

@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@mrserb please create a CSR request and add link to it in JDK-8251854. This pull request cannot be integrated until the CSR request is approved.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Dec 12, 2020
@mrserb
Copy link
Member Author

mrserb commented Dec 24, 2020

@mlbridge
Copy link

mlbridge bot commented Dec 24, 2020

Mailing list message from Philip Race on 2d-dev:

From the CSR ;

This change also improves the startup performance, on my current new
laptop mbp 16 + BigSur 11.1 the switching discrete/integrated causes
unexpected delays up to 10 seconds.

This also has to be a bug. I thought it had gone away. Have we reported
that to Apple ?
If not we should do so ASAP.

-phil.

On 12/23/20 5:04 PM, Sergey Bylokhov wrote:

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 21, 2021

@mrserb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 18, 2021

@mrserb This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org build build-dev@openjdk.org csr Pull request needs approved CSR before integration rfr Pull request is ready for review
5 participants