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

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

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Nov 4, 2024

This PR is in response to @beniroquai's #330 (comment):

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:

  • It does not support check_nargs or check_types
  • It does not use any thread safety locks.
  • It is not possible to query the emitter with Signal.current_emitter()
  • It is not possible to query the sender with Signal.sender()
  • It does not support "queued" or "latest-only" reemission modes for nested
    emits. It will always use "immediate" mode, wherein signals emitted by
    callbacks are immediately processed in a deeper emission loop.

It DOES, however, respect paused() and blocked()

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.

@tlambert03 tlambert03 changed the title fast: add emit_fast method for 10x faster emission (without safety checks) fast: add emit_fast method for 10x faster emission (without safety checks) Nov 4, 2024
Copy link

codspeed-hq bot commented Nov 4, 2024

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%

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

Looks good.
did you want to add some tests to make codecov more happy?

In not, then maybe add #pragma: no cov to remove noisy reports?

src/psygnal/_signal.py Outdated Show resolved Hide resolved
Comment on lines +1211 to +1214
raise RecursionError(
f"RecursionError when "
f"emitting signal {self.name!r} with args {args}"
) from e
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of this solution, as it sometimes produces unreadable stack trace. But I understand that it is just a copy from plain emmit and should be fixed in both places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please suggest a better solution. You've complained about the stack traces in the past, and I tried hard to make what I thought were useful, readable traces. I didn't know you still didn't like them, but it would be good to have some more concrete suggestions

@tlambert03 tlambert03 merged commit 3778c9f into pyapp-kit:main Nov 4, 2024
56 checks passed
@tlambert03 tlambert03 deleted the emit-fast branch November 4, 2024 14:56
@beniroquai
Copy link

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
Copy link
Contributor

Czaki commented Nov 4, 2024

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
Copy link

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
Copy link

jacopoabramo commented Nov 4, 2024

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
Copy link

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
Copy link
Member Author

tlambert03 commented Nov 4, 2024

how does emit_fast drop support for thread safety locks

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

psygnal/src/psygnal/_signal.py

Lines 1230 to 1242 in d6281fc

def _run_emit_loop(self, args: tuple[Any, ...]) -> None:
with self._lock:
self._emit_queue.append(args)
if len(self._emit_queue) > 1:
return
try:
# allow receiver to query sender with Signal.current_emitter()
self._recursion_depth += 1
self._max_recursion_depth = max(
self._max_recursion_depth, self._recursion_depth
)
with Signal._emitting(self):
self._run_emit_loop_inner()

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
Copy link
Member Author

tlambert03 commented Nov 4, 2024

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 😂 ... 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants