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

Generate __init__ with converters inline. #80

Closed

Conversation

@Tinche
Copy link
Member

@Tinche Tinche commented Sep 7, 2016

Here's some more horror. :)

The gains are actually more than I expected.

> pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', {'x': attr.ib(convert=str), 'y': attr.ib(convert=float), 'z': attr.ib(), 'a': attr.ib()}, slots=True)" "C(1,2,3,4)"
> # Before:
Median +- std dev: 2.41 us +- 0.06 us
> # After:
Median +- std dev: 1.02 us +- 0.03 us

Frozen classes:

> pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', {'x': attr.ib(convert=str), 'y': attr.ib(convert=float), 'z': attr.ib(), 'a': attr.ib()}, slots=True, frozen=True)" "C(1,2,3,4)"
> # Before: 
Median +- std dev: 2.94 us +- 0.08 us
> # After: 
Median +- std dev: 1.64 us +- 0.04 us

I was actually a little weirded out, but the tests pass and playing around in the repl, it seems to work.

I thought the gains would be greatest with about 50% attributes with converters on them, but no, even with all attributes having converters, I see more than double the speed.

> pyperf timeit --rigorous  -g -s "import attr; C = attr.make_class('C', {'x': attr.ib(convert=str), 'y': attr.ib(convert=str), 'z': attr.ib(convert=str), 'a': attr.ib(convert=str)}, slots=True)" "C(1,2,3,4)"
> # Before:
Median +- std dev: 3.41 us +- 0.07 us
> #After:
Median +- std dev: 1.37 us +- 0.04 us
@codecov-io
Copy link

@codecov-io codecov-io commented Sep 7, 2016

Current coverage is 100% (diff: 100%)

Merging #80 into master will not change coverage

@@           master   #80   diff @@
===================================
  Files           8     8          
  Lines         398   413    +15   
  Methods         0     0          
  Messages        0     0          
  Branches       88    88          
===================================
+ Hits          398   413    +15   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update dd36808...4da3a4e

@hynek
Copy link
Member

@hynek hynek commented Sep 8, 2016

Omg 🙈

@Tinche
Copy link
Member Author

@Tinche Tinche commented Sep 8, 2016

@hynek
Copy link
Member

@hynek hynek commented Sep 8, 2016

Before I have closer look at your abomination, please add a changelog entry. :)

@Tinche
Copy link
Member Author

@Tinche Tinche commented Sep 8, 2016

Done.

@hynek
Copy link
Member

@hynek hynek commented Sep 9, 2016

At this point I feel like we should write some integration test for _attrs_to_script that goes for some typical use cases and just compares strings afterward. I’m stopping to trust our tests at this point. %)

@Tinche
Copy link
Member Author

@Tinche Tinche commented Sep 9, 2016

Not really sure comparing strings of source code will be a better test than testing the actual behavior. I guess we could assert that the generated init is the most optimal it can be, but we'd also have to take into account some internal stuff, like the __attr_validator_x and __attr_convert_x globals.
🤔

@Tinche
Copy link
Member Author

@Tinche Tinche commented Sep 9, 2016

It's kind of a shame there aren't any libraries for generating Python source code. Or at least I can't find any. Libraries that would expose objects and just let you render to source at the end, instead of glueing strings together.

Maybe I'll write something. 😎

@hynek
Copy link
Member

@hynek hynek commented Sep 9, 2016

I’m not really saying we should do it. I’m just saying that function haunts me at night. ;)

@hynek hynek closed this in 5428710 Sep 10, 2016
@Tinche Tinche deleted the Tinche:optimization/inline-converters branch Sep 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants