-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: build logger messages lazily #3643
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
Conversation
Thank you! Before merging, I'd like to make sure that this actually makes a 'meaningful' difference in performance before we sacrifice code consistency. Do you happen to know what magnitude of faster this is? If not, would you mind running a couple benchmarks? |
This is on the backburner for now. I can tell you it saves about 33% of the time vs the existing code but I know you're going to want to see some data for this. I'm still going to put a benchmark together for that, when I get a chance. Stay tuned! |
@kclowes Another day, another 15 minute wait period while my stuff spins up... Perfect time to put this data together: For this benchmark I used a logger.debug statement comparable to the one found in HTTPProvider.make_request, since every single request goes thru this function. import logging
from time import time
logger = logging.getLogger('test')
endpoint_uri = 'https://somewebsite.com/somereallylongauthkey/'
method = "eth_call"
response = {"jsonrpc": "2.0", "id": 1, "result": "0x" + "0" * 64}
def test_old_3_inputs():
logger.debug(
f"Getting response HTTP. URI: {endpoint_uri}, Method: {method}, Response: {response}",
)
def test_new_3_inputs():
logger.debug(
"Getting response HTTP. URI: %s, Method: %s Response: %s",
endpoint_uri,
method,
response,
)
start = time()
for _ in range(50_000):
test_old_3_inputs()
print(f"old: {time() - start}")
start = time()
for _ in range(50_000):
test_new_3_inputs()
print(f"new: {time() - start}")
So for every ~175k requests it will save about 1s, this is not an insignificant savings for a power user. My 33% estimate above came from my own tests with only 1 input, not 3. So turns out this is more impactful than I originally intended! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! I left one nit, but I think that's it. Do you want to add a newsfragment or should I?
@@ -164,36 +164,34 @@ def get_endpoint_uri_or_ipc_path(self) -> str: | |||
) | |||
|
|||
async def connect(self) -> None: | |||
endpoint = self.get_endpoint_uri_or_ipc_path() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm too. Thanks for benchmarking!
I committed your nit, and added the newsfragment. I think we're good to go! |
oop, that nit was a bad suggestion, sorry. I didn't see that |
fixed, everything is green again |
- Create 3643.performance.rst - chore: black . - fix: linter errors
rebased with main and squashed, merging 👍🏼 |
What was wrong?
When using the logging module, we currently build the log message eagerly which leads to wasted cpu if the logger is not enabled.
According to DuckDuckGo's silly AI assistant:
"Using f-strings for logging in Python is generally discouraged because they evaluate the string immediately, which can lead to unnecessary computations even if the log level is not set to display that message. Instead, it's recommended to use the traditional formatting methods, like the % operator, which defer the string formatting until it's actually needed, improving performance and avoiding potential issues."
With this change, we will build the log messages lazily so the string is never built if the log will not actually be emitted, saving compute. This is just a microoptimization but will make a difference for power users such as myself who have use cases that require making a high volume of requests over a long period.
Related to Issue # N/A
Closes # N/A
How was it fixed?
replacing f-strings with strings which will be used with str.format
Todo:
Cute Animal Picture