| 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
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.
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.
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; }