perf: [3759] Memory fix#5562
Conversation
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | May 1, 2026 10:06p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Thanks, will review asap |
|
First of thank you for taking the time and tinkering with the codebase. It seems like you might have found a good solution on how we can combine the node processes without the limited custom server for standalone nextjs, great 👍🏼 Some thoughts I currently have about this pull request:
|
|
Regarding the idle restart, I don't like it for several reasons:
|
There was a problem hiding this comment.
As mentioned, I think we can revert this
There was a problem hiding this comment.
Yeah i'll take it out, but I think that as an optional feature for people that know what they do or for seriously memory constrained users could be nice.
| import { medias } from "@homarr/db/schema"; | ||
|
|
||
| // Lightweight SVG sanitizer (This replaces DOMPurify!!) | ||
| function sanitizeSvg(svg: string): string { |
There was a problem hiding this comment.
- We should put this into a separate file; we may need to reuse it
- Does this roughly behave the same? We disclosed the vulnerability publicly and a regression in the protection against the exploit would be fatal.
There was a problem hiding this comment.
It does roughly behave the same but on a surface level.
After understanding what dompurify does, which analyzes all the DOM of the svg, it's way more thorough stripping anything bad that an svg could have. I think the small code I wrote could be bypassed with enough persistance... plus the DOMPurify team actively maintains its code and addresses new security vulnerabilities which is nice.
Replacing DOMPurify or making its footprint smaller belongs in another pr, but I'll keep what i wrote in a new file in case we want to reuse it in the future.
| restartTimer = setTimeout(() => { | ||
| restartTimer = null; | ||
| logger.info(`No clients for ${gracePeriodMinutes} minutes - restarting process to free memory`); | ||
| exitProcess(0); | ||
| }, gracePeriodMinutes * 60_000); |
There was a problem hiding this comment.
IMO can be reverted / removed.
| }); | ||
|
|
||
| // Periodically hint V8 to collect garbage that accumulates from cron jobs and | ||
| // request handlers. --expose-gc must be set in NODE_OPTIONS for this to work; |
There was a problem hiding this comment.
Is this flag safe for production?
Also, as a .NET developer, isn't GC supposed to run in background organized by the runtime?
There was a problem hiding this comment.
I've reverted the gc changes too.
--expose-gc is used to enable the GC hint calls which it just nudged the gc to work but it can also ignore it. You know how temperamental gc is
--max-old-space-size=400 can help on constrained hosts by capping heap growth, but 400 MB could be a tad aggressive for a nextjs app and could cause OOM crashes. In my testing I haven't had problems and the app did grow its legs to 800mbs just to go back after some time but without extensive testing I can't vouch for it to work for everyone. That's another option we could expose to an user that knows what's doing,
|
|
||
| // Periodically hint V8 to collect garbage that accumulates from cron jobs and | ||
| // request handlers. --expose-gc must be set in NODE_OPTIONS for this to work; | ||
| // if not available it is silently skipped. |
There was a problem hiding this comment.
Instead of describing what, can you describe why?
Why would it not be available? If the flag mentioned above is not set?
There was a problem hiding this comment.
Very interesting! Thank you for this change :)
There was a problem hiding this comment.
Interesting approach, @Meierschlumpf can you proofread this? I am not deep enough to fully review this code.
There was a problem hiding this comment.
Translations should never be done in a developer change, they are done by the community using Crowdin. You can leave them as is, the community can correct them if they are bad. For next time, you don't have to do this step :)
There was a problem hiding this comment.
I understand. I personally hate crowdin because it takes a crazy amount of time to change each line when I can just use my IDE and change them directly... iirc i tried to help`translate speedtest-tracker once and got frustrated with the tool and just gave up.
It would be great to have the ability to submit translation changes through a PR and have others review them in Crowdin.
Does Crowdin updates what's been changed from github?
There was a problem hiding this comment.
Does this mean now we have duplicate definitions?
There was a problem hiding this comment.
Yeah, but it's intentional. The definition.ts files are server-side stubs so the server can compute default widget option values without dragging in React or UI dependencies. The full definitions in index.tsx are still used client-side as before. It's more files but it keeps the server bundle clean.
Before my changes the server was importing full widget modules (React components, Mantine, Tabler icons, client API etc etc) just to call createOptions(). The definition.ts stubs cut that down to just the schema. It's more of a bundle/startup improvement than a runtime heap reduction though. The big memory improvement comes from consolidating the websocket and tasks processes into Next.js instead of running them separately.
There was a problem hiding this comment.
This change is somewhat unrelated, would be nice to have such changes separate next time :)
622d90c to
07beba5
Compare
|
I also pushed some commits to fix the linting. Can you check those out too? You can run lint and format locally to check. |
|
I'll check out your comments and do the changes as soon as possible! probably between Thursday and Friday. I've been very busy and severely sleep deprived these past few days lol |
|
No pressure - now that we have a solution, we don't have to release it right away. Please take care of yourself; I hope you'll be getting enough sleep again soon. |
|
No worries, please sleep enough. Sleep is more important :) |
…ll the translations
- extract websocket server setup into dedicated module
|
Can you @ me as soon as we can discuss the bounty? You can also DM me on Discord so we can organise the payment (Manicr..) |
# Conflicts: # apps/tasks/package.json # packages/api/src/router/cron-jobs.ts # pnpm-lock.yaml
- conExpressionShcema no longer used - stopJob was async because it used an HTTP client and now it calls JobManager directly so no async needed
Oh btw, done! You have a friend request. :) |


Homarr
Thank you for your contribution. Please ensure that your pull request meets the following pull request:
pnpm build, autofix withpnpm format:fix)devbranchx,y,ior any abbrevation)Continuation of some ramblings on: #3759 (comment)
What I did!
1.- I started by Lazy-loading integration SDKs:
packages/integrations/src/base/creator.tswas importing all 48 integration classes at startup. Even if you only use Proxmox and Pi-hole like in my case, we are loading every single integration, which is crazy.2.- Then I moved tasks and websockets to Nextjs to delete 2 node processes:
Both got merged into Next.js via
instrumentation.ts, which Next.js already calls on startup , so taht one process runs everything.3.- Another thing I changed was to stop the integrations from polling when the page is not being used. I might be overreaching but IMO if the page is closed it should not be working on refreshing the widgets since no one is looking at them. I did this by checking the websocket and setting a 1 minute timer after which the integrations stop working. This will save more cpu than memory but i guess it's nice to have.
4.- I also now force a heap limit and try to call the garbage collection more. It helps but since node seems to not unload libraries once loaded, this isn't helping as much as i'd like.
That's why if I reboot the container the memory sits at 200MB but as long as I open the page for the first time it goes to 340MB or so and sits there.
5.- To address this I first tried doing a small hardcoded test reloading homarr after 5 minutes of inactivity and, when I saw it working, I moved on to adding a setting in the configuration so users can toggle the setting and choose how much inactivity before reloading.
This change REQUIRES that the docker
--restart=unless-stoppedflag is set or it'll just die and never restart.I added translations for all the languages. I used DeepL to translate these where they had the language available and Claude Free for the rest. I can only personally verify the correctness of English, Spanish Catalan, Portuguese and Italian. A German friend verified German and the rest... let's hope for the best.
In all honestly this commit is a bit... idk... maybe you want to take it out. It did work for me in two environments but I can see it leading to issues. That being said, this is the memory footprint after doing a reload:
6.- A change you might not like or has to be tested more is the removal of
isomorphic-dompurifyfrom the SVG upload route. It alone needed an entire jsdom environment just to sanitize uploads which seems excessive. I replaced it with a small regexp that cleans user input.Let me know what you think! I'm really excited about these changes! And I really needed these!! I have about 1Gb free on my proxmox machine. 🫠