GitHub user potiuk edited a comment on the discussion: Discussion: Preventing COW in LocalExecutor Workers
This is a great investigation @wjddn279 ! It's great to find that my "hypothesis" finds confirmation - the unexpected behaviour of `gc`, actually increasing memory usage in our scenario for forks was largely theorethical, after I learned how the internals of `gc` and reference counting works. Glad my intuition did not fail me on that one. @ashb @kaxil @amoghrajesh are likely those who should be looking at this and we should likely have an agreed approach, and I love the idea of actually documenting memory behaviour somewhere. My points for your discussion points: > 1. Are there side effects of gc.freeze? I think we should be able to use it. The documentation about gc.freeze() recommends this pattern for forking and gc.freeze() is available since Python 3.7, so I don't see why we would not be able to use it. We could also use at other usages of fork() in our code and see if it might help as well with memory usage. Likely DagFileProcessor might benefit a bit from it - though there maybe disabling and freezing gc continuously might have more side effects, I am quite sure however that gc.freeze approach for Task SDK and task execution might actually be very helpful to manage the forked process that actually executes the task. Probably worth testing the effect. https://docs.python.org/3/library/gc.html#gc.freeze > gc.freeze() > Freeze all the objects tracked by the garbage collector; move them to a > permanent generation and ignore them in all the future collections. > > If a process will fork() without exec(), avoiding unnecessary copy-on-write > in child processes will maximize memory sharing and reduce overall memory > usage. This requires both avoiding creation of freed “holes” in memory pages > in the parent process and ensuring that GC collections in child processes > won’t touch the gc_refs counter of long-lived objects originating in the > parent process. To accomplish both, call gc.disable() early in the parent > process, gc.freeze() right before fork(), and gc.enable() early in child > processes. > 2. Should we maintain LocalExecutor's current worker creation method? I agree, that if we manage memory in forks more efficiently with gc.freeze() than with the current lazy-loading - that might be better approach indeed. > 3. Should the Job class be coupled with the executor? Yeah. I remember refactoring it in Airflow 2 and trying to get rid of the Executor class from it, because essentially Job is a database model and keeping executor as a field there never felt right. Back then (1.5 year ago or so) I succeded only partially - but since then a lot of the code for Airlfow 3 have changed in this area, so refactoring it out seems like a good idea. GitHub link: https://github.com/apache/airflow/discussions/58143#discussioncomment-14925853 ---- This is an automatically sent email for [email protected]. To unsubscribe, please send an email to: [email protected]
