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

Question about memory consumption #184

Closed
0xERR0R opened this issue Apr 16, 2020 · 16 comments · Fixed by #197
Closed

Question about memory consumption #184

0xERR0R opened this issue Apr 16, 2020 · 16 comments · Fixed by #197

Comments

@0xERR0R
Copy link
Contributor

0xERR0R commented Apr 16, 2020

I used navidrome for a couple of hours and I'm totaly excited. I observed the memory consumption (my Setup: pi3, navidrome as docker, memory consumption metrics the same as "docker stats").

image

I see memory peaks during index creation -> this is ok. I have also memory peaks while I browse album view via web client -> this is ok. The question is: is the avg memory consumption while music playback and while navidrome is idle of ca. 130 MB ok? Does navidrome use some caches or something similar? Or is it so huge amount because all web client content is embedded in the library? I though, the memory consumption of Go applications, should be < 50 MB.

This is no issue, the application runs well, I'm just trying to understand ;)

@deluan
Copy link
Member

deluan commented Apr 16, 2020

Navidrome itself does not cache anything in memory. The embedded assets should not use heap memory, but it is something worth of investigation.

I think this is caused by SQLite, caching indexes in memory. And we have lots of indexes... In early days, I used LedisDB for persistence, and I remember it never going over 80MB, even after a full rescan.

It also could be that the GC is not doing its job well, due to some reference being hold incorrectly. But if that was the case, we would see the memory going up...

I'll check the embedded assets and see if this can be optimized.

Thanks.

@0xERR0R
Copy link
Contributor Author

0xERR0R commented Apr 18, 2020

I think, the problem is the on-the-fly creation of album covers:
image
I used navidrome's webclient and android app and just browsed in the album view. Web client tries to load album covers simultaneously (by firing a lot of requests parallel) -> this leads in a lot of goroutines, which extracts and converts images.
May be I'm wrong, but I think, it would be better to limit amount of parallel requests on server side.

@deluan
Copy link
Member

deluan commented Apr 18, 2020

Yeah, I also noticed that. That's why I introduced the image cache, so requests for already processed images would come straight from the filesystem, without any image conversion happening. Maybe I have to fine tune it...

Not sure if just throttling the number of requests would help with that. Maybe a better (and more complicated) solution is to have a pool of image extractors, and a image request queue. But I still think the allocated memory for processing the images is not being released immediately. Maybe it is a Go's GC fine tuning....

@0xERR0R
Copy link
Contributor Author

0xERR0R commented Apr 19, 2020

To verify this approach, I just tried to throttle requests to verify with:
r.Use(middleware.ThrottleBacklog(2, 50, time.Minute)) in server.go
I scrolled through all albums and max. memory consumption was 120 MB. But sometimes I saw broken image (due to 503 response from server) and I had to click on reload. I will try to increase the parameter for backlog limit.
I think it is not a bad idea to limit request amount (to avoid "denial of service" etc).
You're right, for image resizing and caching there could be probably a better solution.

@deluan
Copy link
Member

deluan commented Apr 19, 2020

Yeah, adding a request limiter is a good idea. Please keep me posted :)

@0xERR0R
Copy link
Contributor Author

0xERR0R commented Apr 20, 2020

image
With r.Use(middleware.ThrottleBacklog(3, 100, time.Minute)). Scrolled through all albums with 84 pc per page -> no errors or broken images, no big peaks in memory consumption.
The first parameter should correspondent the cpu count, it would be better to calculate this dynamically for example use runtime.NumCPU(), but at least 2 or something like this

@GitSchorsch
Copy link

Hi, I have also installed the patched version from 0xERROR on my Raspberry Pi 4 and after that, i had no more problems with running out of memory/ressources when scrolling through my albums (around 1500, not all with covers). And the scrolling was noticeably faster than before.

Btw.: Thanks for your work, it's really a cool music server.

@deluan
Copy link
Member

deluan commented Apr 20, 2020

@0xERR0R Great! I think the first param should be something like: math.Max(2, runtime.NumCPU()), as you proposed, and the other 2 params should be in the consts.go file.

Can you please wrap this up in a PR? Thanks!

@deluan
Copy link
Member

deluan commented Apr 20, 2020

Hey, I'm reopening this as I just realized that if we limit the number of requests to 2 or 3, we could have a bottleneck when you have more than one user streaming from the server, as the streaming requests take a few seconds or more to complete.

I'll have to make some tests to be sure this is not a problem, but most probably it is. If that is the case, I'll introduce a flag to disable it or double the number of simultaneous requests, as streaming is mostly an I/O operation with lots of waiting. Maybe make this value also configurable.

A more long term solution would be to take the stream/download endpoints off this catch-all throttling and have a separated, configurable throttling just for it.

Also, I didn't see much difference in terms of in my setup (running on Docker AMD64)...

Thoughts?

By the way, are you in our Discord server?

@deluan deluan reopened this Apr 20, 2020
@GitSchorsch
Copy link

I think the setup makes the difference. Without throttling at least my Raspberry Pi4 with an samba mount on a Synology NAS and around 15.000 songs was overloaded. I'm sure, that a more powerful AMD64 with a local music library has no problem to handle the load.
So a configurable throttling would be a good idea.

@0xERR0R
Copy link
Contributor Author

0xERR0R commented Apr 20, 2020

We could also enable throttle middleware only for "getCoverArt" endpoint. I think, this is the only one, which is potential "heavy" and will be called often. What do you think?

@deluan
Copy link
Member

deluan commented Apr 20, 2020

@GitSchorsch It can definitely help with the load, for sure. I should have make myself cleat: What I'm saying is that I don't see difference in memory consumption. My instance (with ~40K songs) still uses the same amount of RAM as before the throttling.

Don't worry, I'm not taking this away, just trying to fine tune its usage :)

@deluan
Copy link
Member

deluan commented Apr 20, 2020

@0xERR0R , I agree that getCoverArt is the big villain here, but if we throttle only it, we lose the ability limit the traffic for other reasons (ex: DoS)....

Having said that, if we don't come up with a better approach, I think this (throttling `getCoverArt only) may be the best way for now

@0xERR0R
Copy link
Contributor Author

0xERR0R commented Apr 21, 2020

Short description about my test setup: pi3 with docker, samba mount with music, many flac files. Raspberry pi3 needs about 1-2 seconds to create one album cover (need to transfer flac with > 10 MB from NAS with max 100Mbit, keep this file in memory, convert on-thy-fly the image (in memory) and write it to cache).
I delete the cache directory and start new container, open web client, go to album view and set row number to 84. Now I can wait until all images are being shown and go to next page. Without throttling, server received 84 requests to extract the album cover -> I have Cpu and memory starvation.
With the last fix (limiting only media requests), I also tried to play music with 8 requests parallel (with curl) while I browse through the album view -> all 8 stream requests could be processed.

@deluan
Copy link
Member

deluan commented Apr 21, 2020

Perfect, reviewing it now. Thanks again

@deluan deluan closed this as completed Apr 21, 2020
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants