decorators.Cache Attach an unique id to objects

decorators.Cache Attach an unique id to objects

decorators.Cache keeps track of the function argument by getting their
string representation. But it can happen that two objects have the same
string representation, if they are created in two different functions
(because they might be created in the same memory place).

So, this patch checks the first argument of the function, and if it's
possible to attribute to it an unique id, it does. Then, that unique id
will be used as part of the hash key. Then, if the cached function is
bound to two different objects, it will not consider those objects equal
anymore.

R=tonyg@chromium.org
BUG=

[tonyg, 2014-07-25 21:13:55]

https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
File tools/telemetry/telemetry/decorators.py (right):

https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
tools/telemetry/telemetry/decorators.py:31: self.__CacheUniqueId =
uuid.uuid4().hex
Thanks for digging into this!!

Looking again at this code, I wonder whether the root problem is that the
__cache lives on the function object instead of the instance. In addition to
causing the problem you noticed, it also seems prone to leaking memory.

I wonder whether this (or something similar) works instead:

if args and isinstance(args[0], obj):
  cache = args[0].__cache = {}

This also points out that we should really have a decorators_unittest.py method.

Also, if it turns out that we can't move the cache to the right spot, a better
way to get a unique ID would be id(args[0]).

[a.renevier, 2014-07-25 21:40:28]

On 2014/07/25 21:13:55, tonyg wrote:
>
https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
> File tools/telemetry/telemetry/decorators.py (right):
> 
>
https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
> tools/telemetry/telemetry/decorators.py:31: self.__CacheUniqueId =
> uuid.uuid4().hex
> Thanks for digging into this!!
> 
> Looking again at this code, I wonder whether the root problem is that the
> __cache lives on the function object instead of the instance. In addition to
> causing the problem you noticed, it also seems prone to leaking memory.

I'm not sure how we can avoid that. The decorator (Cache function) is called
with the unbounded function. Also, at some places, the decorator is applied to
function which are not parts of class

> 
> I wonder whether this (or something similar) works instead:
> 
> if args and isinstance(args[0], obj):
>   cache = args[0].__cache = {}

obj is the method, not the objet. So

> This also points out that we should really have a decorators_unittest.py
method.
> 
> Also, if it turns out that we can't move the cache to the right spot, a better
> way to get a unique ID would be id(args[0]).

This also doesn't work. The documentation says that by design, "Two objects with
non-overlapping lifetimes may have the same id() value."
And this is what happened in the situation I encountered. The two objects,
created in two different functions have the same id, and therefore, the same
string representation.
https://docs.python.org/2/library/functions.html#id

[tonyg, 2014-07-27 18:49:16]

On 2014/07/25 21:40:28, a.renevier wrote:
> On 2014/07/25 21:13:55, tonyg wrote:
> >
>
https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
> > File tools/telemetry/telemetry/decorators.py (right):
> > 
> >
>
https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
> > tools/telemetry/telemetry/decorators.py:31: self.__CacheUniqueId =
> > uuid.uuid4().hex
> > Thanks for digging into this!!
> > 
> > Looking again at this code, I wonder whether the root problem is that the
> > __cache lives on the function object instead of the instance. In addition to
> > causing the problem you noticed, it also seems prone to leaking memory.
> 
> I'm not sure how we can avoid that. The decorator (Cache function) is called
> with the unbounded function. Also, at some places, the decorator is applied to
> function which are not parts of class
> 
> > 
> > I wonder whether this (or something similar) works instead:
> > 
> > if args and isinstance(args[0], obj):
> >   cache = args[0].__cache = {}
> 
> obj is the method, not the objet. So
> 
> > This also points out that we should really have a decorators_unittest.py
> method.
> > 
> > Also, if it turns out that we can't move the cache to the right spot, a
better
> > way to get a unique ID would be id(args[0]).
> 
> This also doesn't work. The documentation says that by design, "Two objects
with
> non-overlapping lifetimes may have the same id() value."
> And this is what happened in the situation I encountered. The two objects,
> created in two different functions have the same id, and therefore, the same
> string representation.
> https://docs.python.org/2/library/functions.html#id

Here's something along the lines of what I had in mind:
https://codereview.chromium.org/425613002/

LMK what you think.

[a.renevier, 2014-08-04 23:28:32]

On 2014/07/27 18:49:16, tonyg wrote:
> On 2014/07/25 21:40:28, a.renevier wrote:
> > On 2014/07/25 21:13:55, tonyg wrote:
> > >
> >
>
https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
> > > File tools/telemetry/telemetry/decorators.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/419973004/diff/1/tools/telemetry/telemetry/de...
> > > tools/telemetry/telemetry/decorators.py:31: self.__CacheUniqueId =
> > > uuid.uuid4().hex
> > > Thanks for digging into this!!
> > > 
> > > Looking again at this code, I wonder whether the root problem is that the
> > > __cache lives on the function object instead of the instance. In addition
to
> > > causing the problem you noticed, it also seems prone to leaking memory.
> > 
> > I'm not sure how we can avoid that. The decorator (Cache function) is called
> > with the unbounded function. Also, at some places, the decorator is applied
to
> > function which are not parts of class
> > 
> > > 
> > > I wonder whether this (or something similar) works instead:
> > > 
> > > if args and isinstance(args[0], obj):
> > >   cache = args[0].__cache = {}
> > 
> > obj is the method, not the objet. So
> > 
> > > This also points out that we should really have a decorators_unittest.py
> > method.
> > > 
> > > Also, if it turns out that we can't move the cache to the right spot, a
> better
> > > way to get a unique ID would be id(args[0]).
> > 
> > This also doesn't work. The documentation says that by design, "Two objects
> with
> > non-overlapping lifetimes may have the same id() value."
> > And this is what happened in the situation I encountered. The two objects,
> > created in two different functions have the same id, and therefore, the same
> > string representation.
> > https://docs.python.org/2/library/functions.html#id
> 
> Here's something along the lines of what I had in mind:
> https://codereview.chromium.org/425613002/
> 
> LMK what you think.

This patch seems better than mine. Should I close the issue?