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

Do we want to guarantee deepcopied attributes when converting ? #43

Closed
cournape opened this issue May 18, 2016 · 8 comments
Closed

Do we want to guarantee deepcopied attributes when converting ? #43

cournape opened this issue May 18, 2016 · 8 comments

Comments

@cournape
Copy link
Contributor

cournape commented May 18, 2016

A reminiscence of #28:

# python >= 3.3, rough timing
import time

from attr import attributes, attr
from attr.validators import instance_of

def decode_if_needed(v):
    if isinstance(v, bytes):
        return v.decode()

@attributes
class Arch(object):
    name = attr("name")

@attributes
class ConvertedArch(object):
    name = attr("name", validator=instance_of(str), convert=decode_if_needed)

def timeit(func):
    times = []
    for i in range(10000):
        start = time.perf_counter()
        func()
        times.append(time.perf_counter() - start)
    return "Min cost across 10000 iterations: {:.2f} us".format(min(times) * 1e6)

print(timeit(lambda: Arch("x86")))
print(timeit(lambda: ConvertedArch(b"x86")))
Min cost across 10000 iterations: 1.40 us
Min cost across 10000 iterations: 43.72 us

Now, the fix may not be as trivial as #28, because I can see how it may be part of the API that the original attribute is not touched when using convert, and not using fields to iterate in https://github.com/hynek/attrs/blob/master/src/attr/_make.py#L447 would break that promise. As I have not seen any test related to this, maybe that promise could be relaxed ?

@hynek
Copy link
Member

hynek commented May 18, 2016

Eh could you rephrase + elaborate please? I have a nasty head cold and can’t entirely follow. :)

@cournape
Copy link
Contributor Author

Let me try again :)

As in #28, the speed difference is because we iterate over the attr descriptors using fields, which makes a deepcopy for every field.

In #28, we've fixed it by iterating using inst.__class__.__attrs_attrs__. I was about to prepare a PR to do the same here, but then realized that the deepcopy may have bene intentional in this case. If not, I will be happy to prepare a PR to fix this.

@hynek
Copy link
Member

hynek commented May 18, 2016

Um this would only matter, if someone would modify the attribute list in their container, right? (which they never should)

@cournape
Copy link
Contributor Author

Ok, let me prepare a PR then.

What is the rationale for using deepcopy in fields then ?

@hynek
Copy link
Member

hynek commented May 19, 2016

I find there’s a difference between a public API like fields and misbehaving in an converter. But maybe I’m wrong. :)

@hynek
Copy link
Member

hynek commented May 21, 2016

do you think you can get it done by say Wed? I’d like to push out a release before PyCon

@cournape
Copy link
Contributor Author

looks like I could ! Too bad I could not make it up this year at PyCon...

@hynek
Copy link
Member

hynek commented May 21, 2016

aw :(

@hynek hynek closed this as completed May 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants