Open less files when detecting/opening blobs #4537

Merged
merged 5 commits into from Jan 11, 2017

Projects

None yet

6 participants

@JosJuice
Contributor
JosJuice commented Dec 21, 2016 edited

Remember when Pokémon Snap used to take several minutes to start because we were opening files unnecessarily many times? (PR #2649)

I noticed that the blob code also was opening files unnecessarily many times, so I made this PR that cuts it down to one. I hope it will make game list scanning a little faster, but I don't think the difference will be as dramatic as with Pokémon Snap.

@JMC47 (or someone else?), please test that all file formats still are detected properly. I've only tested ISO and CISO this far. In particular, WBFS disc images that consist of multiple files need to be checked.

@BhaaLseN
Contributor

Code LGTM, but haven't tested it.

@leoetlino
Member

WBFS images that consist of several files seem to be broken.

@JosJuice
Contributor
JosJuice commented Dec 31, 2016 edited

I found and fixed a simple mistake: I wasn't setting new_entry->file in WbfsFileReader::AddFileToList. WBFS files still don't seem to be working, though... There must be some additional bug in the third commit.

@JosJuice
Contributor
JosJuice commented Jan 1, 2017 edited

The other bug was that it didn't seek back to 0 before reading the WBFS header. Now that that's fixed, WBFS seems to be working correctly.

@@ -177,23 +178,24 @@ std::unique_ptr<IBlobReader> CreateBlobReader(const std::string& filename)
if (cdio_is_cdrom(filename))
return DriveReader::Create(filename);
- if (!File::Exists(filename))
+ File::IOFile file(filename, "rb");
+ u32 magic;
@Helios747
Helios747 Jan 1, 2017 Contributor

Would it be too nitpicky to request this variable (and subsequent "magic" vars) be named something more descriptive?

magic_num or something. If you disagree, feel free to ignore. Not terribly important.

@JosJuice
JosJuice Jan 1, 2017 edited Contributor

num isn't especially descriptive, and that it's a u32 already implies that it's a number, so I don't think magic_num is a better name.

@@ -177,23 +178,24 @@ std::unique_ptr<IBlobReader> CreateBlobReader(const std::string& filename)
if (cdio_is_cdrom(filename))
return DriveReader::Create(filename);
- if (!File::Exists(filename))
+ File::IOFile file(filename, "rb");
+ u32 magic;
@iwubcode
iwubcode Jan 2, 2017

This might be an accurate variable name, but I have to ask...why is it called "magic"? Maybe a comment?

@iwubcode
iwubcode Jan 2, 2017

Gah, sorry @JosJuice my comments were pending for like a week, didn't realize I didn't submit them. Now I see @Helios747 had this same question. So what is this magic number doing?

@lioncash
lioncash Jan 2, 2017 Member

magic in this case indicates a set of bytes at the beginning of the file used to identify the file format.

@iwubcode
iwubcode Jan 2, 2017

Thank you @lioncash ! @JosJuice maybe calling it file_format_identifier or file_format_id would be more descriptive? A comment calling this out as a magic number might also be useful? Agree with @Helios747 , not terribly important but just an idea.

@JosJuice
JosJuice Jan 2, 2017 Contributor

I've added a comment here that explains the file format identification in detail. I'd like to keep the word magic in the variable name, though.

Source/Core/DiscIO/WbfsBlob.cpp
- path[path.length() - 1] = '0' + m_total_files;
- }
+ std::string current_path = path;
+ if (m_files.size() >= 10)
@iwubcode
iwubcode Jan 2, 2017

Will m_files ever be greater than 0 when we enter the loop? If not, I'd suggest we move this into the return statement of "AddFileToList"

@JosJuice
JosJuice Jan 2, 2017 Contributor

m_files.size() is always 1 when entering the loop, but moving the check wouldn't be a good idea. The reason I'm checking it here is to avoid getting a strange file name on the next line. AddFileToList shouldn't need to care about the details of how this function generates file names. Also, the IOFile object gets created before calling AddFileToList, and it's cleaner to not attempt to open paths where we expect the file name extension to contain a colon. (Maybe the code would work correctly even with this check completely removed, unless some OS tries to handle colons in a funny way, but I thought it would be better to not rely on that.)

@iwubcode
iwubcode Jan 2, 2017 edited

Ah, I see, I was just thinking of how to optimize the loop, forgot you're opening a file every time. Would it be more performant to check the path for existence first instead of opening the file? If you did that, you could probably remove the if (m_files) completely.

And do you all use asserts? Might make sense to assert that the initial file-size is 1?

@JosJuice
JosJuice Jan 2, 2017 edited Contributor

I don't know if that's faster. Keep in mind that it will waste some unknown amount of time every time there is an additional file. I would prefer to avoid it unless someone really wants to benchmark it.

@lioncash, what do you think of putting an assert there? (The function works as intended for all values of m_files.size(), except 0, which will make it open a .wbf0 file instead of .wbfs. The old version had an extra if statement for using .wbfs when m_total_files was 0.)

Source/Core/DiscIO/CISOBlob.cpp
-static const char CISO_MAGIC[] = "CISO";
-
-CISOFileReader::CISOFileReader(std::FILE* file) : m_file(file)
+CISOFileReader::CISOFileReader(File::IOFile&& file) : m_file(std::move(file))
@lioncash
lioncash Jan 5, 2017 Member

Just recalled this, but IOFile is already a move-only type by default, so you can just take this by value, similar to how unique_ptrs taking ownership in function signatures are taken by value.

@JosJuice
JosJuice Jan 6, 2017 edited Contributor

But I still have to use m_file(std::move(file)) instead of m_file(file), right? Wouldn't it be problematic to use std::move while taking by value if someone makes IOFile copyable in the future?

@lioncash
lioncash Jan 6, 2017 Member

But I still have to use...

Yes

Wouldn't it be problematic to use std::move while taking by value if someone makes IOFile copyable in the future?

No, because an interface that's already built around moves will still work. Just as moving a std::string doesn't have adverse effects when it's no longer needed in a current scope, despite having a copy-constructor.

@JosJuice
JosJuice Jan 6, 2017 Contributor

I forgot that if the file argument is a copy, our std::move will just move the copy, not the original object that the calling code passes. I'll make the change.

@JosJuice
JosJuice Jan 6, 2017 Contributor

Done.

Source/Core/DiscIO/WbfsBlob.h
bool ReadHeader();
File::IOFile& SeekToCluster(u64 offset, u64* available);
bool IsGood() { return m_good; }
struct file_entry
{
+ file_entry(File::IOFile&& file, u64 base_address, u64 size)
@lioncash
lioncash Jan 5, 2017 Member

this will cause variable shadowing warnings on Linux if the constructor params don't have suffixed underscores.

@JosJuice
JosJuice Jan 6, 2017 Contributor

Fixed.

JosJuice added some commits Dec 21, 2016
@JosJuice JosJuice Only open file once when detecting blob type d1ea00e
@JosJuice JosJuice Don't create new IOFiles when creating a blob
...except for WBFS, which is special because
it has the ability to open multiple files.
8d54bbc
@JosJuice JosJuice WbfsBlob: Only open each file once
The first file used to be opened once by
CreateBlobReader and once inside WbfsFileReader.
5c02795
@JosJuice JosJuice WbfsBlob: Remove m_total_files
std::vector already keeps track of this for us.
0363be4
@JosJuice JosJuice WbfsBlob: Don't wrap file_entry in std::unique_ptr
There doesn't seem to be any reason for doing it.
b187326
@lioncash lioncash merged commit 55b82e3 into dolphin-emu:master Jan 11, 2017

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd Build succeeded on builder pr-freebsd
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@JosJuice JosJuice deleted the JosJuice:blob-open-less-files branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment