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

Remove unsound return of borrowed objects #890

Merged
merged 2 commits into from
May 4, 2020

Conversation

davidhewitt
Copy link
Member

As discussed in #883, I think these are a few cases where we should be converting borrowed objects to owned objects for safety reasons.

Closes #883.

@davidhewitt davidhewitt mentioned this pull request May 2, 2020
@davidhewitt
Copy link
Member Author

davidhewitt commented May 2, 2020

In case we're worried about performance regressions, I made some benchmarks (from #891). I ran them on master, this PR, #887 , and also #887 with this PR cherry-picked on top.

Results:

  master no-borrowed-objects new-nativetypes new-nativetypes + no-borrowed-objects
dict_get_item 3160 +- 820 3120 +- 750 2280 +- 1600 2530 +- 1260
iter_dict 3680 +- 1000 3720 +- 700 1620 +- 700 2330 +- 560
iter_list 2090 +- 550 2340 +- 1300 1080 +- 590 1610 +- 430
list_get_item 1080 +- 410 1100 +- 400 470 +- 230 740 +- 320
drop_many_objects 4.4 +- 1.2 4.8 +- 0.4 3.8 +- 0.4 3.7 +- 0.9
iter_tuple 780 +- 110 760 +- 560 790 +- 300 820 +- 340
tuple_get_item 960 +- 250 950 +- 100 420 +- 200 580 +- 330

Conclusions:

  • This PR does now slow down vs master much (maybe just a little)
  • new-nativetypes is much much faster than master (yay!)
  • Applying this PR after new-nativetypes causes greater slowdown because the difference between borrowed and owned objects is greater after that. But the combination is still faster than master 😄

So I would be really keen to see this safety gap closed. If we are really worried about performance we could add unsafe fn get_item_borrowed to PyList etc., but I don't think the performance difference is so great that it's worth it.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!

src/types/tuple.rs Outdated Show resolved Hide resolved
davidhewitt added a commit to davidhewitt/pyo3 that referenced this pull request May 3, 2020
@davidhewitt
Copy link
Member Author

I've undone the change for PyTuple::get_item

davidhewitt added a commit to davidhewitt/pyo3 that referenced this pull request May 3, 2020
@kngwyu
Copy link
Member

kngwyu commented May 4, 2020

Thank you!

@kngwyu kngwyu merged commit 879b0b5 into PyO3:master May 4, 2020
@davidhewitt davidhewitt deleted the no-borrowed-objects branch August 10, 2021 07:19
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.

Returning borrowed objects is probably always unsound
2 participants