falkoschindler
commented
1 year ago Hi @Kamil-Och! Can you provide some more details or a code example reproducing the problem?
Closed Kamil-Och closed 1 year ago
falkoschindler
commented
1 year ago Hi @Kamil-Och! Can you provide some more details or a code example reproducing the problem?
Kamil-Och
commented
1 year ago Okay here is sample code based on my use case:
falkoschindler
commented
1 year ago I can't reproduce the huge memory consumption you're describing. But apart from that you main block looks strange. Creating the UI should happen before calling ui.run():
if __name__ in {"__main__", "__mp_main__"}:
if __name__ == "__mp_main__":
robot = Robot()
joints = [Joint() for _ in range(6)]
UI()
ui_timer = ui.timer(0.1, lambda: reload_data_and_refresh())
ui.run(title='HMI', show=False)Does the problem still occur?
Kamil-Och
commented
1 year ago yea it doesn't seams to change anything. When I check with firefox process Manager in the span of little over 2 minutes memory for this code goes from 200 mb to 4 gb and then ether crash tab or freeze browser. Considering that you can't reproduce the error could it be the problem with my setup or browser?
falkoschindler
commented
1 year ago Oh! Now I looked at the right place: The memory usage of "Google Chrome Helper (Renderer)" grows indeed about 1 GB per minute.
Here is a more compact reproduction:
import time
from nicegui import ui
@ui.refreshable
def labels():
for _ in range(1000):
ui.label(time.time())
labels()
ui_timer = ui.timer(0.1, labels.refresh)
ui.run()Looks like ui.refreshable introduces a memory leak.
falkoschindler
commented
1 year ago But without ui.refreshable the problem still occurs:
import time
from nicegui import ui
def render():
container.clear()
with container:
for _ in range(1000):
ui.label(time.time())
container = ui.column()
ui_timer = ui.timer(0.1, render)
ui.run()i just tried without the ui.refreshable and it works fine to be honest it shows me around 200 mb of memory
falkoschindler
commented
1 year ago Yes, updating the existing elements is certainly the more efficient way to update the UI. Binding could also help to simplify the code.
But nonetheless, removing and re-creating elements should not leak any memory.
falkoschindler
commented
1 year ago I found at least one memory leak:
window.socket.on("update", (msg) => Object.entries(msg).forEach(([id, el]) => this.elements[el.id] = el));We only add elements to this.elements but never remove them. But how to do that? We either need to find out which elements lost their parents during the update, or we need to send explicit delete messages.
falkoschindler
commented
1 year ago I pushed a fix for the leak from my previous comment https://github.com/zauberzeug/nicegui/issues/1089#issuecomment-1613676633 on the "memory" branch: https://github.com/zauberzeug/nicegui/compare/memory
For 1000 labels 10 times per second, the memory consumption still grows quite at bit. I assume something like the garbage collector having trouble to deal with so many objects.
But with reduced frequency to once per second the problem seems to be (almost?) gone. Even after many minutes the consumption oscillates around 500..600MB. I am, however, not 100% sure that there isn't any growth at all. Do we want to merge and close the issue for now, or should we keep investigating?
By the way: So far I didn't find a way to directly get number of Vue components or the memory profile of the Vue app. This would be a more definitive indicator of a memory leak.
falkoschindler
commented
1 year ago No, we should not merge. Tests are red because moving elements from one container to another does not work anymore. And it looks like the memory consumption is indeed still growing.
CrystalWindSnake
commented
1 year ago @falkoschindler I believe that it is only possible to accurately determine whether a component should be released in the backend.Is it possible to consider using weak references and finalize in Python to solve this problem?
like this?
class Element:
def __del__(self):
print("should be release")
# Notify the frontend that this component should be released.
# self.push_del_message()
@CrystalWindSnake Yes, something like this might work. But I'm not sure if every element should notify the client about its removal independently, or if we better collect such element IDs as part of the update message.
CrystalWindSnake
commented
1 year ago Collecting and scheduling seems to be a better approach. it will also be easy to use different schedulers and make different removal dependency strategies.
falkoschindler
commented
1 year ago New code for experimenting with the client-side element count:
def add():
with card:
ui.label('Some text')
async def count():
ui.notify(await ui.run_javascript('return Object.keys(window.app.elements).length'))
card = ui.card()
ui.button('Add element', on_click=add)
ui.button('Remove element', on_click=card.clear)
ui.button('Count', on_click=count)I think I fixed this issue in https://github.com/zauberzeug/nicegui/commit/be306649ee56af3d28cfa30c721194b4750dfea6:
delete() when clearing or removing an element. Somehow we used to call it only when clearing the client.delete() method adds the element ID to a new delete_queue in outbox.py.delete_queue is processed similarly to the update_queue. Therefore I refactored the code a little bit, especially using an _emit function to send messages to local and on-air clients.None values. This way we don't have to introduce another message type that would require adjustments on the relay server.None elements are deleted from the app.This works well with the example from https://github.com/zauberzeug/nicegui/issues/1089#issuecomment-1609273322 and passes all pytests.
While experimenting with this stress test I noticed a huge performance bottleneck caused by the dependency loading function. It seems like calling the async function for every updated element (even without custom component or libraries) is much more expensive than checking for a component or library outside: https://github.com/zauberzeug/nicegui/commit/db18c9467ff81aacf08ffa0d5104543b6adb541e
With this change I can update 1000 elements almost 10 times per second on my machine. Of course, real-life scenarios should be much more conservative regarding performance and bandwidth.
@rodja Sorry for pushing to the main branch right before a big release. But I think we should include these changes.
rodja
commented
1 year ago Looks good.
Description
Hi, I have an problem that when i inspect the browser the memory for the niceGui website is periodically rising from like 0.5 GB to 5 GB and its causing the website to crash. I don't really know what the problem is but I'm using ui.timer to get the data for the gui on 0.1 seconds timer.