jpegview icon indicating copy to clipboard operation
jpegview copied to clipboard

Asynchronous FileList

Open mez0ru opened this issue 1 year ago • 9 comments

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 avatar Mar 19 '23 05:03 mez0ru

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

sylikc avatar Mar 19 '23 06:03 sylikc

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 avatar Mar 19 '23 06:03 sylikc

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 avatar Mar 19 '23 07:03 mez0ru

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.

mez0ru avatar Mar 19 '23 08:03 mez0ru

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...

sylikc avatar Mar 19 '23 08:03 sylikc

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 avatar Mar 19 '23 10:03 mez0ru

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.

mez0ru avatar Mar 19 '23 11:03 mez0ru

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!

sylikc avatar Mar 19 '23 12:03 sylikc

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 avatar Mar 19 '23 16:03 mez0ru