Performance issues with hashing attrs objects

[DRMacIver]

Backstory: I got a bug about a performance regression reported in Hypothesis ( HypothesisWorks/hypothesis#919 ) and in the profiling data in the linked issue, the majority of the extra time (10 out of 13 seconds) is in attrs hashing.

This makes sense, as the enclosing method where most of the time is being spent does a lot of hashing of attrs objects, but it was still a bit surprising that it was this slow.

The class being hashed looks as follows:

@attr.s(slots=True, frozen=True)
class Arc(object):
    filename = attr.ib()
    source = attr.ib()
    target = attr.ib()

I have worked around this problem by now by removing attrs from this class and memoizing creation of it so that all value equal objects are reference equal, which is a bit more of an extreme solution than I expect attrs to support, but means that this bug is in no way critical for me as attrs will no longer be on the hot path of this code. It will probably however limit attrs uptake inside Hypothesis for now (I am likely to add a bunch more classes with a similar usage pattern and now will probably follow the pattern of the workaround I used here rather than using attrs).

The obvious two things that attrs could do to help are:

• Have faster hashing. Almost all of that hashing time is in _attrs_to_tuple. Not creating these intermediate tuples (or creating them faster somehow) might be a significant win here.
• Add support for caching the hash on the object. In my usage pattern the same objects were being hashed over and over again, and paying the hashing cost each time.

[Tinche]

Both of these are probably worth doing. Our hash methods are generic, that’s why they’re slow. It’s just that no one complained until now :) It’d be a rather straightforward change to generate a specialized hash for each class that would be more efficient.

Caching would be good too (as Hynek mentioned, frozen classes only), just need to figure out where to stash the cache.

[DRMacIver]

Caching would be good too (as Hynek mentioned, frozen classes only), just need to figure out where to stash the cache.

FWIW the following is what I'd probably do (which you should feel to disregard as my design opinions are weird, but I feel like they're weird in a way that is very compatible with attrs):

• Add a memoize_hash flag to attr.s defaulting to None.
• When memoize_hash is True, store the hash on a field called __hash.
• If hash is False (or inferred False from a default) it is an error to pass non-None memoize_hash
• If slots and memoize_hash are both True, add a bonus __hash slot to the class to store it.
• If hash is True, memoize_hash defaults to frozen and not slots

[hynek]

I think I see another eval in our immediate future.

[hynek]

I’m sitting in a car on a wine tour but I think I’d be OK with a simple specific eval-based implementation for 17.3. That should help already and should be straight-forward.

[DRMacIver]

Our hash methods are generic, that’s why they’re slow. It’s just that no one complained until now :)

@DRMacIver in "Uses libraries in weird ways that nobody else has done before" shocker. 😉

[hynek]

It might be the Chenin Blanc talking but isn’t hashing invoked whenever you put/get something into dicts and sets?

[DRMacIver]

It is. The unusual thing about what I'm doing I think is key reuse and just how much I'm doing that.

[DRMacIver]

I was in the process of profiling the code for the Hypothesis issue and I noticed that I'm also seeing attrs's generated equality methods in the profile code. This is also using attrs_to_tuple. If you're going to be doing an eval to get hash, __eq__ should probably come along for the ride.

[DRMacIver]

While I'm giving you a wishlist (sorry 😉), I note that the equality generated by attrs doesn't start with an is check. i.e. it would be much faster if it began with if self is other: return True.

The big problem with adding this is that it changes the semantics in the presence of e.g. NaN. Previously x = Foo(float('nan')); x == x would be False, and adding a reference equal check would make it True. Would you be OK with that change?

[DRMacIver]

I note that the equality generated by attrs doesn't start with an is check

I have however also remembered that this is irrelevant for collection performance because the semantics of getitem/setitem/etc bypass equality for reference equal values. So maybe a separate concern.

[hynek]

Did the situation improve as of attrs 17.4 @DRMacIver?

[mr-nfamous]

No idea how to contribute to projects so I'll just post here my solution that makes _make_hash 3x faster:

def _make_hash(attrs):
    attrs = tuple(
        a
        for a in attrs
        if a.hash is True or (a.hash is None and a.cmp is True)
    )
 
    attrs_getter = attrgetter(*map(attrgetter('name'), attrs))
    def hash_(self):
        """
        Automatically created by attrs.
        """
        return hash(attrs_getter(self))
 
    return hash_

[hynek]

Did you benchmark it against the current (17.4.0) implementation? We generate code like for __init__ now so I’d be surprised if an attrsgetter-based approach is 3x faster; or faster at all.

[hynek]

I’m gonna interpret the lack of feedback as that the issue is at least roughly resolved.

[DRMacIver]

I’m gonna interpret the lack of feedback as that the issue is at least roughly resolved.

Sorry about that. I don't have a convenient way to benchmark this as I ended up taking all of this code down a different route where it no longer really makes sense as an attrs benchmark. If you consider it fixed I'm happy, as I'm no longer affected by it either way, and if I run into problems here in future I'll reopen.