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

8304825: MacOS metal pipeline - window isn't painted if created during display sleep #13230

Closed
wants to merge 1 commit into from

Conversation

avu
Copy link
Contributor

@avu avu commented Mar 29, 2023

Use one display link thread per MTLContext. Adjust the refresh rate with the current display.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8304825: MacOS metal pipeline - window isn't painted if created during display sleep

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13230/head:pull/13230
$ git checkout pull/13230

Update a local copy of the PR:
$ git checkout pull/13230
$ git pull https://git.openjdk.org/jdk.git pull/13230/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13230

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13230.diff

Webrev

Link to Webrev Comment

…g display sleep

Use one display link thread per MTLContext. Adjust refresh rate with current display.
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 29, 2023

👋 Welcome back avu! 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 Mar 29, 2023
@openjdk
Copy link

openjdk bot commented Mar 29, 2023

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

  • client

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 client client-libs-dev@openjdk.org label Mar 29, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 29, 2023

Webrevs

@aghaisas
Copy link
Contributor

I tested the test program of this bug and SwingSet2 - with and without external monitor attached.

  • This patch fixes the reported issue. Also, I did not see any regression in SwingSet2 while moving the test window between external monitors. All regression tests also ran successfully with this patch.
  • This patch has a negative impact on rendering performance.
    SwingMark tests show very poor numbers on x64 based macs. There is a performance decrease on M1 macs as well.
    I think, this has to do with removal of a dedicated blitCommandQueue.

We need to address the performance degradation that this patch causes or look for a simpler solution.

@avu
Copy link
Contributor Author

avu commented Mar 30, 2023

Thanks for the testing. I've run only RenderPerf and only on M1 but it didn't show any regressions (there were some improvements actually) I've removed the dedicated blitCommandQueue because it caused the unpredicted order of rendered frames to appear on the screen. In general, it's recommended to use only one command queue per application. If we still want to use a separate queue, we need to provide some synchronization here https://developer.apple.com/documentation/metal/resource_synchronization/synchronizing_events_within_a_single_device?language=objc .

@avu
Copy link
Contributor Author

avu commented Mar 30, 2023

@aghaisas could you return back the blit command queue in my patch and see if it resolves the performance issue with SwingMark? I'm not completely sure that it's the main reason for the performance degradation. Another possible reason is the increased amount of start/stop displaylink calls because of changes in the logic that manages them.

@aghaisas
Copy link
Contributor

aghaisas commented Apr 3, 2023

I modified the patch to add back blitCommandQueue and tested. Below is the comparison between performance results for a build without this PR & a build with (this PR + adding back blitCommandQueue)

  • on x64 based systems : Swingmark performance is back to normal, but, one of the J2DBenchmark test shows 20% degradation.
  • on aarch64 based systems : Swingmark performance still indicates -15% (It was -20% without adding back the blitCommandQueue

This testing indicates that there is an additional performance hit due to the change in logic apart from blitCommandQueue removal.

@aghaisas
Copy link
Contributor

aghaisas commented Apr 3, 2023

I suggest we fix this bug on by either of the two options below-

  • Option 1 : Recreating the CVDisplaylink on screen sleep/wake events
  • Option 2 : As noted by you as a JBS comment - Actually, the simple hack to resolve this particular issue is to replace this call with CVDisplayLinkCreateWithCGDisplay(CGMainDisplayID(), &displayLink)

I have implemented and tested Option 1. I have not tried Option 2 yet.

The code refactoring that you have done can be done as a separate JBS issue if needed in future. What do you think?

@avu
Copy link
Contributor Author

avu commented Apr 4, 2023

OK, let's try Option 1. It still may be useful to recreate CVDisplaylink on the fly, for example if macOS switch GPU from internal to discrete one. I suppose my changes could be adjusted with this approach and pushed as a separate PR.

@avu avu closed this Apr 4, 2023
@avu
Copy link
Contributor Author

avu commented Apr 6, 2023

on x64 based systems : Swingmark performance is back to normal, but, one of the J2DBenchmark test shows 20% degradation.

@aghaisas what particular test showed the regression, I've tested x64 and aarch64 macs with different tests from J2DBench and haven't found regressions yet. Also, could you provide your J2DBench options file?

@aghaisas
Copy link
Contributor

aghaisas commented Apr 6, 2023

Those are the test batches run on internal servers.
I will see if I can construct and share the J2DBench option file that shows the difference.

The results that prompted my review comments-

**J2DBench test related to opaque image rendering **

Compared to the current mainline metal rendering pipeline performance -

Architecture Your fix Your fix + adding back BlitCommandQueue
x64 -88% degradation -20% degradation
aarch64 -4% degradation -1% degradation (negligible)

Swingmark test shows -

Architecture Your fix Your fix + adding back BlitCommandQueue
x64 > -100% degradation Equal to mainline
aarch64 -20% degradation -15% degradation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org rfr Pull request is ready for review
2 participants