Remove the BuildGraph reference from Target

[stuhood]

There has been a very long lived object graph cycle between BuildGraph and Target: each references the other.

While our ambition with tickets like #4769 is to eventually do away with the BuildGraph entirely, we may want to start executing deprecations that will allow us to remove this reference sooner.

One (relatively straightforward?) strategy would be to deprecate all methods on Target that deal with dependees (telling people to directly consume self.context.build_graph instead), and then convert Target objects directly into a graph of references to one another in order to fill Target.dependencies and other similar methods that walk down the graph.

[baroquebobcat]

That sounds reasonable to me.

[kwlzn]

same

[stuhood]

...it occurred to me just now that this somewhat duplicates #3991, which I closed. Sorry about that @kwlzn . I'd propose to keep this one open, because it covers the first step of #3991, and the rest of the endgame there was less clear.

[stuhood]

Two thoughts here:

1. @illicitonion mentioned that he had seen indication that modern python (possibly python 3?) does support breaking reference cycles that pass through built-in collections.
2. It's possible that Target could use a weakref to point to its build graph, allowing the cycle to be broken that way.

[stuhood]

@patliu85 :

pants/src/python/pants/build_graph/target.py

Line 328 in 5ee0e81

self._build_graph = build_graph

creates a cycle, so the suggestion is to wrap that line in a weakref.

[patliu85]

weakref.ref (self._build_graph = weakref.ref(build_graph)) doesn't work in Target class because there are several other places in this class where self._build_graph is being used to call methods in BuildGraph.

For instance, following code snippet

    return [self._build_graph.get_target(dep_address)
            for dep_address in self._build_graph.dependencies_of(self.address)]

would throw this error:
Exception message: 'weakref' object has no attribute 'dependencies_of'

But weakref.proxy (self._build_graph = weakref.proxy(build_graph)) works and mitigates the memory issue a bit.
Take this command:
for i in 'seq 1 10'; do ./pants --enable-pantsd filter :: ; done
as an example.
Without weakref.proxy, the memory usage increases by 27MB to 29MB in each run consistently, but with weakref.proxy, the memory usage increases by 19MB to 27MB in each run (about 23MB in average). So, it's about 20% improvement.

I was hoping weakref.ref would give a better result than weakref.proxy, so I have tried to solve that 'weakref' object has no attribute ...
issue by using weakref.WeakMethod or dereference weakref object, but those didn't work. will continue the google search.

[stuhood]

@patliu85 : Thanks! Do you still see the cycle being created in the objgraph output from #7647?

[patliu85]

@stuhood: Thanks. It's confirmed by using weakref.proxy the LegacyBuildGraph cycle is gone.
Without weakref.proxy, following line would generate a graph that shows LegacyBuildGraph cycle in each run:

objgraph.show_backrefs(objgraph.by_type('LegacyBuildGraph'), max_depth=10, extra_ignore=[id(locals())])

but with weakref.proxy, that line's output is empty.

The graph of random samples of 500 objects doesn't always include LegacyBuildGraph.
Attached two .dot files (complete_without_weakref.dot.zip & complete_with_weakref.dot.zip)
complete_without_weakref.dot.zip
complete_with_weakref.dot.zip

[stuhood]

While the reference still exists, the cycle no longer does. Resolving.