pyapp-kit / psygnal

Python observer pattern (callback/event system). Modeled after Qt Signals & Slots (but independent of Qt)
https://psygnal.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
86 stars 15 forks source link

fast: add `emit_fast` method for 10x faster emission (without safety checks) #331

Closed tlambert03 closed 3 weeks ago

tlambert03 commented 4 weeks ago

This PR is in response to @beniroquai's https://github.com/pyapp-kit/psygnal/issues/330#issuecomment-2436321438:

but the frame-rate drops significantly in the psygnal case

Indeed, the compiled version of psygnal is roughly 5 times slower than Qt (the uncompiled version is about 6.5-7 times slower). It looks like all that slowdown comes from safety mechanisms and other "features" that may not be used in many cases, (like the ability to query the sender, etc...). So, this PR adds a stripped down emit_fast() method that has the following changes:

By stripping away those less-used features, the emission is about 10x faster (making the compiled version ~2x faster than Qt). It should be used with caution, but for most simple cases, it is probably everything one needs.

codspeed-hq[bot] commented 4 weeks ago

CodSpeed Performance Report

Merging #331 will improve performances by 11.08%

Comparing tlambert03:emit-fast (a78324e) with main (019232e)

Summary

⚡ 1 improvements ✅ 65 untouched benchmarks

🆕 1 new benchmarks

Benchmarks breakdown

Benchmark main tlambert03:emit-fast Change
🆕 test_emit_fast N/A 22.1 µs N/A
test_emit_time[partial-2] 69 µs 62.1 µs +11.08%
codecov[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (019232e) to head (a78324e). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #331 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 21 21 Lines 2062 2076 +14 ========================================= + Hits 2062 2076 +14 ```

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

beniroquai commented 3 weeks ago

This sounds great! Thanks a lot for all the effort! Is it an in-place upgrade or would I need to tweak any of the settings? CC: @jacopoabramo

Czaki commented 3 weeks ago

You need to call emit_fast instead of emit so it may require some subclassing if you want to use the same interface as in QSignal.

beniroquai commented 3 weeks ago

That's it? Super cool! Will test it later today! Also - maybe a not perfectly fitting side question - Is there a proper way to enforce --no-binary when installing psygnal through the setup.py? I have used this without knowing if it actually does the trick (I don't have an x86 with me unfortunately..).

jacopoabramo commented 3 weeks ago

This sounds great! Thanks a lot for all the effort! Is it an in-place upgrade or would I need to tweak any of the settings? CC: @jacopoabramo

I guess you can just replace the call to emit with emit_fast within the child class. The base API doesn't change.

jacopoabramo commented 3 weeks ago

Out of curiosity, how does emit_fast drop support for thread safety locks? From the PR it doesn't that there's anything related to threading management but I don't know the whole code base very well

tlambert03 commented 3 weeks ago

how does emit_fast drop support for thread safety locks

the "standard" emit() function mostly just defers to the internal _run_emit_loop function:

https://github.com/pyapp-kit/psygnal/blob/d6281fc25a4f12d997078365cfc9baeec2a52fec/src/psygnal/_signal.py#L1230-L1242

in the interest of speed, emit_fast doesn't call that function, it just directly iterates over callbacks, without first entering the two context managers at the top of _run_emit_loop (with self._lock and with Signal._emitting(self)). I testing, i found entering and existing those those contexts (on every emission) was a good part of the slowdown. Even if I used contextlib.nullcontext() ... so I think it's indicative of something deeper in python's context manager implementation.

We could add back the thread safety, at a slight cost, but even adding a conditional to check an argument (e.g. thread_safe=True) would cause some overhead

tlambert03 commented 3 weeks ago

Is there a proper way to enforce --no-binary when installing psygnal through the setup.py

@beniroquai ... no there's no way to enforce that in your dependencies, since that is a pip-specific option at install time, and not related to project metadata. You can, however, tell pip to install with no_binary for a specific package at install time:

PIP_NO_BINARY=psygnal pip install ImSwitch 

Alternatively, you can do the "decompilation" (which simply renames the compiled libraries) in your source code, next to where you subclass, though this requires write access to the psygnal files in site-packages:

# somewhere in your ImSwitch code
import psygnal.utils
psygnal.utils.decompile()

note: i haven't actually tried this :joy: ... so it's possible that the first run would fail... making it not a great solution

... I'd call that a short-term patch, until I have time to look closer at #330

edit: if your solution of installing from github works for you, then that's also appropriate I suppose ... but i think i'd prefer one of the above solutions