Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Switch over to a global hash cache #616

Closed
wants to merge 4 commits into from

Conversation

pganssle
Copy link
Member

This uses an alternate approach to hash caching than the current master or #615. Rather than trying to store the hash cache on the object itself, we create a global hash cache object mapping the id of the object to its hash.

This pretty dramatically simplifies the code. The two biggest downsides:

  1. This introduces weak references and mutable global state into the code. I think I've got a thread-safe implementation, but it's very hard to test for that. Even if we weren't worried about memory leaks, we definitely need the cache lifecycle tied to the object's lifecycle, because otherwise the interpreter might assign another cache_hash=True object the same id() value.

  2. At least the way I've implemented it (and I don't know if there's a much faster way to do it), this method adds what I would consider a lot of overhead, compare the speed for this version of the code (both using Python 3.7.4):

>>> import attr
>>> @attr.s(cache_hash=True, hash=True)
... class C:
...     x = attr.ib()
...
>>> %timeit hash(C(1))
6.2 µs ± 176 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
>>> x = C(1)
>>> %timeit hash(x)
1.3 µs ± 7.58 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

To the speed of the equivalent code using #615

>>> import attr
>>> @attr.s(cache_hash=True, hash=True)
... class C:
...     x = attr.ib()
...
>>> %timeit hash(C(1))
684 ns ± 9.06 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
>>> x = C(1)
>>> %timeit hash(x)
207 ns ± 1.42 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

For a more complicated class (where cache_hash=True might be more valuable), the "create-and-hash" numbers narrow a bit, but there's still a pretty significant difference for the "lookup in hash" case:

>>> import attr
>>> @attr.s(cache_hash=True, hash=True)
... class C:
...     x = attr.ib(default="qbert")
...     y = attr.ib(default="hi")
...     z = attr.ib(default=2**120)
...     a = attr.ib(default=tuple(["a"] * 500))
...     b = attr.ib(default=b"A" * 100000)
...     c = attr.ib(default=200)
...
>>> %timeit hash(C())
8.85 µs ± 72.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
>>> x = C()
>>> %timeit hash(x)
1.24 µs ± 19.1 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

#615 version:

>>> @attr.s(cache_hash=True, hash=True)
... class C:
...     x = attr.ib(default="qbert")
...     y = attr.ib(default="hi")
...     z = attr.ib(default=2**120)
...     a = attr.ib(default=tuple(["a"] * 500))
...     b = attr.ib(default=b"A" * 100000)
...     c = attr.ib(default=200)
...
>>> %timeit hash(C())
3.13 µs ± 156 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
>>> x = C()
>>> %timeit hash(x)
212 ns ± 3.54 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

I have spent less time working on this version, so it could probably be optimized somewhat, but I doubt we'll get the first-hash time down much, and I think this way will always be slower.

Fixes #613.
Fixes #494.

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

I couldn't think of any way to make a useful and meaningful class that
has no state and also has no custom __reduce__ method, so I went
minimalist with it.
Previously, there was some miniscule risk of hash collision, and also it
was relying on the implementation details of `pickle` (the assumption
that `hash()` is never called as part of `pickle.loads`).
Rather than trying to modify the serialization input and output for
objects with a cached hash, we instead create a cache keyed off of the
unique object identifier, and use weak references to the objects to
keep the cache from leaking memory.

This will make the first hash and subsequent hash lookups slower at the
cost of making it easier to write a custom __reduce__.
@gabbard
Copy link
Member

gabbard commented Jan 14, 2020

I would be inclined to say the performance difference would out-weigh the simplification to the attrs code, especially since if someone is bothering to turn on cache_hash=True, it's probably because they know or suspect the object may be used in a context where performance matters (this is certainly true for many of our uses of it at my workplace). But that's not my call. :-)

@pganssle
Copy link
Member Author

This seems like the worst of the approaches, so I'm going to go ahead and close it.

@pganssle pganssle closed this Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants