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

Improve performance on pointer drop #851

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Apr 7, 2020

There's a Mutex involved each time we drop a PyObject or Py<T>, so that we can safely push the reference into the shared Vec. I have been wondering for a while if we could add a simple check if the GIL is held (as a thread_local! variable) and if so, decrease the refcount immediately.

I tried this out - turns out in the simple benchmark I made (bench_object) that adding this check makes the benchmark 5x faster.

I think decreasing the reference count sooner is also more predictable from a pyo3 user perspective. It helps with some of the issues around #311. (Though this doesn't improve the situation for references.)

If the GIL is not held, then I made it so that the pointers will be released as soon as the GIL is next acquired.

To track when the GIL is held I have added counters to GILGuard::new() and GILGuard:drop().

For this optimization to apply all the time we also need to increment / decrement this counter when we call assume_gil_acquired() - which suggests to me that we should wait to finish this off once #800 is solved.

TODO:

  • Also increment / decrement GIL_COUNT when we call assume_gil_acquired() (solved by also using GILPool for the counter)

let pool: &'static mut ReleasePool = &mut *POOL;
pool.release_pointers();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to release pointers here?
To reduce memory usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two reasons:

  1. Yes, memory usage - we're now releasing the pointers sooner
  2. This (Why object creation is slow in PyO3 and how to enhance it #679 (comment)) suggested to me that it's important in the long run we migrate to releasing objects on GIL acquire, not GIL exit.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I understand. Thank you for the reference.

benches/bench_object.rs Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Apr 8, 2020

Great, thanks!

For this optimization to apply all the time we also need to increment / decrement this counter when we call assume_gil_acquired()

I'm not sure how the next assume_gil_acquired API would be so I think it's sufficient if GILPool increments/decrements the counter for now.

@davidhewitt davidhewitt force-pushed the pointer-optimization branch 2 times, most recently from 66aaf86 to ca027cf Compare April 8, 2020 08:08
@davidhewitt
Copy link
Member Author

Good idea. I now use both GILGuard and GILPool for the counter, which should cover all use cases.

@davidhewitt davidhewitt marked this pull request as ready for review April 8, 2020 08:09
src/gil.rs Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Formatting error again 🤦 I'll fix later

@davidhewitt
Copy link
Member Author

Re-pushed with fixed formatting; should be green in CI now I hope.

src/gil.rs Show resolved Hide resolved
src/gil.rs Outdated Show resolved Hide resolved
src/gil.rs Outdated Show resolved Hide resolved
Co-Authored-By: Yuji Kanagawa <yuji.kngw.80s.revive@gmail.com>
@kngwyu
Copy link
Member

kngwyu commented Apr 10, 2020

Thank you!

@kngwyu kngwyu merged commit 7b1e8a6 into PyO3:master Apr 10, 2020
@davidhewitt davidhewitt deleted the pointer-optimization 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.

2 participants