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

experimental: owned objects API (alternative) #1304

Closed
wants to merge 9 commits into from

Conversation

davidhewitt
Copy link
Member

This is a fork of #1300 to use PyAny<'py> as an owned pointer, as per discussion in that thread. The PyOwned type is removed.

I haven't implemented any design around PyAnyRaw / PySend / Py yet.

Overall this design feels simpler to work with, so I plan to continue refining this branch over the original one.

The tests aren't all passing yet, but I did run a benchmark:

benchmark current api experimental api (PyOwned) experimental api (PyAny)
bench_call_0 52,895 ns/iter (+/- 6,956) 54,954 ns/iter (+/- 2,175) 51,247 ns/iter (+/- 1,852)
bench_call_method_0 153,977 ns/iter (+/- 9,671) 153,001 ns/iter (+/- 6,288) 141,100 ns/iter (+/- 10,980)
dict_get_item 1,933,300 ns/iter (+/- 328,382) 1,780,027 ns/iter (+/- 79,625) 1,700,250 ns/iter (+/- 52,607)
extract_btreemap 9,538,110 ns/iter (+/- 634,895) 8,688,905 ns/iter (+/- 590,359) 8,201,100 ns/iter (+/- 153,532)
extract_btreeset 8,099,900 ns/iter (+/- 570,799) 7,842,395 ns/iter (+/- 535,694) 7,569,645 ns/iter (+/- 144,288)
extract_hashmap 5,013,300 ns/iter (+/- 483,341) 4,292,595 ns/iter (+/- 358,152) 4,133,425 ns/iter (+/- 231,738)
extract_hashset 5,418,065 ns/iter (+/- 440,259) 4,900,090 ns/iter (+/- 355,389) 4,730,320 ns/iter (+/- 86,596)
iter_dict 1,759,862 ns/iter (+/- 209,509) 1,304,683 ns/iter (+/- 133,726) 1,328,632 ns/iter (+/- 203,382)
iter_list 1,262,143 ns/iter (+/- 76,108) 1,109,227 ns/iter (+/- 52,926) 1,163,203 ns/iter (+/- 86,611)
iter_set 1,257,083 ns/iter (+/- 100,680) 1,037,567 ns/iter (+/- 120,639) 1,069,825 ns/iter (+/- 132,988)
iter_tuple 824,397 ns/iter (+/- 46,234) 753,351 ns/iter (+/- 125,318) 990,398 ns/iter (+/- 93,625)
list_get_item 573,128 ns/iter (+/- 61,386) 531,383 ns/iter (+/- 21,875) 530,595 ns/iter (+/- 37,357)
tuple_get_item 408,859 ns/iter (+/- 25,755) 407,605 ns/iter (+/- 21,552) 520,050 ns/iter (+/- 36,936)

So the conclusion is that it's pretty much the same as the other branch. This is good; I was worried it would be slower!

The only slowdown compared to the other branch is that now tuple iteration / get_item must return an owned PyAny.

@davidhewitt davidhewitt marked this pull request as draft December 1, 2020 09:09
@kngwyu
Copy link
Member

kngwyu commented Dec 8, 2020

Thank you for your big efforts in moving this forward!
Could you please consider pushing this branch upstream?
I'm sorry it would force you to do some redundant git&github operations, but I definitely need to play in the code to review such a big PR. Maybe some others can do so.
Now I cannot afford to review this at once but will play with this branch in my break time.

@davidhewitt davidhewitt mentioned this pull request Dec 8, 2020
3 tasks
@davidhewitt
Copy link
Member Author

Sure thing, I pushed this as the experimental-api branch to the main PyO3 repo: https://github.com/PyO3/pyo3/tree/experimental-api

Also opened #1308 to track merging that branch.

@davidhewitt davidhewitt closed this Dec 8, 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
Development

Successfully merging this pull request may close these issues.

2 participants