weakrefs and avoiding bad gc cycles

[Andrew Bennetts]

vila wrote:
[…]
> Without presuming how good or bad python is, I think that avoiding
> cycles when it's obvious we're creating one doesn't hurt.

I don't agree.  In the case of the config-lock-remote branch it
complicates the code, which hurts.

So it's a question of: is the tradeoff worth it?  Do the benefits
outweigh the drawbacks?  On the face of it is manually doing something
the VM already does an OK job of doing automatically, so the benefit is
pretty low.  Do you have reason to think otherwise?

>     > I'd like to know how bad people think the second one is.  In that
>     > review, Vincent seems to imply that we should always preemptively
>     > avoid cycles.
> 
> No. I never said nor implied *always*, this implies a huge amount of
> work that I don't intend to even start with and certainly not ask anyone
> else to start either.

Ok, then I'm unsure when you feel we should avoid cycles.  You said
elsewhere “I'd rather not introduce a new [cycle] when I can avoid it”
which sounds fairly unconditional to me.

What criteria do you propose for evaluating if a cycle should be
avoided?  e.g. why avoid the Branch <--> BranchConfig cycle in
particular?

>     > If we have to do something approaching manual memory management in
>     > Python that's pretty disappointing,
> 
> Indeed.
> 
> Without telling my personal story regarding memory management, let's
> just say that I had my share of malloc/free bugs and that I welcomed
> using gc-based languages. The (almost only) warning I got with gc is:
> beware of cycles. So I kept the habit of avoiding them when possible be
> it by design or by avoiding the obvious ones (unfortunately only a few
> are obvious :-/).

But *why* beware of cycles, precisely?

I'd say a better warning is: beware of holding references (whether or
not they are cyclic) longer than you need them, because that prevents
reclaiming the memory being referenced when it is no longer going to be
used.

[…]
>      a weak reference is a reference that does not protect the
>      referenced object from collection by a garbage collector.
> 
> The config can't (and doesn't need to) exist without its branch so the
> branch will always exist when needed.
> 
> End of the story, perfect match for a weakref (thought I....).

I think a perfect match for weakrefs is a cache, as Martin says below,
where it's ok if the weakref is stale, and the lifetime of the object
shouldn't be affected by the existence of the cache.  This case where
it's *not* ok if the weakref is stale is an unusual use of them in my
(limited) experience.

[…]
> In this proposal (and the related ones), I focused on implementing only
> what I needed to make reviews easier. Such discussions tell me that this
> doesn't work. That is far more concerning that using weakrefs or not to
> me.

I don't see how avoiding a cycle fits with “implementing only what I
needed” so I'm not surprised it didn't make reviews easier!  You seem to
be saying that avoiding a new cycle is somehow an essential part of the
configuration patches?  I'm confused about why you're saying that, or
at least seem to be saying that.

> I hope this clarifies my point of view on the issue,
> 
>         Vincent
> 
> [1]: Ok, I *thought* further and looked at where weakref were used in
>      the bzr code base. I came across SmartClientHTTPMedium, asserted
>      Smart == spiv and thought: "I have a previous usage, I'm fine
>      reusing it for the exact same reasons". Case closed, next.

Yes, but as you see with annotate that added by you, not me :)

bzrlib/smart/medium.py does use weakrefs, a) to avoid references (not
cycles specifically), and b) to get notification when an object is
collected.  And it's only used by debugging code.

>      This doesn't mean I'm against discussing it *now* (quite the contrary),
>      just that I wasn't playing with a new toy when I used it, I *really*
>      thought it was just business as usual.

Well, I'm not sure I'd noticed that weakref-for-avoiding-a-cycle before
now, and even if I had I definitely didn't realise this was intended to
be a default policy for our code rather than something specifically
analysed as important to SmartClientHTTPMedium, so it's new to me.

-Andrew.