Codecov Report

Merging #358 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #358   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         806    825   +19     
  Branches      171    173    +2     
=====================================
+ Hits          806    825   +19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 780e1e5...cfc376a. Read the comment docs.

This is going to significantly slow down repr, isn't it? Usually people don't repr on a hot path, but I log attrs classes all the time.

If it gets prohibitively expensive, we can always make it an option.

Is repr speed the bottleneck?
Usually repr is used for logging, errors handling etc. but never for not fast paths if successful code execution.

Guys, it is your library, you establish the rule.
But for example if comprehensive repr in aiohttp takes extra time -- I don't care, correctness first.

I'd like to see some perf numbers before doing anything drastic :) but on the perf front we could also disable this for frozen classes. You can participate in a cycle as a frozen object, but you can't be the cause of the cycle, so it's the (mutable) cyclic object that should be responsible for the ....

Re: logging attrs classes: yes, this is precisely why I really want this to happen :). Sometimes having repr in a hot logging loop be kinda slow is vastly preferable to sometimes entirely losing a log message because an object got into a cycle somehow.

Anyone I can motivate to give me a review? :)

@asvetlov Thanks for your input, your review is appreciated! But per project policy, I do need another project member to sign off :).

So show us some performance numbers?

BTW I log attrs classes all the time on the happy path. Not in hot loops of course, but once per request for sure. A slow repr isn't prohibitive, but maybe it should be opt-in. Personally I have never had a cycle, why pay the price?

Preliminary testing suggests a 20% slowdown on both CPython and PyPy.

Test:

import attr
from textwrap import dedent

@attr.s
class Foo(object):
    bar = attr.ib()
    baz = attr.ib()
    boz = attr.ib()

import timeit
print(timeit.timeit(
    'repr(foo)',
    dedent('''
    from __main__ import Foo
    foo = Foo(1, "hello", ["some", "values"])
    '''),
    number=1000000
))

@Tinche On the one hand, I like the philosophy of "don't pay for what you don't use". On the other hand, what would you call this if you had to opt in to it? dont_crash_on_repr? I don't like "behave correctly" flags. Not sure how to resolve that here, and on balance I feel like this is acceptable overhead for starters, and if we wanted to optimize repr there are a few other things which might be a good idea to investigate as well.

How do you feel about frozen= being the opt-in?

Actually I guess since frozen=False is the default that would make it an opt-out not an opt-in.

@Tinche With the micro-optimization I just committed, this change is about 25% faster on PyPy and about 10% faster on CPython.

It's true that if we removed the code that makes it correct now it could probably be even faster, but presumably if the previous performance was acceptable, faster performance is also acceptable :).

(25%/10% faster than master, I mean, not than the previous implementation of this fix)

Restraining the optimization golfer in me, I think eliminating the .format() call and generator expression is a cromulent and uncontroversial way to address repr performance concerns. +1

Let’s get some numbers.

Hynek’s Molasses

➤ pyperf timeit --rigorous -s 'import attr; C = attr.make_class("C", ["a", "b", "c"]); x = C(1, "2", 3.0)' 'repr(x)'
.........................................
Mean +- std dev: 3.22 us +- 0.09 us

Glyph’s Vroom-Vroom

➤ pyperf timeit --rigorous -s 'import attr; C = attr.make_class("C", ["a", "b", "c"]); x = C(1, "2", 3.0)' 'repr(x)'
.........................................
Mean +- std dev: 3.12 us +- 0.08 us

Hawkie’s Turbo

However, looking at the code reminded me of @hawkowl ’s blog post on string concats (well, she wrote about byte strings but I figured the lessons are transferable) and I did some more micro optimizations that even made the code shorter and nicer. And lo and behold:

pyperf timeit --rigorous -s 'import attr; C = attr.make_class("C", ["a", "b", "c"]); x = C(1, "2", 3.0)' 'repr(x)'
.........................................
Mean +- std dev: 2.99 us +- 0.07 us

So I’m not sure what the etiquette here is but I’ve just pushed my changes against Glyph’s branch. If you (Glyph) are OK with it, we can merge it as soon as CI passes.

Although I have one last question: does this work with greenlets too? Or do we have to do something like I did in structlog?

It should work well enough with greenlets - in principle it's possible to suspend and reentrantly repr something in a different greenlet but the same thread but:

• you'd have to do something truly horrible to make that happen
• if you're doing that reentrantly in a paused greenlet then you might have the same issues with recursion anyway so this logic is not wrong exactly.

If we're happy with the perf numbers, I'm going to go ahead and merge.

I helped, without even having to help. Woo!

“Hawkie’s Turbo” is a thing now

In 3.7 you could use contextvars instead, to avoid saying the Forbidden Word :-).

(I haven't managed to convince Yury to make a backport yet though.)

Yay, everyone's happy!