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

fix: don't double-load chd files for chd-based items #109

Merged
merged 2 commits into from
Oct 1, 2023

Conversation

aschmitz
Copy link
Contributor

@aschmitz aschmitz commented Oct 1, 2023

If emulator_ext is chd, we pull .chd files for the game files and then again as drive files. (For example, this happens on psx items, such as https://archive.org/details/net_yaroze_2019.)

It appears as though the drive files aren't necessary in that case. This change prevents us from pulling the .chd files twice, speeding up the loading.

It's possible there is a better way to identify which files are game files versus which are drive files, possibly via an optional drive_ext meta setting as identified in the existing comment, but this seems to work in practice for a couple test psx items. Happy to rewrite if there are other preferences.

@db48x
Copy link
Owner

db48x commented Oct 1, 2023

I suppose that would work, though I prefer not to have that kind of special case. Have you tried simply not setting an emulator_ext metadata property at all?

@aschmitz
Copy link
Contributor Author

aschmitz commented Oct 1, 2023

I haven't dug through the rest of the code to see if this is avoidable, but right now for psx or psu, if the .chd files aren't in the root, it will fail to load. (Whereas it doesn't need to be in the driver's directory.) There doesn't appear to be an existing way to not load the driver files, though.

(I agree, I'm not a huge fan of the special-casing there. However, I don't have a great way to survey the IA items that use this to determine whether they use the drivers without a drive_ext set, or how hard setting one where necessary would be - I was mostly just trying to make a minimal change to fix the issue - happy to have it changed though!)

@db48x
Copy link
Owner

db48x commented Oct 1, 2023

Ah, I see. Well, maybe we should just deduplicate the lists, to avoid loading anything twice.

@aschmitz
Copy link
Contributor Author

aschmitz commented Oct 1, 2023

That could be a fine solution, though I'm not sure whether you'd want to call back for the files separately (and store the duplicates?) or handle symlinks.

@db48x
Copy link
Owner

db48x commented Oct 1, 2023

We would want to do the simplest thing that works. We can ignore symlinks, since it is impossible to create an item on IA containing them. We can simply assume that each unique url identifies a unique file.

@aschmitz
Copy link
Contributor Author

aschmitz commented Oct 1, 2023

Yeah, I more meant in the emulated filesystem, but it looks like BrowserFS doesn't have a way to handle one file's data in multiple locations. (Looks like symlink support might come in version 2.0.)

I'm slightly concerned that in some cases that means the memory usage will double (which isn't nothing for 300+ MB items that are at issue here), but maybe that's acceptable if we assume users will have recent computers. (Plausible, as emulating some of these won't be trivial.)

It sounds like your suggestion would be to, around here, have a loop beforehand to fetch files by address, and then another to actually do the mounting?

emularity/loader.js

Lines 1935 to 1947 in 6b318f0

var promises = game_data.files
.map(function (f) {
if (f && f.file) {
if (f.drive) {
return fetch(f.file).then(mountat(f.drive));
} else if (f.mountpoint) {
var path = f.mountpoint[0] != '/' ? '/'+ f.mountpoint : f.mountpoint;
f.file.cached = deltaFS.existsSync(path);
return fetch(f.file).then(saveat(path));
}
}
return null;
});

@db48x
Copy link
Owner

db48x commented Oct 1, 2023

I was just thinking of doing it in get_mame_files.

@db48x
Copy link
Owner

db48x commented Oct 1, 2023

if (!already_fetched_urls.includes(url)) {
  files.push(cfgr.mountFile(modulecfg.driver + '/' + file.name,
                            cfgr.fetchFile(title, url)));
}

@aschmitz
Copy link
Contributor Author

aschmitz commented Oct 1, 2023

Sure, I'll update the PR accordingly.

(There are no good solutions here: if .chd files are typically drivers but we don't know when they're required, we're sort of stuck either assuming we won't need drivers if the game file is a .chd, duplicating them, or resolving the problem with more detailed metadata.)

This prevents us from loading a file as both a driver and a game file,
preventing double-loading .chd files when `meta.emulator_ext` is `chd`.

In these cases, it appears that the files aren't needed as driver files,
so we can safely just load them as game files and ignore the duplicates
we'd get by loading all .chd files as drivers.
loader.js Show resolved Hide resolved
@db48x db48x merged commit 3195735 into db48x:master Oct 1, 2023
@db48x
Copy link
Owner

db48x commented Oct 1, 2023

@textfiles
Copy link
Collaborator

textfiles commented Oct 1, 2023 via email

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.

3 participants