Opened 12 years ago
Closed 12 years ago
#10161 closed Patch - Feature (fixed)
Speed up loading of Watch Recordings screen
Reported by: | 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)
Change History (26)
Changed 12 years ago by
Attachment: | lazystrings_v1.patch added |
---|
Changed 12 years ago by
Attachment: | lazystrings_v2.patch added |
---|
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Attachment: | lazystrings_v4.patch added |
---|
comment:4 Changed 12 years ago by
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 12 years ago by
Attachment: | lazystrings_v5.patch added |
---|
Use templates and interface classes to generalize and clean up the interface.
comment:5 Changed 12 years ago by
Owner: | set to danielk |
---|---|
Status: | new → accepted |
Changed 12 years ago by
Attachment: | lazystrings_v7.patch added |
---|
Changed 12 years ago by
Attachment: | lazystrings_0.24.patch added |
---|
Attempted back-port to 0.24. Compiles, but not tested.
Changed 12 years ago by
Attachment: | lazystrings_v8.patch added |
---|
Greatly simplified, but a bit visually unpleasant when navigating page by page.
Changed 12 years ago by
Attachment: | lazystrings_v10.patch added |
---|
comment:6 Changed 12 years ago by
Milestone: | unknown → 0.25 |
---|---|
Resolution: | → fixed |
Status: | accepted → closed |
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 12 years ago by
Revert "Speed up initial loading of Watch Recordings screen."
This reverts commit 3dbd1bd169e4b30fe100d786a4818f8b14ab74c0.
Branch: master Changeset: 29b66a14d98b7eb5ca1a4145dd02273df3dc95ae
comment:8 Changed 12 years ago by
Milestone: | 0.25 → 0.26 |
---|---|
Resolution: | fixed |
Status: | closed → new |
comment:9 Changed 12 years ago by
Owner: | changed from danielk to Jim Stichnoth |
---|---|
Status: | new → accepted |
Changed 12 years ago by
Attachment: | lazystrings_v11.patch added |
---|
Use the PlaybackBox? helper thread to initialize the button list items in the background.
Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Attachment: | lazystrings_v13a.patch added |
---|
Here's a version of the v13 patch that refactors the event structure into the MythUIButtonList class.
Changed 12 years ago by
Attachment: | lazystrings_0.24_v13a.patch added |
---|
Attempted back-port of v13a patch to 0.24. Compiles, but not tested.
comment:11 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
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.