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

DETECTOR: Modify getFileProperties to cache md5 results #2997

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

@a-yyg
Copy link
Contributor

@a-yyg a-yyg commented May 2, 2021

Define a Singleton that stores the md5 and size of files, used during mass add.
This skips the double computing of md5s during a single mass add, with the cache being cleared in subsequent mass adds.

@a-yyg a-yyg changed the title DETECTOR: Modify getFileProperties to cache md5 results DETECTOR: Move MD5CacheManager definition to advancedDetector.h May 2, 2021
@a-yyg a-yyg changed the title DETECTOR: Move MD5CacheManager definition to advancedDetector.h DETECTOR: Modify getFileProperties to cache md5 results May 2, 2021
Copy link
Member

@sev- sev- left a comment

I added a comment about optimization.

Also, I believe, you need to clear the MD5s before every detection, not just when you're running massAdd.

fileProps.md5.clear();

const char *numbers = "0123456789";
Common::String hashname = allFiles[fname].getPath() + ":";

This comment has been minimized.

@sev-

sev- May 2, 2021
Member

Common::String hashname = Common::String::format("%s:%d", allFiles[fname].getPath().c_str(), _md5Bytes);
@@ -393,6 +394,7 @@ void LauncherDialog::massAddGame() {
MessageDialog alert(_("Do you really want to run the mass game detector? "
"This could potentially add a huge number of games."), _("Yes"), _("No"));
if (alert.runModal() == GUI::kMessageOK && _browser->runModal() > 0) {
MD5Man.clear();

This comment has been minimized.

@sev-

sev- May 2, 2021
Member

You need to run it not only during massAdd, but during the conventional detection too, since it always goes through all the engines.

I would say, the more proper place for this would be somewhere around DetectionResults EngineManager::detectGames(const Common::FSList &fslist)

This comment has been minimized.

@a-yyg

a-yyg May 2, 2021
Author Contributor

Thank you for the suggestions. I've force pushed the suggestion regarding Common::String::format.
As for the second suggestion,
I had put MD5Man.clear() inside of DetectionResults EngineManager::detectGames(const Common::FSList &fslist) before, but noticed that the amount of cache hits decreased when the engine had directoryGlobs defined. This is most likely because the file (located in a subfolder defined in directoryGlobs) is added to allFiles and has their md5 computed, but the cache is still cleared between folder jumps inside void MassAddDialog::handleTickle().
To solve this I thought using the entire path as the hash key would be a good choice since there is (supposedly) no chance of the wrong md5 cache being used that way.
I could be understanding the system incorrectly and this solution could be more detrimental than I thought.

This comment has been minimized.

@a-yyg

a-yyg May 2, 2021
Author Contributor

Force pushed the second suggestion since not clearing the cache may have unwanted side effects. Thank you for your comments.

@a-yyg a-yyg force-pushed the a-yyg:md5cache branch from 65c1d94 to 20b128d May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants