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

Restructure skipExisting checks to reuse previous values #83

Closed

Conversation

J-Swift
Copy link

@J-Swift J-Swift commented Aug 10, 2024

Another bottleneck : D

For a large db.xml + a sizeable gamelist this got really expensive to keep doing the same checks. Restructured it to save information that can be reused.

This took my local skipExisting check from ~5mins to ~5seconds

@J-Swift
Copy link
Author

J-Swift commented Aug 10, 2024

Also, there is still room to improve it but I can't quite figure out the algorithm.

Right now, we iterate over the queue for every gamelist entry, and we call N removes on the queue for each found game. I think It would be better to do a full iteration of the queue, building a cache of "index N has this filepath", simliar to my previous PR. Then do a full cycle of the gamelist to find all indexes to remove. Then remove those in a tight loop (or in batch? is that possible? I havent done c++ in 20 years and never messed with QT).

@Gemba
Copy link
Owner

Gemba commented Aug 12, 2024

That must be some gamelist!
Your suggested change of this PR saves one if. I tested it but noticed no measurable difference between the "now" and your changes. However, my gamelist for the platform only has just below 700 entries.

Can you provide evidence for your claim (5min --> 5secs)?
Do you compile against Qt6 standard or against Qt5 standard (see manual installation in the readme)?
What hardware do you use, incl. storage interface, is the storage network attached?

@J-Swift
Copy link
Author

J-Swift commented Aug 12, 2024 via email

@Gemba
Copy link
Owner

Gemba commented Aug 14, 2024

Yes I did try the ES frontend to reproduce your issue. I saw also your other minor changes.
And yes, the docker file / update_skyskraper.sh script uses Qt5 standard. This takes the qt6 changes on QList/QVector out of the mix.

Seems it is primary caused by a large quickids.xml, db.xml and resource cache. If you have this context it would help to find the real culprit.

The nested loop remove.at() is another issue -as you spotted- which can be optimized, from M*N/2 (in average) to M+N (worst case).

@J-Swift
Copy link
Author

J-Swift commented Sep 23, 2024

Sorry about the delay, got sidetracked on other projects. I dont have the backup to compare any more

@Gemba
Copy link
Owner

Gemba commented Sep 27, 2024

Thanks for keeping me posted. I will close the PR now. Feel free to reopen, when you have evidence.

@Gemba Gemba closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants