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.

Sign up for GitHub

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

Jump to bottom

[GameList][Performance] Redundant file accesses to Game Info when scrolling by 1 #753

Open
XenuIsWatching opened this issue May 19, 2021 · 3 comments · May be fixed by #755
Open

[GameList][Performance] Redundant file accesses to Game Info when scrolling by 1 #753

XenuIsWatching opened this issue May 19, 2021 · 3 comments · May be fixed by #755

Comments

@XenuIsWatching
Copy link

XenuIsWatching commented May 19, 2021 •
edited

I noticed significant stuttering on my system when scrolling on my game lists. I put the debugger on it, and found that it was spending most of it's time in VideoGameListView::updateInfoPanel() which makes sense as this is doing file access to the video, marquee, and images to be displayed... but what I found odd was that it was called 3 times for the same game (performing file accesses each time!) and when I only scrolled by 1 (which seems rather unnecessary). It was called once by the input for the up or down button, then it was called twice again for by the stop scrolling.

It calls from the up or down button from here:

EmulationStation/es-app/src/components/TextListComponent.h

Line 283 in c6bbd38

listInput(1);

Which then calls listInput(delta) then calls scroll(), when then calls onCursorChanged() which then calls the updateInfoPanel() later on.

The stop scrolling is from here:

EmulationStation/es-app/src/components/TextListComponent.h

Line 313 in c6bbd38

stopScrolling();

This function is even more strange to me... It calls listInput(0) which will call the onCursorChanged() function calling updateInfoPanel(), but then it performs a direct call to onCursorChanged() right after finishing listInput(0).

VideoGameListView::updateInfoPanel()

EmulationStation/es-app/src/views/gamelist/VideoGameListView.cpp

Line 247 in c6bbd38

void VideoGameListView::updateInfoPanel()

I tried to think to myself.. "How can I fix this?", but as I was reading through the lower portions of the code, but more I just began to see spaghetti 🍝 (or I just need to spend more time understanding it) and wasn't quite sure how to re-architect it in order to fix these redundant files calls without breaking anything else as I believe this code is used with more than just Game Lists.

Anyways, I'm raising this issue just so it gets visibility from those who may have insights
@XenuIsWatching XenuIsWatching changed the title [GameList][Performance] Redunant Calls to updateInfoPanel() when scrolling by 1 [GameList][Performance] Redunant file access to Game Info when scrolling by 1 May 19, 2021
@XenuIsWatching XenuIsWatching changed the title [GameList][Performance] Redunant file access to Game Info when scrolling by 1 [GameList][Performance] Redunant file accesses to Game Info when scrolling by 1 May 19, 2021
@Gemba
Copy link

Gemba commented May 19, 2021

Thanks for the detailed analysis.

From a more general perspective it is expected to have at least two events onCursorChanged() for a short button press (i.e. not press and hold). One for the rising edge (released -> pressed) IList::listinput(1 /* in general some value != 0 */) and
one for the failing edge (pressed -> released) IList::listIpnut(0).

But more than two are too much.

Some educated guesses:

  1. One onCursorChanged(CURSOR_STOPPED) call is surplus to my understanding. Can you check, if you want to, what happens if you remove it from IList::stopScrolling() method? (May introduce regression in SystemView, ImageGridComponent, ComponentList (used e.g. in Scraping from within ES)).

  2. In TextListComponent::input() after the stopScrolling()-call the input() method should be exited/jumped out with return true, to avoid further processing by subseqent GUI Components and possible additional events. I assume ImageGridComponent::input() copy-pasted this from TextListComponent::input(), so it should be added there also if tests are successful.

@XenuIsWatching
Copy link
Author

XenuIsWatching commented May 19, 2021 •
edited

For the rising edge, I would imagine that the ideal case would be for to retain the information in the info panel (another option would be to clear it, but I would lean against this from a ux point of view because its so quick).

For the held state, it clears the info panel which makes sense.

Only for the falling edge, I would expect it to perform the 'expensive' operation of reading the file system for screenshots, marquees, and etc.
But it doesn't really make sense for it to be calling onCursorChanged() twice here.

Currently the onCursorChanged() function just always calls the updateInfoPanel() no matter the CursorState called to it with. There is a check within updateInfoPanel() which checks to see if it is in the "Press and Hold" state and will blank the info panel through the function isScrolling().

  1. IList::stopScrolling() will call listInput(0) update the infopanel (which will blank it), but then after that it will reset all the scrolling values to 0. Then will call onCursorChanged() again, and will update the infopanel with screenshots, videos, etc because all the scrolling values have just been resetted. Removing onCursorChanged() within IList::stopScrolling() will just leave the info panel blank in the Press and Held State. It might make sense to remove the call with in listInput(0) instead.
  2. [Still need to look into]

@XenuIsWatching
Copy link
Author

I have started experimenting on my branch

https://github.com/XenuIsWatching/EmulationStation/tree/optimize_game_list

@XenuIsWatching XenuIsWatching changed the title [GameList][Performance] Redunant file accesses to Game Info when scrolling by 1 [GameList][Performance] Redundant file accesses to Game Info when scrolling by 1 May 20, 2021
@XenuIsWatching XenuIsWatching linked a pull request May 21, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix stuttering when starting to scroll a Game List XenuIsWatching/EmulationStation
2 participants
@Gemba @XenuIsWatching