Patchwork [2,of,3,in,crew] parsers: use Py_INCREF safely

login
register
mail settings
Submitter Bryan O'Sullivan
Date Sept. 16, 2013, 7:27 p.m.
Message ID <b3c8c6f2b5c146c9d346.1379359651@australite.local>
Download mbox | patch
Permalink /patch/2502/
State Accepted
Commit b3c8c6f2b5c146c9d3464058ff40d031a97b371d
Headers show

Comments

Bryan O'Sullivan - Sept. 16, 2013, 7:27 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1379358757 25200
#      Mon Sep 16 12:12:37 2013 -0700
# Node ID b3c8c6f2b5c146c9d3464058ff40d031a97b371d
# Parent  5e25d71a58cc93627f635bae699602dc7bb8afcf
parsers: use Py_INCREF safely
Antoine Pitrou - Sept. 16, 2013, 7:36 p.m.
On Mon, 16 Sep 2013 12:27:31 -0700
Bryan O'Sullivan <bos@serpentine.com> wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bryano@fb.com>
> # Date 1379358757 25200
> #      Mon Sep 16 12:12:37 2013 -0700
> # Node ID b3c8c6f2b5c146c9d3464058ff40d031a97b371d
> # Parent  5e25d71a58cc93627f635bae699602dc7bb8afcf
> parsers: use Py_INCREF safely
> 
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -540,11 +540,12 @@ static PyObject *index_get(indexObject *
>  			      uncomp_len, base_rev, link_rev,
>  			      parent_1, parent_2, c_node_id, 20);
>  
> -	if (entry)
> +	if (entry) {
>  		PyObject_GC_UnTrack(entry);
> +		Py_INCREF(entry);
> +	}

Are you sure about that?
I can't think of a situation where you'd want to incref something after
untracking it (normally PyObject_GC_UnTrack should only be used in a
tp_dealloc function).

Regards

Antoine.
Matt Mackall - Sept. 16, 2013, 9:15 p.m.
On Mon, 2013-09-16 at 21:36 +0200, Antoine Pitrou wrote:
> On Mon, 16 Sep 2013 12:27:31 -0700
> Bryan O'Sullivan <bos@serpentine.com> wrote:
> > # HG changeset patch
> > # User Bryan O'Sullivan <bryano@fb.com>
> > # Date 1379358757 25200
> > #      Mon Sep 16 12:12:37 2013 -0700
> > # Node ID b3c8c6f2b5c146c9d3464058ff40d031a97b371d
> > # Parent  5e25d71a58cc93627f635bae699602dc7bb8afcf
> > parsers: use Py_INCREF safely
> > 
> > diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> > --- a/mercurial/parsers.c
> > +++ b/mercurial/parsers.c
> > @@ -540,11 +540,12 @@ static PyObject *index_get(indexObject *
> >  			      uncomp_len, base_rev, link_rev,
> >  			      parent_1, parent_2, c_node_id, 20);
> >  
> > -	if (entry)
> > +	if (entry) {
> >  		PyObject_GC_UnTrack(entry);
> > +		Py_INCREF(entry);
> > +	}
> 
> Are you sure about that?
> I can't think of a situation where you'd want to incref something after
> untracking it (normally PyObject_GC_UnTrack should only be used in a
> tp_dealloc function).

This patch simply moves the INCREF to a place where we know entry is
non-null, so yes, I think we can be pretty sure. Whether GC_UnTrack also
belongs there is orthogonal to whether we should accept the patch.

We use GC_UnTrack pretty aggressively as we're fairly sensitive to the
performance impact of the GC pointlessly walking over half a million
extra tuples.
Siddharth Agarwal - Sept. 16, 2013, 9:36 p.m.
On 09/16/2013 12:36 PM, Antoine Pitrou wrote:
> Are you sure about that?
> I can't think of a situation where you'd want to incref something after
> untracking it

Python's GC (which is actually just a cycle collector) is different from 
its refcounting implementation. All Python objects have a refcount, but 
only some container objects are tracked by the GC. We call 
PyObject_GC_Untrack because we know that entry cannot take part in a cycle.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -540,11 +540,12 @@  static PyObject *index_get(indexObject *
 			      uncomp_len, base_rev, link_rev,
 			      parent_1, parent_2, c_node_id, 20);
 
-	if (entry)
+	if (entry) {
 		PyObject_GC_UnTrack(entry);
+		Py_INCREF(entry);
+	}
 
 	self->cache[pos] = entry;
-	Py_INCREF(entry);
 
 	return entry;
 }