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

[macOS][iOS] Proposal to cache [NS|UI]Screen.MainScreen derived data #9098

Closed
spouliot opened this issue Jun 23, 2022 · 4 comments · Fixed by #9114
Closed

[macOS][iOS] Proposal to cache [NS|UI]Screen.MainScreen derived data #9098

spouliot opened this issue Jun 23, 2022 · 4 comments · Fixed by #9114
Assignees
Labels
difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification

Comments

@spouliot
Copy link
Contributor

Current behavior

DisplayInformation is doing too many native (ObjC) calls on values that rarely changes. Here's a sample of the existing code:

public uint ScreenHeightInRawPixels
{
	get
	{
		var screenSize = NSScreen.MainScreen.Frame.Size;
		var scale = NSScreen.MainScreen.BackingScaleFactor;
		return (uint)(screenSize.Height * scale);
	}
}

That's four native (ObjC) calls to return the Height.

The methods could be optimized, individually, to cache [NSScreen mainScreen] saving one native (ObjC) call for each invocation, example:

public uint ScreenHeightInRawPixels
{
	get
	{
		var screen = NSScreen.MainScreen;
		return (uint)screen.ConvertRectToBacking(screen.Frame).Height;
	}
}

That's only three ObjC calls (and avoids the extra step of multiplication).

Still the following, unavoidable, code pattern remain very inefficient:

var width = di.ScreenHeightInRawPixels;
var height = di.ScreenWidthInRawPixels;

since it requires to recompute the same data twice, requiring issuing again the same native calls (twice three ObjC calls).

If this code ends up being called inside a loop (issue) then performance can suffer a lot. The get_Scale inside "issue" looks like such an occurrence.

Expected behavior

Querying DisplayInformation properties should be fast and not appear as bottleneck inside benchmarks.

IMHO we can assume that

  • Screen changes are infrequent (there's often only one screen for the lifetime of the app);
  • A small delay (e.g. 500ms) to detect a screen change wont cause visual issues [1];
  • The check for the delay is much smaller than the cost of all the native calls involved;

[1] #9027 (about scale) was found reading the code, not reported as an issue (as far as I know).

How to reproduce it (as minimally and precisely as possible)

Create a loop over some properties of DisplayInformation and print the time it takes to go over it.

Workaround

Applications can cache the values.

Works on UWP/WinUI

No response

Environment

Uno.UI / Uno.UI.WebAssembly / Uno.UI.Skia

NuGet package version(s)

No response

Affected platforms

iOS, macOS

IDE

No response

IDE version

No response

Relevant plugins

No response

Anything else we need to know?

This affect fixing #9027 since it means replacing the use of very fast readonly fields with very slow calls to DisplayInformation properties.

Once the replacement is done the MainScreen becomes a large bottleneck inside the Dope benchmark (16 seconds out of the 120 seconds run).

Screen Shot 2022-06-22 at 8 36 30 PM

Proposal

  • Calculate all [NS|UI]Screen-based values in a single method
  • Call that method if more than 500ms (for example) has gone by since last time it was called
  • Change properties to return the cached values
@spouliot spouliot added difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification labels Jun 23, 2022
@jeromelaban
Copy link
Member

Interesting! Does iOS provide the ability to observe UIScreen changes? It's likely to affect the rest of the properties from DisplayInformation for sure.

@spouliot
Copy link
Contributor Author

Does iOS provide the ability to observe UIScreen changes?

IIRC, at least on macOS, you can be notified for some things, but not others (like the actual resolution). I'll check again to be sure...

@spouliot
Copy link
Contributor Author

spouliot commented Jun 23, 2022

AppKit's applicationDidChangeScreenParameters: triggers when:

  • display configuration change (used for DPI and orientation)
  • apps go full screen or come back to a window (TIL as it does not change the config)
    but it does not trigger when:
  • an app change from screen A to screen B

CoreGraphics' CGDisplayRegisterReconfigurationCallback is similar. The API is to track changes screen changes (added, removed monitors) and not track the app's window wrt screens.

Speaking of Window (instead of Application) there's AppKit's NSWindowDidChangeScreenNotification that seems to do the job (for macOS) 😄

There's nothing identical for UIWindow (iOS/iPadOS) but there's [UIScreenModeDidChangeNotification](https://developer.apple.com/documentation/uikit/uiscreenmodedidchangenotification?language=objc) which tracks changes in screen resolution / mode.

@jeromelaban
Copy link
Member

Interesting change indeed. iOS follows a similar pattern, and if we can avoid invoking those APIs repeatedly, it'll help. Also, we're bound to call them more in the future as we reuse pieces coming from WinUI. Every small gain will be good there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants