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

Asynchronous FileList #172

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

Asynchronous FileList #172

wants to merge 12 commits into from

Conversation

mez0ru
Copy link

@mez0ru mez0ru commented Mar 19, 2023

The original behavior of the program is to check all files synchronously upon startup.
which in my opinion is a bad practice. It should however check asynchronously while it's viewing the startup image for quicker startup.
Rationale: I have thousands of images in one folder, and opening each image individually takes about 1-3 seconds to load.

To be honest, I know my own solution is terribly bad, I'm not good at C++. However, it works great for me. I think it could help make the project better, if my solution is not acceptable then hopefully a better solution could implemented for this. Thanks.

mez0ru added 2 commits March 19, 2023 12:53
The original behavior of the program is to check all files synchronously upon startup, which in my opinion is a bad practice.
It should however check asynchronously while it's viewing the startup image for quicker startup.
Rationale: I have thousands of images in one folder, and opening each image individually takes about 1-3 seconds to load.
My own solution is immature, but it works.
@sylikc
Copy link
Owner

sylikc commented Mar 19, 2023

I can look into the PR and see if there's any particular impacts of using async load for the file list. What if the file list is not ready and we try to "jump" to the 1000th file? (there's a pending "jump to" feature that is on the TODO list...

What happens if you load images in between?

I'm going to mark a few files for removal, as the changes you checked in are not needed... if you can remove those first it'd make it easier for me. Thanks

src/UpgradeLog.htm Outdated Show resolved Hide resolved
@sylikc
Copy link
Owner

sylikc commented Mar 19, 2023

you can run with the upgraded files (I'm assuming it's VS2022) but just don't check it in.

Also, the WTL should be found automatically if you clone the submodule in the deps folder.

@sylikc sylikc added the enhancement New feature or request label Mar 19, 2023
@mez0ru
Copy link
Author

mez0ru commented Mar 19, 2023

Thanks, I removed the unnecessary files.
As for the behavior, it's just a copy paste of (Is not done) ? => Wait until async finishes. It's like thread.join. This is only called if the user requested a read from m_pFileList and the async is still processing. I'm sure there are edge cases, that's why I think it could be improved if done better. However even though my solution is not great. I can move to the next/previous image normally without any error, it just have to execute isNotDone -> Wait() . I included these checks whenever m_pFileList is called from KeyDown method and RunCommand.

Unfortunately, I had to overload 3 other methods that need to read from m_pFileList, those are CJPEGProvider::RequestImage, CMainDlg::AfterNewImageLoaded, CMainDlg::UpdateWindowTitle. Because otherwise, it will not work without heavy changes to the main class.

@mez0ru
Copy link
Author

mez0ru commented Mar 19, 2023

Note that it's not run on demand. The user only has to wait for the async if it's not yet finished; if it's already finished, the program will work as normal (meaning it should be transparent). If the user changes the image before the async finishes, block the main thread until it finishes.

@sylikc
Copy link
Owner

sylikc commented Mar 19, 2023

Thanks for the explainer... give me a few days to create some test cases with thousands of files... how many thousand and what sized files before you notice a slow down? On a local drive or on the network?..

Does the feature for Reload file list when files changed still work in this async setup?

I have a folder with 11k files and I don't notice any lag behavior...

@mez0ru
Copy link
Author

mez0ru commented Mar 19, 2023

how many thousand and what sized files before you notice a slow down? On a local drive or on the network?..

I have a mechanical hard drive, if the folder only have below 1k, then I certainly don't notice any delay. However that can't be said for a folder I have with about >10k images. Opening an image takes around 1-2 seconds? you can feel it the more you open images individually through the file explorer. If you copy any image from this folder to an empty folder you will notice a huge decrease in delay compared to when the image was inside the big folder.
When I remove CFileList initialization from the Open function, I noticed an instantaneous startup & opening images regardless if the image is inside a >10k folder or an empty one. It's very noticeable. I confirmed it using the debugger, it showed that it takes almost 500ms-1s for CFileList to initialize.

Does the feature for Reload file list when files changed still work in this async setup?

Unfortunately, I didn't take this into account, since the purpose was to view the initial image very quickly when I execute the program through the file explorer or Open Dialog. Reload file list is probably still synchronous.

I have a folder with 11k files and I don't notice any lag behavior...

You could try my code and see if there's an improvement in the startup delay, or if not, you can try using the debugger and go line by line to see performance hit when CFileList gets initialized. In my case I definitely attributed it to the scanning of filelist that was slowing down viewing the initial image. Especially FindFiles function inside CFileList.

@mez0ru
Copy link
Author

mez0ru commented Mar 19, 2023

Now I think about it, it's probably better if async is to be run internally inside CFileList. Adding wait() inside the expensive functions instead of in the main dialog. I think it will be much more robust and less error prone. I will try to do this, if I can make it work I will edit this pull request again.

@sylikc
Copy link
Owner

sylikc commented Mar 19, 2023

Sounds good. I'm going to look at the existing changes to see if there's any side effects. I'm also on mechanical hard drives, though I have a performance hardware RAID setup, so I'll probably test it on a slower computer to see if I can replicate the realtime feeling...

It is likely to be cleaner if the async is wrapped inside the CFileList. main dialog can just query CFileList for status if that's needed, but let's see how the implementation pans out. In any case, thanks in advance for submitting your custom changes!

Removed all instances of async inside MainDlg. It's now only implemented inside CFileList.
Some methods needed to be overloaded like RequestImage inside JPEGProvider, so that it does not rely on fileList (during startup & Open Dialog only).
@mez0ru
Copy link
Author

mez0ru commented Mar 19, 2023

So far I have tested:

  • Navigating right and left.
  • deleting an image.
  • Opening a corrupted image.

Of course tested all of them very quickly on startup to check if async is being waited before executing the operation inside FileList.
All of which seems to be working fine. I hope it's a good implementation this time. I did overuse WaitIfNotReady just in case. Suggestions are welcome though.

mez0ru added 5 commits March 20, 2023 00:51
m_isProcessing is true only when it's inside the same scope as the async function.
src/JPEGView/MainDlg.cpp Outdated Show resolved Hide resolved
sdneon added a commit to sdneon/jpegview that referenced this pull request Jun 11, 2023
* Add cherry-picked _mod_ of [mez0ru's PR](sylikc#172) for [Issue: Slow startup when opening a file in a highly populated folder](sylikc#194). thanks mez0ru
  * Not fully tested
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants