Opened 8 years ago

Closed 8 years ago

#10161 closed Patch - Feature (fixed)

Speed up loading of Watch Recordings screen

Reported by: Jim Stichnoth <stichnot@…> Owned by: Jim Stichnoth
Priority: minor Milestone: 0.26
Component: MythTV - General Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

The attached patch improves the initial loading time of the Watch Recordings screen (PlaybackBox?), in cases where there are a large number of recordings to display and the frontend has a relatively weak CPU such as an Atom.

This is accomplished through lazy creation of many of the program attribute strings in ProgramInfo::ToMap?(), particularly ones that involve expensive computation like string rendering of dates. ToMap?() creates 68 such attribute strings for each program, the majority of which are unlikely to be used by a given theme.

On my system (an ION frontend, about 2000 recordings, and using the Blue Abstract theme), lazy computation saves about 1 millisecond per program, reducing the total wall-clock load time from about 6 seconds down to about 4 seconds. The patch includes some logging (which should ultimately be removed) to help with performance comparisons.

If developers feel this is a reasonable direction, then I would also advocate converting all other uses of InfoMap? to use the lazy data structure, which would remove the code duplication in the SetTextFromLazyMap?() methods. Also, it would be useful to consolidate the 3 separate typedefs of InfoMap?, and clean up multiple spots where QHash<> is used directly instead of InfoMap?.

Attachments (15)

lazystrings_v1.patch (35.0 KB) - added by Jim Stichnoth <stichnot@…> 8 years ago.
lazystrings_v2.patch (32.9 KB) - added by Jim Stichnoth <stichnot@…> 8 years ago.
lazystrings_v3.patch (35.3 KB) - added by Jim Stichnoth <stichnot@…> 8 years ago.
Modified two places so that the original object is memoized, not a copy. Add some more logging for timing comparison.
lazystrings_v4.patch (17.2 KB) - added by Jim Stichnoth <stichnot@…> 8 years ago.
lazystrings_v5.patch (16.3 KB) - added by Jim Stichnoth <stichnot@…> 8 years ago.
Use templates and interface classes to generalize and clean up the interface.
lazystrings_v7.patch (16.4 KB) - added by Jim Stichnoth 8 years ago.
lazystrings_0.24.patch (12.3 KB) - added by Jim Stichnoth 8 years ago.
Attempted back-port to 0.24. Compiles, but not tested.
lazystrings_v8.patch (5.7 KB) - added by Jim Stichnoth 8 years ago.
Greatly simplified, but a bit visually unpleasant when navigating page by page.
lazystrings_v9.patch (4.2 KB) - added by Jim Stichnoth 8 years ago.
Simpler still.
lazystrings_v10.patch (4.7 KB) - added by Jim Stichnoth 8 years ago.
lazystrings_v11.patch (11.5 KB) - added by Jim Stichnoth 8 years ago.
Use the PlaybackBox? helper thread to initialize the button list items in the background.
lazystrings_v12.patch (10.3 KB) - added by Jim Stichnoth 8 years ago.
Could this be the last version? No reason to use a separate helper thread; use the event loop of the UI thread to do the lazy work.
lazystrings_v13.patch (16.2 KB) - added by Jim Stichnoth 8 years ago.
Simplify the event structure and the background state. Also, apply the same principle to Previously Recorded and other screens in proglist.cpp.
lazystrings_v13a.patch (15.6 KB) - added by Jim Stichnoth 8 years ago.
Here's a version of the v13 patch that refactors the event structure into the MythUIButtonList class.
lazystrings_0.24_v13a.patch (16.6 KB) - added by Jim Stichnoth 8 years ago.
Attempted back-port of v13a patch to 0.24. Compiles, but not tested.

Download all attachments as: .zip

Change History (26)

Changed 8 years ago by Jim Stichnoth <stichnot@…>

Attachment: lazystrings_v1.patch added

Changed 8 years ago by Jim Stichnoth <stichnot@…>

Attachment: lazystrings_v2.patch added

comment:1 Changed 8 years ago by Jim Stichnoth <stichnot@…>

Uploaded v2 patch after realizing the new static ProgramInfo? methods were unnecessary because all their work could be done using public accessor methods. Thanks to the IRC discussion for drawing attention to that point.

Speaking of the IRC discussion, I would like to point out that the larger inefficiency would not be solved by caching the ProgramInfo? records outside of the PBB, which is the direction the discussion was taking. The problem is that all MythUIButtonList objects are created up front, one per recording group entry, using the relatively expensive ProgramInfo::ToMap?() function, and this is where the bulk of the time is going.

comment:2 Changed 8 years ago by danielk

To back you up, the program info's are already cached in the the ProgramInfoCache? class, and the UI is already incrementally updated via PlaybackBox::UpdateUIListItem(). The problem is the initial load is can take several seconds. It is updateRecList() which needs to be made faster.

But only a 33% improvement with the extra complexity doesn't seem worth it.

Get the wall clock time closer to 50 ms and we'll really be making a real difference...

Changed 8 years ago by Jim Stichnoth <stichnot@…>

Attachment: lazystrings_v3.patch added

Modified two places so that the original object is memoized, not a copy. Add some more logging for timing comparison.

comment:3 Changed 8 years ago by Jim Stichnoth <stichnot@…>

This patch only improves the phase that translates cached ProgramInfo? objects into MythUIButtonList objects. It doesn't address the other phases, including loading recordings-ui.xml, loading and caching ProgramInfo? objects, and all the MythUI layout and drawing code.

Looking at it another way, the patch also improves the performance of the "Change Group Filter" menu. On the ION frontend with 2000 recordings, changing the group filter to All Programs improves from 3 seconds (without the patch) to .75 seconds (with the patch), a 75% improvement. Not quite down to 50 ms, but getting there...

Changed 8 years ago by Jim Stichnoth <stichnot@…>

Attachment: lazystrings_v4.patch added

comment:4 Changed 8 years ago by Jim Stichnoth <stichnot@…>

The v4 patch simplifies the implementation and brings the updateRecList() time down to about 15 ms (ION frontend, 2000 recordings). Total wall-clock time for loading the Watch Recordings screen is about 3.5 seconds, and "Change Group Filter" is virtually instantaneous.

This is accomplished by breaking the updateRecList() loop body into a pair of individual functions and registering them with the MythUIButtonListItem object as callbacks to be executed when the m_strings and m_states members are first accessed.

The callback interface could probably be improved, particularly so that ProgramInfo? and PlaybackBox? pointer types aren't explicitly mentioned in MythUI files. I can continue on that track if there is any real interest in this patch.

Changed 8 years ago by Jim Stichnoth <stichnot@…>

Attachment: lazystrings_v5.patch added

Use templates and interface classes to generalize and clean up the interface.

comment:5 Changed 8 years ago by danielk

Owner: set to danielk
Status: newaccepted

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v7.patch added

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_0.24.patch added

Attempted back-port to 0.24. Compiles, but not tested.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v8.patch added

Greatly simplified, but a bit visually unpleasant when navigating page by page.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v9.patch added

Simpler still.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v10.patch added

comment:6 Changed 8 years ago by Github

Milestone: unknown0.25
Resolution: fixed
Status: acceptedclosed

Speed up initial loading of Watch Recordings screen.

Fixes #10161. Defer initialization of string properties and states in the PlaybackBox? button list items until the item receives the itemVisible signal. This can greatly improve responsiveness when the recording group being displayed has 1000+ recordings and the frontend CPU is relatively weak, e.g. an ION-based system. The improvement is particularly noticeable when using the Change Group Filter menu item within the Watch Recordings screen.

Branch: master Changeset: 3dbd1bd169e4b30fe100d786a4818f8b14ab74c0

comment:7 Changed 8 years ago by Github

Revert "Speed up initial loading of Watch Recordings screen."

This reverts commit 3dbd1bd169e4b30fe100d786a4818f8b14ab74c0.

Refs #10161. Fixes #10470.

Branch: master Changeset: 29b66a14d98b7eb5ca1a4145dd02273df3dc95ae

comment:8 Changed 8 years ago by Jim Stichnoth

Milestone: 0.250.26
Resolution: fixed
Status: closednew

comment:9 Changed 8 years ago by Jim Stichnoth

Owner: changed from danielk to Jim Stichnoth
Status: newaccepted

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v11.patch added

Use the PlaybackBox? helper thread to initialize the button list items in the background.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v12.patch added

Could this be the last version? No reason to use a separate helper thread; use the event loop of the UI thread to do the lazy work.

comment:10 Changed 8 years ago by Jim Stichnoth

The v12 patch is optimistically the final structure of this patch. I would appreciate anyone who can try it out for some time before it gets committed.

This approach defers initialization of the button list items until after all items are created. Then it adds an event to initialize a "page" of items. After that event is handled and the page of items is initialized, another event is added for the next page, and so forth until all button list items are initialized.

The page size is set to 20 button list items. A redraw is forced after each page, so a small page size will take a lot of time to go through all items, whereas a large page size will impact UI responsiveness during the background operation. Processing 20 at a time gets through a large set reasonably fast with little or no perceptible sluggishness, and almost certainly keeps ahead of a fast-scrolling user.

If the button list is regenerated while background processing is going on, the original processing is aborted and the new button list is processed in the background. If playback begins during background processing, the processing is aborted and restarted after playback finishes.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v13.patch added

Simplify the event structure and the background state. Also, apply the same principle to Previously Recorded and other screens in proglist.cpp.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_v13a.patch added

Here's a version of the v13 patch that refactors the event structure into the MythUIButtonList class.

Changed 8 years ago by Jim Stichnoth

Attachment: lazystrings_0.24_v13a.patch added

Attempted back-port of v13a patch to 0.24. Compiles, but not tested.

comment:11 Changed 8 years ago by Jim Stichnoth <jstichnoth@…>

Resolution: fixed
Status: acceptedclosed

In 825182e48119190d821c053ac31fb93e8bc2ea19/mythtv:

Use background buttonlist loading to speed up the Watch Recordings screen.

Fixes #10161. MythUIButtonList is enhanced with new methods for
loading and fully initializing button list items in the background,
i.e. several items during each iteration of the Qt event loop. This
can give a faster response time for Watch Recordings, particularly on
weak frontends like ION when there are hundreds or thousands of
recordings. Previously Recorded is also modified to use background
loading, which is even more dramatic as long-running MythTV
installations may have many thousands of Previously Recorded entries.

Note: See TracTickets for help on using tickets.