Patchwork [1,of,2,STABLE] repoview: make propertycache.setcache compatible

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 10, 2013, 11:43 a.m.
Message ID <accf46f194e4f5aee5d4.1378813388@yamac-wifi.lan>
Download mbox | patch
Permalink /patch/2418/
State Deferred
Headers show

Comments

Pierre-Yves David - Sept. 10, 2013, 11:43 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1378806515 -7200
#      Tue Sep 10 11:48:35 2013 +0200
# Branch stable
# Node ID accf46f194e4f5aee5d48c5c910ad169dd419458
# Parent  fd4f612f7cb6413940d4cf2052334cd23f60e042
repoview: make propertycache.setcache compatible

Propertycache used standard attribute assignment. In the repoview case, this
assignment was forwarded to the unfiltered repo. This result in:
(1) unfiltered repo got a potentially wrong cache value,
(2) repoview never reused the cached value.

This patch replaces the standard attribute assignment by an assignment to
`objc.__dict__` which will bypass the `repoview.__setattr__`. This will not
affects other `propertycache` users and it is actually closer to the semantic we
need.
David Soria Parra - Sept. 10, 2013, 2:31 p.m.
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -277,11 +277,12 @@ class propertycache(object):
>          result = self.func(obj)
>          self.cachevalue(obj, result)
>          return result
>  
>      def cachevalue(self, obj, value):
> -        setattr(obj, self.name, value)
> +        # __dict__ assigment required to bypass __setattr__ (eg: repoview)
> +        obj.__dict__[self.name] = value

I am not sure if this is the right way to do it. It will work at the
moment and has the desired effect as we bypass __setattr__ magic. But n
case some extension will use propertycache + some __set__ magic (e.g.
redirecting it) we will end up with the same problem again. I think a
more future proof version would be to store the value in the
propertycache itself and keep propertycache objects.

- David
Idan Kamara - Sept. 10, 2013, 8:06 p.m.
On Tue, Sep 10, 2013 at 2:43 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1378806515 -7200
> #      Tue Sep 10 11:48:35 2013 +0200
> # Branch stable
> # Node ID accf46f194e4f5aee5d48c5c910ad169dd419458
> # Parent  fd4f612f7cb6413940d4cf2052334cd23f60e042
> repoview: make propertycache.setcache compatible
>

This whole cached+proxy attribute hell desperately needs a bunch of unit
tests.


>
> Propertycache used standard attribute assignment. In the repoview case,
> this
> assignment was forwarded to the unfiltered repo. This result in:
> (1) unfiltered repo got a potentially wrong cache value,
> (2) repoview never reused the cached value.
>
> This patch replaces the standard attribute assignment by an assignment to
> `objc.__dict__` which will bypass the `repoview.__setattr__`. This will not
> affects other `propertycache` users and it is actually closer to the
> semantic we
> need.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -277,11 +277,12 @@ class propertycache(object):
>          result = self.func(obj)
>          self.cachevalue(obj, result)
>          return result
>
>      def cachevalue(self, obj, value):
> -        setattr(obj, self.name, value)
> +        # __dict__ assigment required to bypass __setattr__ (eg: repoview)
> +        obj.__dict__[self.name] = value
>
>  def pipefilter(s, cmd):
>      '''filter string S through command CMD, returning its output'''
>      p = subprocess.Popen(cmd, shell=True, close_fds=closefds,
>                           stdin=subprocess.PIPE, stdout=subprocess.PIPE)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Sept. 11, 2013, 12:22 a.m.
On 10 sept. 2013, at 16:31, David Soria Parra wrote:

> 
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -277,11 +277,12 @@ class propertycache(object):
>>         result = self.func(obj)
>>         self.cachevalue(obj, result)
>>         return result
>> 
>>     def cachevalue(self, obj, value):
>> -        setattr(obj, self.name, value)
>> +        # __dict__ assigment required to bypass __setattr__ (eg: repoview)
>> +        obj.__dict__[self.name] = value
> 
> I am not sure if this is the right way to do it. It will work at the
> moment and has the desired effect as we bypass __setattr__ magic. But n
> case some extension will use propertycache + some __set__ magic (e.g.
> redirecting it) we will end up with the same problem again. I think a
> more future proof version would be to store the value in the
> propertycache itself and keep propertycache objects.

I disagree with the __set__ approach here.

Storing the value in the property itself will have a significant performance cost. Simple attribute lookup is much faster that class lookup + descriptor detection + function call. (50 usages in core, some of them performance critical)

Some codes in core and extensions relies on the current behavior and explicitly check in __dict__ (or vars) (about 20 usages in core)

We are patching the "cachevalue" method of propertycache. This function should be the one overwritten by extension altering the way the propertycache store its value. (no bundled extension break with the change)

Our propertycache is just yet another instance of a common python idioms and the __dict__ version is often used[1].

[1] http://www.toofishes.net/blog/python-cached-property-decorator/

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -277,11 +277,12 @@  class propertycache(object):
         result = self.func(obj)
         self.cachevalue(obj, result)
         return result
 
     def cachevalue(self, obj, value):
-        setattr(obj, self.name, value)
+        # __dict__ assigment required to bypass __setattr__ (eg: repoview)
+        obj.__dict__[self.name] = value
 
 def pipefilter(s, cmd):
     '''filter string S through command CMD, returning its output'''
     p = subprocess.Popen(cmd, shell=True, close_fds=closefds,
                          stdin=subprocess.PIPE, stdout=subprocess.PIPE)