8000 remove key-existence checks against dict.keys() calls by bentsku · Pull Request #11694 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

remove key-existence checks against dict.keys() calls #11694

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 16, 2024
Merged

remove key-existence checks against dict.keys() calls #11694

merged 1 commit into from
Oct 16, 2024

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Oct 15, 2024 •
edited
Loading

Motivation

While looking over some performance improvements for SQS, I've stumbled into a key existence checks like that if key in dict.keys().

Calling dict.keys() will create a dict view of the dict keys. In itself, this is not terrible, but it has a bit of a cost performance.

Giving it a small benchmark:

Small benchmarking code 

import timeit


test_dict = {f"key{k}": k for k in range(1000)}
last_key = list(test_dict)[-1]


def check_in_keys():
    p = last_key in test_dict.keys()


def check_in_dict():
    p = last_key in test_dict


def check_not_in_keys():
    p = last_key not in test_dict.keys()


def check_not_in_dict():
    p = last_key not in test_dict

Give the following results for a dictionary with 1000 entries:

  • Check in dict.keys(): 0.04537 (+62%)
  • Check in dict 0.02794

(Changing the dict size does not change the results too much, it's always at least +50%)

I thought about introducing a lint rule for it, I found the following SIM118, but it's a bit too eager and will also change for loops, which I believe sometimes makes it a bit more readable and I haven't touched yet.

https://docs.astral.sh/ruff/rules/in-dict-keys/

Changes

  • remove occurrences of key-existence checks against dict.keys() in favor of in dict.

@bentsku bentsku self-assigned this Oct 15, 2024
@bentsku bentsku added the semver: patch Non-breaking changes which can be included in patch releases label Oct 15, 2024
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 41m 58s ⏱️ - 1m 57s
3 496 tests ±0  3 083 ✅ ±0  413 💤 ±0  0 ❌ ±0 
3 498 runs  ±0  3 083 ✅ ±0  415 💤 ±0  0 ❌ ±0 

Results for commit 0b631b1. ± Comparison against base commit 32e05ec.

@bentsku bentsku requested a review from alexrashed October 16, 2024 00:37
@bentsku bentsku marked this pull request as ready for review October 16, 2024 00:38
viren-nadkarni
viren-nadkarni approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member
@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

LTGM 🚀

alexrashed
alexrashed approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Nice drill down and performance evaluation! The changes are looking good!
But I would really encourage you to try and enable the linting rule to be honest. Why would you say it has a negative impact on readability?

@bentsku
Copy link
Contributor Author
bentsku commented Oct 16, 2024

But I would really encourage you to try and enable the linting rule to be honest. Why would you say it has a negative impact on readability?

@alexrashed I gave the lint rule a try locally and the results of the formatting and errors were not really good to be honest, it modified lots of instance in list comprehensions and for loops, even though the rule itself says: "Checks for key-existence checks against dict.keys() calls.". Also, it raised a lot of errors for "unsafe" checks, when the linter cannot be 100% sure the object is a dict, as for example Headers has the keys method as well, but it might not behave like a dict, which we might need to ignore.

Plus, having a clear loop of for key in dict.keys() helps a bit for readability and might make sense sometimes to be very specific, and this lint rule would prevent this somehow.

@bentsku bentsku merged commit be2aba2 into master Oct 16, 2024
40 of 41 checks passed
@bentsku bentsku deleted the remove-in-keys-check branch October 16, 2024 10:17
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@bentsku @macnev2013
remove key-existence checks against dict.keys() calls (#11694)
987058a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

@viren-nadkarni viren-nadkarni viren-nadkarni approved these changes

@MEPalma MEPalma Awaiting requested review from MEPalma MEPalma is a code owner

@joe4dev joe4dev Awaiting requested review from joe4dev joe4dev is a code owner

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

@thrau thrau Awaiting requested review from thrau thrau is a code owner

@baermat baermat Awaiting requested review from baermat baermat is a code owner

@macnev2013 macnev2013 Awaiting requested review from macnev2013 macnev2013 is a code owner

@maxhoheiser maxhoheiser Awaiting requested review from maxhoheiser maxhoheiser is a code owner

@Morijarti Morijarti Awaiting requested review from Morijarti

@giograno giograno Awaiting requested review from giograno giograno is a code owner

@pinzon pinzon Awaiting requested review from pinzon pinzon is a code owner

@steffyP steffyP Awaiting requested review from steffyP steffyP is a code owner

@simonrw simonrw Awaiting requested review from simonrw simonrw is a code owner

@cloutierMat cloutierMat Awaiting requested review from cloutierMat cloutierMat is a code owner

Assignees

@bentsku bentsku

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bentsku @alexrashed @viren-nadkarni
0