Add window refresh lock to block window from updating while handler is running.

[cvwillegen]

When a handler is running it may add a lot of widgets to a window. The window keeps updating its layout, leading to an O(n^2) in calls to the refresh() function. When the number of widgets added to a window is doubled, run time is quadrupled (etc).

This leads to slow updates when going from view to view.

My solution is to keep a list of 'dirty' children when a window is locked, so that the children will receive a refresh() call when the window is unlocked. Also, a window is automatically locked when a handler is called that has a window in its interface. No interaction from the user or application programmer is needed to use this functionality.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[HalfWhitt]

This seems highly relevant to #2938.

[freakboy3742]

Thanks for the suggestion/patch. I definitely agree that the "refresh flood" is an issue that we need to address.

I've done a quick initial review, my immediate impressions are:

1. I'm not sure I'm completely comfortable with the way that lock is being applied - "does the context object have a window attribute?" feels like something that could backfire. It also means that certain classes of handlers (e.g., handlers from menu commands) won't apply the lock. I wonder if it might be better to hold the lock on the app (i.e., something global), with some sort of reporting mechanism that lets the app know which windows need to be refreshed?
2. This will prevent multiple calls to refresh on any single widget (which is a definite improvement); but there will also be potential for optimising the downstream chain of refresh. Refreshing a widget implicitly refreshes the container, which is also an operation that can be optimised AFAICT.
3. There's likely a platform-backend angle to this that should be considered. Most backends have a "suspend painting" option that can be used specifically for this sort of redraw optimisation; if we're suspending updates of Toga's layout and hinting, we should probably also be suspending painting operations.
4. The one notable functionality issue I can see is the interaction with async handlers. If an async handler awaits internally, we definitely need to release the lock - otherwise, a long-lived async handler will permanently lock up redraws.

Of these, (2) and (3) are things I could probably live with as "future enhancements"; but (1) and especially (4) need to be resolved before we could move forward with this approach.

[cvwillegen]

Thanks for the suggestion/patch. I definitely agree that the "refresh flood" is an issue that we need to address.

Thanks for looking into it!

1. I'm not sure I'm _completely_ comfortable with the way that lock is being applied - "does the context object have a window attribute?" feels like something that could backfire. It also means that certain classes of handlers (e.g., handlers from menu commands) _won't_ apply the lock. I wonder if it might be better to hold the lock on the _app_ (i.e., something global), with some sort of reporting mechanism that lets the app know which windows need to be refreshed?

I tried 'lifting' to lock to the App level, but I failed miserably. It's probably because:
a) I suck at programming Python (seriously! I started about a year ago because I set myself the goal of only doing new (side) projects in Python, and I needed something to run on Android, and here I am...), and
b) I have no experience with the code base and its structure

I would love to have some help here...

2. This will prevent multiple calls to refresh on any single widget (which is a definite improvement); but there will also be potential for optimising the downstream chain of refresh. Refreshing a widget implicitly refreshes the container, which is also an operation that can be optimised AFAICT.

When the refresh() calls are locked, the containers will also only get refreshed once (when the lock is lifted), and not for all widgets within them. Oh, thinking about this, I'm not sure that that's fully true, the containers probably get updated once for every widget within them, but not for every widget within them while they're being added as well...

3. There's likely a platform-backend angle to this that should be considered. Most backends have a "suspend painting" option that can be used specifically for this sort of redraw optimisation; if we're suspending updates of Toga's layout and hinting, we should probably also be suspending painting operations.

I wonder if repaints are triggered when the objects are not being re-layouted, but I haven't researched that yet. Like you said: That could be saved for later.

4. The one notable functionality issue I can see is the interaction with async handlers. If an async handler `awaits` internally, we _definitely_ need to release the lock - otherwise, a long-lived async handler will permanently lock up redraws.

Yes. In my original PR #2438 you indicated that having to specify that a handler wants to have refresh locked until it's done adding widgets (by explicitly using a with window.refresh_lock(): around heavy code) was sub-optimal (and I agree) and it should be handled automatically, but this point you're raising is fair (and I agree here, as well!).

So, could a decorator(?) or a context manager(?) be used to indicate that a handler does not want to have a refresh lock present during its implementation? This burdens the programmer...

something like:

def async_handler(self, widget):
  with self._window.unlock_refresh():
    await long_func()

Thoughts? It almost looks as if 'let's do it automatically, without a burden on the programmer, but not always' is hard to get right :-)

Christ van Willegen

[freakboy3742]

I tried 'lifting' to lock to the App level, but I failed miserably. It's probably because: a) I suck at programming Python (seriously! I started about a year ago because I set myself the goal of only doing new (side) projects in Python, and I needed something to run on Android, and here I am...), and b) I have no experience with the code base and its structure

I would love to have some help here...

I think this isn't an issue any more if the locking API is a public window API (see point 4).

2. This will prevent multiple calls to refresh on any single widget (which is a definite improvement); but there will also be potential for optimising the downstream chain of refresh. Refreshing a widget implicitly refreshes the container, which is also an operation that can be optimised AFAICT.

When the refresh() calls are locked, the containers will also only get refreshed once (when the lock is lifted), and not for all widgets within them. Oh, thinking about this, I'm not sure that that's fully true, the containers probably get updated once for every widget within them, but not for every widget within them while they're being added as well...

That's the call I was checking - AFAICT, the "refresh parent container" is performed on every widget refresh; while any given child widget will only be refreshed once, the parent will be refreshed once for every child.

3. There's likely a platform-backend angle to this that should be considered. Most backends have a "suspend painting" option that can be used specifically for this sort of redraw optimisation; if we're suspending updates of Toga's layout and hinting, we should probably also be suspending painting operations.

I wonder if repaints are triggered when the objects are not being re-layouted, but I haven't researched that yet. Like you said: That could be saved for later.

It's backend dependent. IIRC, Winforms is the only platform that currently does "live" updates - this is usually reported as a bug when a user has a Windows app with a blocking, synchronous loop:

for i in range(0, 10):
    label.text = f"Loop {I}"
    time.sleep(1)

On Windows, this will update every second; on every other platform, it only draws the final "Loop 9" version, because control is never yielded to the event loop, so the paint operation isn't performed.

Agreed we can wait on this until later; I mostly want to make sure that it's clear where such an interface would exist (i.e., that we aren't painting ourself into an architectural corner).

4. The one notable functionality issue I can see is the interaction with async handlers. If an async handler awaits internally, we definitely need to release the lock - otherwise, a long-lived async handler will permanently lock up redraws.

So, could a decorator(?) or a context manager(?) be used to indicate that a handler does not want to have a refresh lock present during its implementation? This burdens the programmer...

True - but an explicit lock that works is better than an implicit lock that doesn't :-) Plus, we can treat this as a v1 implementation - if, at some point in the future, someone works out how to make the locking completely implicit, we can make the explicit lock a no-op, and mark it as deprecated.

Of the two options you present, I'd be inclined to use a context manager - a decorator would ultimately be subject to the same problems as the implicit handler approach, because there's no way to "unlock" around an await call.

[cvwillegen]

For reference: Showing a list of (over 400) Podcast episodes in my Podcast Player (no, you don't want to see the source ;-) I told you I suck a programming Python) without a window refresh lock takes about 45 seconds. With it, the runtime goes to 5 seconds.

[cvwillegen]

(I guess the test failure is unrelated...?)

[mhsmith]

if, at some point in the future, someone works out how to make the locking completely implicit, we can make the explicit lock a no-op, and mark it as deprecated

#2938 already suggests a way to make this implicit – instead of thinking in terms of a lock, just defer any O(n) work until the next main loop iteration. The simplest solution I can think of is:

• Add the work to a list instead of doing it immediately.
• Use loop.call_soon to arrange for a function to be called once (and only once) per main loop iteration, which goes through the list and does the work.

[freakboy3742]

(I guess the test failure is unrelated...?)

Yes - that one is a transient error we see occasionally; I've restarted the test.

[freakboy3742]

This looks like a good implementation of the manual lock approach; a couple of fairly minor cosmetic things, but it otherwise pretty good.

The only other hesitation I have at this point is the point @mhsmith made - that this is actually not that far from a true "automatic" solution.

If instead of a locked boolean, make it an attribute that is either None or an async Task. when a refresh call is made, it checks for the existence of a Task, and if it doesn't exist, creates it. All refresh calls then mark the object as dirty rather than processing; and the task becomes a co-routine that does the current refresh implementation. That should (AFAICT) ensure that any changes are batched and implemented on the main event loop.

[freakboy3742]

This flag definitely shouldn't be writable. A user shouldn't be able to inadvertently change the locking state. A getter makes sense; but I can't see a use case for releasing the lock being a public API (outside of the context manager)

[freakboy3742]

I'd prefer to see this in the form of an assert_action_count (or better still, an assert_ action_performed(count=N)).

[freakboy3742]

Why all these updates to remove punctuation?