-
Notifications
You must be signed in to change notification settings - Fork 357
perf: change preload to use stack instead of queue #2291
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
Conversation
|
I've been testing this for a few days and I can say that the new stack-based preloading mechanism feels much more responsive when previewing images and jumping from one image directory to another. There is no longer a very long wait time before the first image appears on the screen and that is a very good improvement. But this is not just a performance improvement, as it also fixes the ctrl-r bug that caused some files to get stuck in the loading state. So this PR kills two birds with one stone, which is excellent. However, I found a new bug, which is a variation of the loading bug. I can only reproduce it when previewing images with chafa and sixels, so I think it is limited to just that use case. It is very random and difficult to reproduce. When I scroll down through the images by holding I debugged the issue and found that the blank screen is caused by a race condition because the missing image is loaded properly and it is even printed here: fmt.Fprint(os.Stderr, "\033[?2026h") // Begin synchronized update
fmt.Fprint(os.Stderr, "\0337") // Save cursor position
fmt.Fprint(os.Stderr, b.String()) // Write data
fmt.Fprint(os.Stderr, "\0338") // Restore cursor position
fmt.Fprint(os.Stderr, "\033[?2026l") // End synchronized updateBut right after that, another image comes in and calls if sxs.lastFile != "" && (filePath != sxs.lastFile || *win != sxs.lastWin || sxs.forceClear) {
screen.LockRegion(sxs.lastWin.x, sxs.lastWin.y, sxs.lastWin.w, sxs.lastWin.h, false)
}Then the image calls if reg.loading {
if previewLoading {
st = st.Reverse(true)
win.print(screen, 2, 0, st, "loading...")
}
return
}So that another image is clearly trying to print a loading screen, but in its final steps it realizes that it shouldn't be printing the loading screen. Unfortunately, the damage has already been done, because it has already removed the sixel that was supposed to be printed on the screen. Since you know the code inside out, I'll let you decide how to resolve this race condition. |
Thanks, I think this is a large improvement too. I don't see a reason to prefer the current queue-based implementation, so I will remove the draft status from this PR. As for this new blank sixel issue, I think this is actually separate existing issue. I can reproduce it even with preloading disabled (hopefully we are looking at the same problem here). So what happens is that |
Ok, I just thought this PR caused it because I haven't noticed this bug before. |
Co-authored-by: veltza <106755522+veltza@users.noreply.github.com>
|
Regarding the missing sixel bug, I have now merged #2301, and merged it into this PR branch. |
|
Everything seems to be working great now. I think this is ready to be merged. |
cc #2287
Currently preloading is based on a FIFO approach (really just reading files from the preload channel). However might not be ideal when scrolling through files quickly, since newer files will have to wait until all the previous files have finished loading.
This PR changes the preloading mechanism to use a stack-based approach instead.