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

reduce method invalidations and latency by remove method of converting PyObject to nothing #1055

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

songjhaha
Copy link

@songjhaha songjhaha commented Sep 26, 2023

This PR is trying to solve the high latency when using PyCall in second time like #1052.

We found the method defination Base.convert(::Type{Nothing}, ::MyType) would create many method invalidations, which casues this latency (see JuliaLang/julia#51389), so this PR remove the method convert(::Type{Nothing}, ::PyObject), and some patches by @thautwarm solve the auto converting convert(::Type{PyAny}, o::PyObject).

before this PR:

julia> @time using PyCall
  0.478829 seconds (255.49 k allocations: 18.250 MiB, 1.91% compilation time)

julia> @time using PyCall
  1.957530 seconds (1.29 M allocations: 90.161 MiB, 5.23% gc time, 99.97% compilation time: 100% of which was recompilation)

after this PR:

julia> @time using PyCall
  0.424794 seconds (261.29 k allocations: 18.610 MiB, 2.36% compilation time)

julia> @time using PyCall
  0.000566 seconds (309 allocations: 21.836 KiB)

This latency time is estimated in Julia1.9.3 and win10 OS

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2a9f077) 67.30% compared to head (436b314) 67.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1055      +/-   ##
==========================================
+ Coverage   67.30%   67.44%   +0.13%     
==========================================
  Files          20       20              
  Lines        2025     2024       -1     
==========================================
+ Hits         1363     1365       +2     
+ Misses        662      659       -3     
Flag Coverage Δ
unittests 67.44% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/conversions.jl 63.32% <100.00%> (+0.58%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MilesCranmer
Copy link
Contributor

MilesCranmer commented Jan 8, 2024

Hey @songjhaha,
Sorry this hasn't been reviewed yet. I just saw this PR while scrolling by and was wondering if this still has an impact on v1.10 or whether JuliaLang/julia#49525 was enough to fix this issue?

I see this locally:

julia> @time using PyCall
  0.311416 seconds (367.73 k allocations: 24.860 MiB, 3.42% gc time, 66.53% compilation time: 98% of which was recompilation)

julia> @time using PyCall
  0.000159 seconds (151 allocations: 13.984 KiB)

Thanks!
Miles

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.

None yet

3 participants