Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stream decoding images #74

Merged

Conversation

CosmicHorrorDev
Copy link
Collaborator

@CosmicHorrorDev CosmicHorrorDev commented Apr 12, 2023

Just opening as a draft since I still have to do more cleanup, but this streams the decoding and compression of images. I'll update this message when I've got it cleaned up It's now cleaned up!

main

main

PR

pr

@trimental
Copy link
Collaborator

Had a look over this, looks great so far. Didn't know the image crate had a guess format function. How have you found the changes to async image loading, are the images still loading worse then they did before for you?

@CosmicHorrorDev
Copy link
Collaborator Author

How have you found the changes to async image loading, are the images still loading worse then they did before for you?

It certainly seems better than the original switch to async, but there's still some blocking on the initial rendering compared to the non-async version

Also note that we seem to be using more idle memory now. In the plot on #69 you can see the idle memory usage is ~19 MiB, but the switch to async bumped it up to ~26 MiB. From some quick glances at memory profiles it looks like the extra memory is from font stuff, but that could be wrong

Currently I'm not sure if the switch to async is carrying its weight. The main advantages for switching to async in general would be:

  1. Better efficiency/performance when doing a lot of waiting-intensive tasks
  2. Better readability/ergonomics when doing a lot of waiting-intensive tasks

Where I think the main issue is that we're not doing a lot of waiting-heavy tasks to begin with. For 1. it's primarily just reading files and performing networks requests which we don't do much of either (Maybe into the 10's of files and network requests). For 2. it seems like we mix CPU-intensive and waiting-intensive tasks enough that it seems to be harder to reason about compared to using threads and threadpools. Currently it's pretty easy for us to accidentally block the executor in a lot of ways that will likely be hard to notice (things like decoding images or calculating layouts)

I'm fine with sticking with the async version and trying to smooth out the bumps that we've hit from just porting, or I would be fine with reverting the async changes and just trying to refactor the existing thread-based code to improve readability and performance (probably using some dedicated threadpools for things and using channels for clearer transfer of data). It's up to you

@trimental
Copy link
Collaborator

Hmm yeah to be honest I agree. I was sort of blinding with thinking that async must be better then just blindly spawning threads for each image, but it seems like each image doesn't block long enough for async to really matter.

I'll probably switch it back to a better threaded approach soon. I think queuing up repositions for when images load in, and executing them when main events are cleared is still something to take back when we revert.

@CosmicHorrorDev
Copy link
Collaborator Author

Sounds good to me! I think experimenting with async brought up some new design ideas at the very least :)

I'll try to finish off this PR this weekend. It just needs some refactoring, logging cleanup, and automated testing

@CosmicHorrorDev
Copy link
Collaborator Author

It should now be ready for review!

It's probably easiest to review the new commits while ignoring the refactor: ... commits since it makes diffing a pain

@CosmicHorrorDev CosmicHorrorDev marked this pull request as ready for review April 23, 2023 20:15
Copy link
Collaborator

@trimental trimental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @CosmicHorrorDev. This looks great to me, really clean code as always. I tried for a while to create a simpler read implementation for Rgba8Adapter that performs equally as well but I couldn't. Thanks for all the time you've spent on image efficiency.

Just looks like there's a merge conflict to resolve and then we can merge this 🥳

@CosmicHorrorDev
Copy link
Collaborator Author

Thanks for the review! I'll deal with the merge conflict tomorrow

@CosmicHorrorDev
Copy link
Collaborator Author

Merge should be clean now

@CosmicHorrorDev CosmicHorrorDev merged commit 99cc33d into Inlyne-Project:main Apr 28, 2023
@CosmicHorrorDev CosmicHorrorDev deleted the stream-decoding-images branch April 6, 2024 04:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants