Value reference counting

Value reference counting

[Daniel Jacobowitz]

This patch, based on an old patch from Vladimir, implements reference
counting for values.  Tom, this is the approach I discussed with you
on IRC: instead of treating the value chain as a normal reference and
using release_value to take references, this separates the value chain
(which is boolean; a value is either on it or not) from references
(which are counted).  So you take a reference with value_incref.
release_value transforms the value chain's reference into a normal
reference.  That's an entirely theoretical operation, by which I mean
release_value doesn't have to do anything special.

Does this look OK?  Tom, will it work for the Python code?

Tested on x86_64-linux, no regressions.

-- 
Daniel Jacobowitz
CodeSourcery

2009-07-17  Daniel Jacobowitz  <dan@codesourcery.com>
	    Vladimir Prus <vladimir@codesourcery.com>

	* value.c (struct value): Add reference_count field.
	(allocate_value_lazy): Initialize reference_count.
	(value_incref): New function.
	(value_free): Check the reference count.
	* value.h (value_incref): New prototype.

---
 gdb/value.c |   27 +++++++++++++++++++++++++++
 gdb/value.h |    2 ++
 2 files changed, 29 insertions(+)

Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2009-07-17 09:52:16.000000000 -0400
+++ src/gdb/value.c	2009-07-17 10:07:10.000000000 -0400
@@ -194,6 +194,11 @@ struct value
   /* Actual contents of the value.  Target byte-order.  NULL or not
      valid if lazy is nonzero.  */
   gdb_byte *contents;
+
+  /* The number of references to this value.  This initially includes
+     one reference from the value chain; if release_value is called,
+     it converts that into a normal reference.  */
+  int reference_count;
 };
 
 /* Prototypes for local functions. */
@@ -259,6 +264,10 @@ allocate_value_lazy (struct type *type)
   val->pointed_to_offset = 0;
   val->modifiable = 1;
   val->initialized = 1;  /* Default to initialized.  */
+
+  /* Values start out on the all_values chain.  */
+  val->reference_count = 1;
+
   return val;
 }
 
@@ -583,11 +592,29 @@ value_mark (void)
   return all_values;
 }
 
+/* Take a reference to VAL.  VAL will not be deallocated until all
+   references are released.  */
+
+void
+value_incref (struct value *val)
+{
+  val->reference_count++;
+}
+
+/* Release a reference to VAL, which was acquired with value_incref.
+   This function is also called to deallocate values from the value
+   chain.  */
+
 void
 value_free (struct value *val)
 {
   if (val)
     {
+      gdb_assert (val->reference_count > 0);
+      val->reference_count--;
+      if (val->reference_count > 0)
+	return;
+
       if (VALUE_LVAL (val) == lval_computed)
 	{
 	  struct lval_funcs *funcs = val->location.computed.funcs;
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2009-07-17 10:05:12.000000000 -0400
+++ src/gdb/value.h	2009-07-17 10:05:17.000000000 -0400
@@ -582,6 +582,8 @@ extern int unop_user_defined_p (enum exp
 
 extern int destructor_name_p (const char *name, const struct type *type);
 
+extern void value_incref (struct value *val);
+
 extern void value_free (struct value *val);
 
 extern void free_all_values (void);

Re: Value reference counting

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> This patch, based on an old patch from Vladimir, implements
Daniel> reference counting for values.  Tom, this is the approach I
Daniel> discussed with you on IRC: instead of treating the value chain
Daniel> as a normal reference and using release_value to take
Daniel> references, this separates the value chain (which is boolean; a
Daniel> value is either on it or not) from references (which are
Daniel> counted).

Daniel> So you take a reference with value_incref.  release_value
Daniel> transforms the value chain's reference into a normal reference.
Daniel> That's an entirely theoretical operation, by which I mean
Daniel> release_value doesn't have to do anything special.

Daniel> Does this look OK?  Tom, will it work for the Python code?

This will work fine for Python.  Also, I think that this model is
clearer than what I did.

It seems to me that at this point, release_value is doing a walk of a
linked list for no particular benefit.  Suppose we deleted release_value
and replaced all calls to it with calls to value_incref?

This might result in some values living slightly longer than they
otherwise would have (they will live until free_all_values, whereas
currently they will be deleted at value_free time, which might or might
not be sooner).

The only thing I could think of is whether this would affect watchpoint
operation, since IIUC the watchpoint code examines all_values.  But, if
this problem exists, it could be worked around by examining the
reference count of values on the chain.

What do you think?

Tom

Re: Value reference counting

[Daniel Jacobowitz]

On Fri, Jul 17, 2009 at 12:53:52PM -0600, Tom Tromey wrote:
> This will work fine for Python.  Also, I think that this model is
> clearer than what I did.
> 
> It seems to me that at this point, release_value is doing a walk of a
> linked list for no particular benefit.  Suppose we deleted release_value
> and replaced all calls to it with calls to value_incref?
> 
> This might result in some values living slightly longer than they
> otherwise would have (they will live until free_all_values, whereas
> currently they will be deleted at value_free time, which might or might
> not be sooner).
> 
> The only thing I could think of is whether this would affect watchpoint
> operation, since IIUC the watchpoint code examines all_values.  But, if
> this problem exists, it could be worked around by examining the
> reference count of values on the chain.
> 
> What do you think?

I had a good reason this would leak values for watchpoints, but now I
can't get it to work out.  Still, it makes me nervous.

free_all_values can have a very long interval.  But it looks like
breakpoint commands always run it, so not unbounded user behavior.
What about breakpoint conditions?  Is anything released and free'd
during a condition check going to linger until we stop?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Value reference counting

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> I had a good reason this would leak values for watchpoints, but now I
Daniel> can't get it to work out.  Still, it makes me nervous.

Daniel> free_all_values can have a very long interval.  But it looks like
Daniel> breakpoint commands always run it, so not unbounded user behavior.
Daniel> What about breakpoint conditions?  Is anything released and free'd
Daniel> during a condition check going to linger until we stop?

Good questions all, which I don't have the answer for immediately.  I
don't think this should block your patch, which looks plainly correct to
me.  Any other change here can easily be done later.

Tom

Re: Value reference counting

[Vladimir Prus]

On Friday 17 July 2009 Daniel Jacobowitz wrote:

>         * value.c (struct value): Add reference_count field.
>         (allocate_value_lazy): Initialize reference_count.
>         (value_incref): New function.
>         (value_free): Check the reference count.
>         * value.h (value_incref): New prototype.
> 
> ---
>  gdb/value.c |   27 +++++++++++++++++++++++++++
>  gdb/value.h |    2 ++
>  2 files changed, 29 insertions(+)
> 
> Index: src/gdb/value.c
> ===================================================================
> --- src.orig/gdb/value.c        2009-07-17 09:52:16.000000000 -0400
> +++ src/gdb/value.c     2009-07-17 10:07:10.000000000 -0400
> @@ -194,6 +194,11 @@ struct value
>    /* Actual contents of the value.  Target byte-order.  NULL or not
>       valid if lazy is nonzero.  */
>    gdb_byte *contents;
> +
> +  /* The number of references to this value.  This initially includes
> +     one reference from the value chain; if release_value is called,
> +     it converts that into a normal reference.  */
> +  int reference_count;
>  };

I do not fully understand this comment, specifically the "it converts 
that into a normal reference". What is "it", what is "that" and what
is "normal reference" and where the convention happens? From your email,
I gather it's intentional that release_value does not have to be changed,
but I don't understand anything else :-( I am sure it's just me, but
a better comment would be good.

- Volodya

Re: Value reference counting

[Thiago Jung Bauermann]

Em Sexta-feira 17 Julho 2009 16:04:18 Daniel Jacobowitz escreveu:
> On Fri, Jul 17, 2009 at 12:53:52PM -0600, Tom Tromey wrote:
> > This will work fine for Python.  Also, I think that this model is
> > clearer than what I did.
> >
> > It seems to me that at this point, release_value is doing a walk of a
> > linked list for no particular benefit.  Suppose we deleted release_value
> > and replaced all calls to it with calls to value_incref?
> >
> > This might result in some values living slightly longer than they
> > otherwise would have (they will live until free_all_values, whereas
> > currently they will be deleted at value_free time, which might or might
> > not be sooner).
> >
> > The only thing I could think of is whether this would affect watchpoint
> > operation, since IIUC the watchpoint code examines all_values.  But, if
> > this problem exists, it could be worked around by examining the
> > reference count of values on the chain.
> >
> > What do you think?
>
> I had a good reason this would leak values for watchpoints, but now I
> can't get it to work out.  Still, it makes me nervous.

Watchpoints hold their own references to values. Other than that, IMHO if 
watchpoint code causes values to be leaked, that is a bug, not a special 
feature which should be respected... WDYT?

> free_all_values can have a very long interval.  But it looks like
> breakpoint commands always run it, so not unbounded user behavior.
> What about breakpoint conditions?  Is anything released and free'd
> during a condition check going to linger until we stop?

I don't like the idea of having the GC being ran at unpredictably long 
intervals. Aren't we aiming at debugging big fat apps with big insane 
debugging sessions afterall?
-- 
[]'s
Thiago Jung Bauermann

Re: Value reference counting

[Daniel Jacobowitz]

On Mon, Jul 20, 2009 at 10:22:19AM -0300, Thiago Jung Bauermann wrote:
> Watchpoints hold their own references to values. Other than that, IMHO if 
> watchpoint code causes values to be leaked, that is a bug, not a special 
> feature which should be respected... WDYT?

Confused by your question :-)

The way free_all_values works is that it's only run after a command.
For instance, a breakpoint command, or a user-typed command.  It's not
run every time we stop the target and do some thinking if the thinking
is not in the form of a command.  So it's possible to get values
created that don't get cleaned up for a while.

Leaked was probably not the right word, just not GC'd promptly.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Value reference counting

[Daniel Jacobowitz]

On Mon, Jul 20, 2009 at 10:13:35AM +0400, Vladimir Prus wrote:
> > +  /* The number of references to this value.  This initially includes
> > +     one reference from the value chain; if release_value is called,
> > +     it converts that into a normal reference.  */
> > +  int reference_count;
> >  };
> 
> I do not fully understand this comment, specifically the "it converts 
> that into a normal reference". What is "it", what is "that" and what
> is "normal reference" and where the convention happens? From your email,
> I gather it's intentional that release_value does not have to be changed,
> but I don't understand anything else :-( I am sure it's just me, but
> a better comment would be good.

"It" is release_value, and "that" is the one reference.  Is this
clearer?

  /* The number of references to this value.  When a value is created,
     the value chain holds a reference, so REFERENCE_COUNT is 1.  If
     release_value is called, this value is removed from the chain but
     the caller of release_value now has a reference to this value.
     The caller must arrange for a call to value_free later.  */


-- 
Daniel Jacobowitz
CodeSourcery

Re: Value reference counting

[Thiago Jung Bauermann]

Em Segunda-feira 20 Julho 2009 10:26:50 Daniel Jacobowitz escreveu:
> On Mon, Jul 20, 2009 at 10:22:19AM -0300, Thiago Jung Bauermann wrote:
> > Watchpoints hold their own references to values. Other than that, IMHO if
> > watchpoint code causes values to be leaked, that is a bug, not a special
> > feature which should be respected... WDYT?
>
> Confused by your question :-)
>
> The way free_all_values works is that it's only run after a command.
> For instance, a breakpoint command, or a user-typed command.  It's not
> run every time we stop the target and do some thinking if the thinking
> is not in the form of a command.  So it's possible to get values
> created that don't get cleaned up for a while.

Right. And that's what I'm complaining about. You're talking about 
intermediate values generated every time the inferior stops and the watchpoint 
expression is evaluated, right? That's what I'm thinking about.

If you're just thinking about the values stored in struct breakpoint, then I 
agree with you...

> Leaked was probably not the right word, just not GC'd promptly.

Yes, and I'm complaining about not GCing for a great, out-of-our-control, 
while...

Am I having a delusion here? :-)
-- 
[]'s
Thiago Jung Bauermann

Re: Value reference counting

[Vladimir Prus]

On Monday 20 July 2009 Daniel Jacobowitz wrote:

> On Mon, Jul 20, 2009 at 10:13:35AM +0400, Vladimir Prus wrote:
> > > +  /* The number of references to this value.  This initially includes
> > > +     one reference from the value chain; if release_value is called,
> > > +     it converts that into a normal reference.  */
> > > +  int reference_count;
> > >  };
> > 
> > I do not fully understand this comment, specifically the "it converts 
> > that into a normal reference". What is "it", what is "that" and what
> > is "normal reference" and where the convention happens? From your email,
> > I gather it's intentional that release_value does not have to be changed,
> > but I don't understand anything else :-( I am sure it's just me, but
> > a better comment would be good.
> 
> "It" is release_value, and "that" is the one reference.  Is this
> clearer?
> 
>   /* The number of references to this value.  When a value is created,
>      the value chain holds a reference, so REFERENCE_COUNT is 1.  If
>      release_value is called, this value is removed from the chain but
>      the caller of release_value now has a reference to this value.
>      The caller must arrange for a call to value_free later.  */

Thank you, this is now crystal clear!

- Volodya

Re: Value reference counting

[Tom Tromey]

>>>>> "Thiago" == Thiago Jung Bauermann <thiago.bauermann@gmail.com> writes:

Daniel> free_all_values can have a very long interval.  But it looks like
Daniel> breakpoint commands always run it, so not unbounded user behavior.
Daniel> What about breakpoint conditions?  Is anything released and free'd
Daniel> during a condition check going to linger until we stop?

Thiago> I don't like the idea of having the GC being ran at unpredictably long 
Thiago> intervals. Aren't we aiming at debugging big fat apps with big insane 
Thiago> debugging sessions afterall?

Running free_all_values at a long interval is potentially a problem.
However, any particular problem here can (most likely) be resolved
locally, using value_mark and value_free_to_mark in the code that causes
too much allocation.

Tom

Re: Value reference counting

[Thiago Jung Bauermann]

Em Segunda-feira 20 Julho 2009 13:45:55 Tom Tromey escreveu:
> >>>>> "Thiago" == Thiago Jung Bauermann <thiago.bauermann@gmail.com>
> >>>>> writes:
>
> Daniel> free_all_values can have a very long interval.  But it looks like
> Daniel> breakpoint commands always run it, so not unbounded user behavior.
> Daniel> What about breakpoint conditions?  Is anything released and free'd
> Daniel> during a condition check going to linger until we stop?
>
> Thiago> I don't like the idea of having the GC being ran at unpredictably
> long Thiago> intervals. Aren't we aiming at debugging big fat apps with big
> insane Thiago> debugging sessions afterall?
>
> Running free_all_values at a long interval is potentially a problem.
> However, any particular problem here can (most likely) be resolved
> locally, using value_mark and value_free_to_mark in the code that causes
> too much allocation.

Ok, now we're all talking about the same issue. :-)

If you guys feel that you can fix any potential performance issues for
Real Bigâ„¢ apps and debugging sessions, then I'm happy.
-- 
[]'s
Thiago Jung Bauermann

Re: Value reference counting

[Daniel Jacobowitz]

On Fri, Jul 17, 2009 at 02:41:53PM -0400, Daniel Jacobowitz wrote:
> This patch, based on an old patch from Vladimir, implements reference
> counting for values.  Tom, this is the approach I discussed with you
> on IRC: instead of treating the value chain as a normal reference and
> using release_value to take references, this separates the value chain
> (which is boolean; a value is either on it or not) from references
> (which are counted).  So you take a reference with value_incref.
> release_value transforms the value chain's reference into a normal
> reference.  That's an entirely theoretical operation, by which I mean
> release_value doesn't have to do anything special.
> 
> Does this look OK?  Tom, will it work for the Python code?
> 
> Tested on x86_64-linux, no regressions.

I've checked this in with the revised comment.  The long-lived memory
usage issues Thiago commented on aren't related particularly to this
patch.

-- 
Daniel Jacobowitz
CodeSourcery