-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Debug fstring bottleneck in sending #2102
Comments
@dvora-h mind taking a look? This looks like a bug we picked up in the cluster PR and should not be present. |
@chayim @dvora-h can i take this issue and fix it all over the code? I'm happy to 😊 Here is explanation about why %-string is better: https://blog.pilosus.org/posts/2020/01/24/python-f-strings-in-logging/ |
There's been a lot of discussion on f-strings being better than %s formatters recently (and hence the switch to that in redis-py). If we can demonstrate this is true, then we should also remove the string checker for CI (cc @dvora-h ) |
f-strings are only a bottleneck for logging because %s substitutions are deferred & possibly skipped, like in the case of debug logs in cluster code, which were recently removed in #2238. |
@chayim I'm happy to demonstrate, do you mean in general demonstration, or in redis's use case? If you mean in redis, do you have a set of performance tests which I can use? |
@alongouldman We don't have perf tests here in the repo that are viable - though we keep starting and stopping that. I mean the redis use case. But frankly, if it's generically pythonic (as in how list comprehensions beat out pure for loops), that's fine. |
Hi @chayim This is the test I run:
These are the results:
(I tried to change the order of the function calls, just to make sure that its nothing to do with the order for some reason) Looks like in a single short string format, there is no much difference... but either long text, or multiple arguments, %-format is faster. What do you think? Should I change that around redis-py's codebase? |
This issue is marked stale. It will be closed in 30 days if it is not updated. |
In the file cluster.py there is a line that logs the args as debug using a fstring. This is a significant bottleneck when sending large messages even when logging is disabled since the fstring is always evaluated. Please could it be changed as follows:
diff cluster.py cluster.py -u
--- cluster.py 2022-04-12 09:47:46.545790433 +0000
+++ cluster.py 2022-04-12 09:51:55.789286947 +0000
@@ -900,7 +900,7 @@
def _should_reinitialized(self):
The text was updated successfully, but these errors were encountered: