JuliaPy / PyCall.jl

Package to call Python functions from the Julia language
MIT License
1.45k stars 186 forks source link

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

Open songjhaha opened 9 months ago

songjhaha commented 9 months ago

This PR is trying to solve the high latency when using PyCall in second time like https://github.com/JuliaPy/PyCall.jl/issues/1052.

We found the method defination Base.convert(::Type{Nothing}, ::MyType) would create many method invalidations, which casues this latency (see https://github.com/JuliaLang/julia/issues/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 commented 9 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1055/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy) | `67.44% <100.00%> (+0.13%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy) | Coverage Δ | | |---|---|---| | [src/conversions.jl](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy#diff-c3JjL2NvbnZlcnNpb25zLmps) | `63.32% <100.00%> (+0.58%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1055/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MilesCranmer commented 5 months ago

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 https://github.com/JuliaLang/julia/pull/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