Have Styles and RenderStyles keep a weak reference to node

[fancidev]

Background

j0shm4n reported in discord that adding and removing TabPanes repeatedly had memory usage grow until indeterministic time points. After some investigation, this appears to be due to the (added and then removed) TabPane objects being kept alive by reference cycles until they are collected by CPython's generational garbage collector.

To illustrate this, run the following script and press the key sequence a a a a a a a a d d d d d d d d g, which adds eight tabs, removes them all, and then generates a GraphViz file showing (part of) the reference graph of the living TabPane objects. The script makes use of the objgraph package, which may be installed by pip install objgraph.

Demo Script

import gc
import objgraph
import uuid
import weakref

from textual.app import App, ComposeResult
from textual.widgets import Header, Footer, TabbedContent, TabPane


g_panes: weakref.WeakSet[TabPane] = weakref.WeakSet()


class Test(App[None]):
    BINDINGS = [
        ("a", "add_tab", "Add Tab"),
        ("d", "delete_tab", "Close Tab"),
        ("g", "generate_graph", "Generate Object Graph"),
        ("0", "gc0", "GC(0)"),
        ("1", "gc1", "GC(1)"),
        ("2", "gc2", "GC(2)"),
    ]

    def compose(self) -> ComposeResult:
        yield Header()
        yield TabbedContent()
        yield Footer()

    async def action_add_tab(self) -> None:
        tabbed_content = self.query_one(TabbedContent)
        pane_id = f"tab_{uuid.uuid4().hex[:8]}"
        pane = TabPane(pane_id, id=pane_id)
        await tabbed_content.add_pane(pane)
        tabbed_content.active = pane_id
        g_panes.add(pane)

    async def action_delete_tab(self) -> None:
        tabbed_content: TabbedContent = self.query_one(TabbedContent)
        pane_id = tabbed_content.active
        if pane_id:
            await tabbed_content.remove_pane(pane_id)

    def action_gc0(self) -> None:
        gc.collect(0)
        self.notify("GC(0) complete")

    def action_gc1(self) -> None:
        gc.collect(1)
        self.notify("GC(1) complete")

    def action_gc2(self) -> None:
        gc.collect(2)
        self.notify("GC(2) complete")

    def action_generate_graph(self) -> None:
        objgraph.show_backrefs(
            list(g_panes), max_depth=6, filename=f"backref-panes.dot"
        )
        self.notify("Object graph generated")


if __name__ == "__main__":
    Test().run()

The generated GraphViz file may be viewed by e.g. GraphViz Online. It looks like below:

The graph shows that one instance of TabPane is kept alive by FIFOCache, and seven other instances are kept alive by reference cycles to (and from) Styles and RenderStyles members. The reference-cycled objects are collected automatically at certain times or when gc.collect(2) is called to perform a full garbage collection. This may be verified by pressing 2 before g to see that the resulting object graph doesn't contain any "dangling" reference cycles.

Change

While reference cycles are ultimately garbage collected and hence not really memory leaks, they do make the memory footprint larger than necessary if the program adds and removes widgets repeatedly. In addition, the cyclic reference between DOMNode and Styles/RenderStyles makes less obvious the ownership relation between the objects.

This PR makes Styles and RenderStyles object keep a weak (rather than strong) reference to the associated DOMNode. With this change, performing the same steps in the Demo Script generates the following object graph:

Note that there is no more reference cycles involving DOMNode.

Backward compatibility

A side effect of the change is that code that relies on the Styles or RenderStyles object keeping alive the associated node would break. Such code must be updated to keep a strong reference to the node explicitly. An example is DOMNode._component_styles, which held a sole reference to a "virtual node" for each component class. This is fixed by storing the virtual nodes (in DOMNode._component_styles_nodes) instead. No other usage is identified in the test cases.

Checklist

• Docstrings on all new or modified functions / classes
• Updated documentation
• Updated CHANGELOG.md (where appropriate)

[willmcgugan]

A think it would be a better solution to break those reference cycles. Widgets have a well-defined lifetime. Unneeded references can be removed when the widget has finished processing events.

[fancidev]

A think it would be a better solution to break those reference cycles. Widgets have a well-defined lifetime. Unneeded references can be removed when the widget has finished processing events.

Thanks for the feedback Will. By "break those reference cycles" do you mean to set for example Styles.node to None at suitable places?

[willmcgugan]

A think it would be a better solution to break those reference cycles. Widgets have a well-defined lifetime. Unneeded references can be removed when the widget has finished processing events.

Thanks for the feedback Will. By "break those reference cycles" do you mean to set for example Styles.node to None at suitable places?

That would be one way, yes.

[fancidev]

A think it would be a better solution to break those reference cycles. Widgets have a well-defined lifetime. Unneeded references can be removed when the widget has finished processing events.

Is my understanding correct that one lifecycle of a widget starts when it is mounted into the DOM tree and ends when it is removed from the DOM tree? There seems nothing that prevents a widget from being mounted again after it is removed from the DOM tree.

It seems to me a StylesBase can be in one of two logical states: unbound or bound. An unbound StylesBase (whose node member is None) simply stores a bunch of attributes and behaves like "plain old data". A bound StyleBase (whose node member is not None) is associated to a Widget, and modifying the attributes of the StylesBase object causes a refresh of that Widget. In this sense, the StylesBase object behaves like a "proxy project", e.g. instead of writing button.set_forground_color('red') one writes button.styles.foreground_color = 'red'.

By treating a StylesBase object (with non-None node) as being "bound" to that node, a weak reference seems conceptually appropriate to me. Once the node is destroyed, the StyleBase object would automatically become unbound. That's why I changed the node member to weakref in this PR.

---

On the other hand, I also agree that introducing weak references complicates the code and increases maintenance burden in the future. A key advantage of Python (and Java, C#, etc) is to free the developer from having to reason about the lifetime of objects, and let the runtime do it instead. Code constructs (such as weak reference) whose main purpose is to precisely define the lifetime adds complexity that is unrelated to the "feature" of the product.

In the current case, the additional memory footprint caused by the reference cycle seems insignificant, and the user may call gc.collect() regularly if they found this to be a problem. Without proper profiling, it is not obvious that the reference cycle between Widget and StylesBase is a concern in real-world applications. Therefore, for the benefit of keeping the code concise and readable, I'd close this PR.

Thanks for checking!