Re: [PATCH stable] revset: don't cache abstractsmartset min/max invocations infinitely

[Mads Kiilerich]

On 10/26/2016 02:31 PM, Yuya Nishihara wrote:

On Wed, 26 Oct 2016 14:09:17 +0200, Mads Kiilerich wrote:

Instead, I would perhaps prefer to introduce a 'gettercache' that works
on methods that only have self as parameter (and thus can set a simple
instance method as I do here)  ... or a 'methodcache' that would be like
cachefunc but store the cache on the first 'self' parameter.

That also seems fine. (I'm not a big fan of overwriting methods since we're
likely to create self->self cycle, but that wouldn't be the case here.)



I guess it could be slightly more gc efficient to use 'lambda v=v: v' to 
avoid having a reference to v in the outer scope that also has self ...


/Mads

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH stable] revset: don't cache abstractsmartset min/max invocations infinitely

[Yuya Nishihara]

On Wed, 26 Oct 2016 14:09:17 +0200, Mads Kiilerich wrote:
> On 10/26/2016 01:21 PM, Yuya Nishihara wrote:
> > On Tue, 25 Oct 2016 18:57:26 +0200, Mads Kiilerich wrote:
> >> # HG changeset patch
> >> # User Mads Kiilerich 
> >> # Date 1477414587 -7200
> >> #  Tue Oct 25 18:56:27 2016 +0200
> >> # Branch stable
> >> # Node ID c2fe58cd4235fc6c8cabea882794303d620bec3a
> >> # Parent  76c57e1fe79b0980b377b4f305635dea393d6315
> >> revset: don't cache abstractsmartset min/max invocations infinitely
> >>
> >> There was a "leak", apparently introduced in ab66c1dee405. When running:
> >>
> >>  hg = hglib.open('repo')
> >>  while True:
> >>  hg.log("max(branch('default'))")
> >>
> >> all filteredset instances from branch() would be cached indefinitely by the
> >> @util.cachefunc annotation on the max() implementation.
> > Indeed. We've cached {self: v} pair every time max() was called.
> > Queued this for stable, thanks.
> >
> >> -@util.cachefunc
> >>   def min(self):
> >>   """return the minimum element in the set"""
> >> -if self.fastasc is not None:
> >> -for r in self.fastasc():
> >> -return r
> >> -raise ValueError('arg is an empty sequence')
> >> -return min(self)
> >> -
> >> -@util.cachefunc
> >> +if self.fastasc is None:
> >> +v = min(self)
> >> +else:
> >> +for v in self.fastasc():
> >> +break
> >> +else:
> >> +raise ValueError('arg is an empty sequence')
> >> +self.min = lambda: v
> >> +return v
> > I slightly prefer using propertycache() and wrap it by a function, which
> > is a common pattern seen in context.py, but that's just a nitpicking.
> 
> Exactly what do you mean with wrapping the property by a function? I 
> don't see a clear pattern of that in context.py?

Yes. What I have in mind is parents() -> self._parents for instance.

> Instead, I would perhaps prefer to introduce a 'gettercache' that works 
> on methods that only have self as parameter (and thus can set a simple 
> instance method as I do here)  ... or a 'methodcache' that would be like 
> cachefunc but store the cache on the first 'self' parameter.

That also seems fine. (I'm not a big fan of overwriting methods since we're
likely to create self->self cycle, but that wouldn't be the case here.)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH stable] revset: don't cache abstractsmartset min/max invocations infinitely

[Mads Kiilerich]

On 10/26/2016 01:21 PM, Yuya Nishihara wrote:

On Tue, 25 Oct 2016 18:57:26 +0200, Mads Kiilerich wrote:

# HG changeset patch
# User Mads Kiilerich 
# Date 1477414587 -7200
#  Tue Oct 25 18:56:27 2016 +0200
# Branch stable
# Node ID c2fe58cd4235fc6c8cabea882794303d620bec3a
# Parent  76c57e1fe79b0980b377b4f305635dea393d6315
revset: don't cache abstractsmartset min/max invocations infinitely

There was a "leak", apparently introduced in ab66c1dee405. When running:

 hg = hglib.open('repo')
 while True:
 hg.log("max(branch('default'))")

all filteredset instances from branch() would be cached indefinitely by the
@util.cachefunc annotation on the max() implementation.

Indeed. We've cached {self: v} pair every time max() was called.
Queued this for stable, thanks.


-@util.cachefunc
  def min(self):
  """return the minimum element in the set"""
-if self.fastasc is not None:
-for r in self.fastasc():
-return r
-raise ValueError('arg is an empty sequence')
-return min(self)
-
-@util.cachefunc
+if self.fastasc is None:
+v = min(self)
+else:
+for v in self.fastasc():
+break
+else:
+raise ValueError('arg is an empty sequence')
+self.min = lambda: v
+return v

I slightly prefer using propertycache() and wrap it by a function, which
is a common pattern seen in context.py, but that's just a nitpicking.


Exactly what do you mean with wrapping the property by a function? I 
don't see a clear pattern of that in context.py?


Instead, I would perhaps prefer to introduce a 'gettercache' that works 
on methods that only have self as parameter (and thus can set a simple 
instance method as I do here)  ... or a 'methodcache' that would be like 
cachefunc but store the cache on the first 'self' parameter.


/Mads

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH stable] revset: don't cache abstractsmartset min/max invocations infinitely

[Yuya Nishihara]

On Tue, 25 Oct 2016 18:57:26 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich 
> # Date 1477414587 -7200
> #  Tue Oct 25 18:56:27 2016 +0200
> # Branch stable
> # Node ID c2fe58cd4235fc6c8cabea882794303d620bec3a
> # Parent  76c57e1fe79b0980b377b4f305635dea393d6315
> revset: don't cache abstractsmartset min/max invocations infinitely
> 
> There was a "leak", apparently introduced in ab66c1dee405. When running:
> 
> hg = hglib.open('repo')
> while True:
> hg.log("max(branch('default'))")
> 
> all filteredset instances from branch() would be cached indefinitely by the
> @util.cachefunc annotation on the max() implementation.

Indeed. We've cached {self: v} pair every time max() was called.
Queued this for stable, thanks.

> -@util.cachefunc
>  def min(self):
>  """return the minimum element in the set"""
> -if self.fastasc is not None:
> -for r in self.fastasc():
> -return r
> -raise ValueError('arg is an empty sequence')
> -return min(self)
> -
> -@util.cachefunc
> +if self.fastasc is None:
> +v = min(self)
> +else:
> +for v in self.fastasc():
> +break
> +else:
> +raise ValueError('arg is an empty sequence')
> +self.min = lambda: v
> +return v

I slightly prefer using propertycache() and wrap it by a function, which
is a common pattern seen in context.py, but that's just a nitpicking.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH stable] revset: don't cache abstractsmartset min/max invocations infinitely

[Mads Kiilerich]

# HG changeset patch
# User Mads Kiilerich 
# Date 1477414587 -7200
#  Tue Oct 25 18:56:27 2016 +0200
# Branch stable
# Node ID c2fe58cd4235fc6c8cabea882794303d620bec3a
# Parent  76c57e1fe79b0980b377b4f305635dea393d6315
revset: don't cache abstractsmartset min/max invocations infinitely

There was a "leak", apparently introduced in ab66c1dee405. When running:

hg = hglib.open('repo')
while True:
hg.log("max(branch('default'))")

all filteredset instances from branch() would be cached indefinitely by the
@util.cachefunc annotation on the max() implementation.

util.cachefunc seems dangerous as method decorator and is barely used elsewhere
in the code base. Instead, just open code caching by having the min/max
methods replace themselves with a plain lambda returning the result.

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2924,23 +2924,29 @@ class abstractsmartset(object):
 """True if the set will iterate in topographical order"""
 raise NotImplementedError()
 
-@util.cachefunc
 def min(self):
 """return the minimum element in the set"""
-if self.fastasc is not None:
-for r in self.fastasc():
-return r
-raise ValueError('arg is an empty sequence')
-return min(self)
-
-@util.cachefunc
+if self.fastasc is None:
+v = min(self)
+else:
+for v in self.fastasc():
+break
+else:
+raise ValueError('arg is an empty sequence')
+self.min = lambda: v
+return v
+
 def max(self):
 """return the maximum element in the set"""
-if self.fastdesc is not None:
-for r in self.fastdesc():
-return r
-raise ValueError('arg is an empty sequence')
-return max(self)
+if self.fastdesc is None:
+return max(self)
+else:
+for v in self.fastdesc():
+break
+else:
+raise ValueError('arg is an empty sequence')
+self.max = lambda: v
+return v
 
 def first(self):
 """return the first element in the set (user iteration perspective)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel