search

webp-sh / webp_server_go

Go version of WebP Server. A tool that will serve your JPG/PNG/BMP/SVGs as WebP/AVIF format with compression, on-the-fly.
https://docs.webp.sh
GNU General Public License v3.0
1.83k stars 175 forks source link

Lazy mode - async image convert implementation #226

Closed bugfest closed 1 year ago

bugfest commented 1 year ago

This PR implements "lazy mode", an async image convert implementation using queues and worker pools

Helps to mitigate resources exhaustion as observed in #198 and allows the user to have more control over the number of parallel conversions.

I've updated the README with some aditional considerations to fine tune it. By default will use the same parameters of the default mode but running in the background. When images are requested for the very first time the server will return the original image while it's preparing the optimized version. Subsequent requests once the server has the optimized one available, will get the smallest asset from the local cache.

codecov[bot] commented 1 year ago

Codecov Report

Merging #226 (a36ecda) into master (a7b5992) will increase coverage by 49.22%. The diff coverage is 83.33%.

:exclamation: Current head a36ecda differs from pull request most recent head 6fb938b. Consider uploading reports for the commit 6fb938b to get more accurate results

@@             Coverage Diff             @@
##           master     #226       +/-   ##
===========================================
+ Coverage   31.00%   80.22%   +49.22%     
===========================================
  Files           3        6        +3     
  Lines         200      718      +518     
===========================================
+ Hits           62      576      +514     
+ Misses        132       99       -33     
- Partials        6       43       +37
Impacted Files Coverage Δ
config.go 100.00% <ø> (ø)
helper.go 82.35% <ø> (ø)
encoder.go 68.90% <78.33%> (ø)
webp-server.go 81.25% <88.88%> (ø)

... and 5 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

n0vad3v commented 1 year ago

Nice and thank you @bugfest !

But we're a little bit busy these days so please allow us some more days to take a look on this PR.

BTW, as discussed earlier(on https://github.com/webp-sh/webp_server_go/issues/206#issuecomment-1557218845), @BennyThink is doing code refactor on PR https://github.com/webp-sh/webp_server_go/pull/220, so my personal plan is to review that PR first and then try to review this PR and port to the refactored code.

bugfest commented 1 year ago

BTW, as discussed earlier(on #206 (comment)), @BennyThink is doing code refactor on PR #220, so my personal plan is to review that PR first and then try to review this PR and port to the refactored code.

That's ok. Let me know if you need some help to get it migrated

n0vad3v commented 1 year ago

Hello @bugfest , we've finally finished our refactor yesterday, and since there is a huge change on code structure, this PR has lots of conflicts. I've tried to port some of your code in this PR to branch https://github.com/webp-sh/webp_server_go/compare/port-lazy-mode , maybe we can create a new PR to that branch and finalize this feature?

bugfest commented 1 year ago

Hello @bugfest , we've finally finished our refactor yesterday, and since there is a huge change on code structure, this PR has lots of conflicts.

That's great! Congrats @n0vad3v & @BennyThink for this impressive work 🎉

I've tried to port some of your code in this PR to branch https://github.com/webp-sh/webp_server_go/compare/port-lazy-mode , maybe we can create a new PR to that branch and finalize this feature?

Sure, I'll move this PR to draft and port this functionality into the new structure

BennyThink commented 1 year ago

This feels like a magic. However I have a simple solution, why don't run a background task, find the missing avif files, convert it one by one. AVIF conversion is extremely CPU and memory intensive, run in in background won't affect to much.

bugfest commented 1 year ago

This feels like a magic. However I have a simple solution, why don't run a background task, find the missing avif files, convert it one by one. AVIF conversion is extremely CPU and memory intensive, run in in background won't affect to much.

Hi @BennyThink, you can limit the number of AVIF conversion jobs with the lazy-heavy-jobs parameter. What you describe can be done by setting it to 1

bugfest commented 1 year ago

Hi @BennyThink, @n0vad3v,

This PR implements Lazy Conversion. It allows users to setup minimal RAM/CPU footprint by using configurable priority queues. This mode also reduces response latency as it will return by default the non-optimized image until the optimized one is calculated (this might take some seconds depending on the number of queued images waiting to be converted and the users parameters)

n0vad3v commented 1 year ago

@bugfest Nice! We'll take a look at this PR soon.

(Some off-topic, recently we've enabled AVIF conversion on WebP Cloud, and the way implemented that is a little different, we've shared a blog post at WebP Cloud Services now supports AVIF, I personally think that's more simple in design and less control in the conversion.(It converts images to AVIF one by one, the conversion queue is done by scanning the filesystem for unconverted (to AVIF) images, and we've not seen any IO wait pressure. (But it's just some of my simple thoughts, might be incorrect, discussions are welcomed.

bugfest commented 1 year ago

@bugfest Nice! We'll take a look at this PR soon.

Thanks @n0vad3v

(Some off-topic, recently we've enabled AVIF conversion on WebP Cloud, and the way implemented that is a little different, we've shared a blog post at WebP Cloud Services now supports AVIF, I personally think that's more simple in design and less control in the conversion.(It converts images to AVIF one by one, the conversion queue is done by scanning the filesystem for unconverted (to AVIF) images, and we've not seen any IO wait pressure. (But it's just some of my simple thoughts, might be incorrect, discussions are welcomed.

I agree there're multiple ways to implement this. I wan't aware you have implemented a solution for this already (have you published this implementation in a branch or separate repo?).

Nevertheless, I think my proposal might have some additional benefits:

n0vad3v commented 1 year ago

@bugfest

have you published this implementation in a branch or separate repo

No, it's currently in a private repo, but you can try register a WebP Cloud account here to experience the background AVIF conversion. Docs are at https://docs.webp.se/webp-cloud/.

I think my proposal might have some additional benefits:

Agree with it, your implementation can make WebP Server Go more extensive than WebP Cloud's implementation, currently we use NFS + multiple nodes for background AVIF conversion when scaling, a little bit hacky for separating work load and HTTP server load.

bugfest commented 1 year ago

Hi @n0vad3v, @BennyThink plase let me know if you have plans to merge this one. If you're not interested or think it does not adds much value I'll close it and leave the code in my fork for future reference

n0vad3v commented 1 year ago

@bugfest Sorry for putting this PR not updated for so long, I and @BennyThink are recently mostly occupied by WebP Cloud's related stuff in spare time...My fault, truly sorry!

let me know if you have plans to merge this one

Yes, we do have plan to merge this function, but we may only have time to take a closer look at this PR when we've updated docs and examples regarding your other merged PR: https://github.com/webp-sh/webp_server_go/pull/207, please understand that this takes time.

There are two things you can do to help us out.

  1. Help us optimize docs regarding the multi-path function, our docs repo: https://github.com/webp-sh/docs.webp.sh/tree/master/content
  2. Would you like to join our Discord or Telegram(preferred) group, at https://webp.se/contact/, so you can have a better idea of what we're busying about and we could communicate more directly on these?
bugfest commented 1 year ago

Thanks @n0vad3v, please let me know if you have any work inprogress for that doc piece or I should start from scratch documenting the multibackend part (that'd be also ok).

I understand your other priorities; I also want to avoid updating the PR continously. Let's find when is the best time to do that. I'll ping you thru one of those contact methods (proof: e461499dd44aff2f8ce1be1244d531e2ad79c75de32f7d9012b3fcb9f076703c)

n0vad3v commented 1 year ago

@bugfest

I've updated https://docs.webp.sh/comparisons/ and https://docs.webp.sh/usage/multipath/, but I think there can be added more on MultiPath page(for multiple hosts support part), may be you can help refine this page and add multiple hosts support part? (Just make markdown PR to https://github.com/webp-sh/docs.webp.sh/blob/master/content/usage/multipath.md will just be fine)

Good to see you on Discord❤️!(Though I and @BennyThink are using Telegram more for internal communication) you can join our Telegram group at https://t.me/webp_se, or we can come to you on Matrix you've mentioned on Discord.