Skip to content

bpo-46675: Allow object value arrays and split key dictionaries larger than 16 #31191

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

Merged
merged 5 commits into from
Feb 8, 2022

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Feb 7, 2022

At a slight increase in overhead for for small object dictionaries, and an extra word of memory for sizes 7-15, this PR allows split-key dicts and "virtual" object dicts for size > 16.

The benchmarks suggest that this is not an improvement, but looking a bit closely reveals a couple of things.

The logging benchmarks are significantly faster. The log record object has ~20 attributes, so this is an expected improvement.
The pickle benchmarks are significantly slower, I don't know why.

Overall, this may not make things faster, but it does remove a "sharp edge". Adding an extra attribute (from 16 to 17) should not reduce performance by a significant amount.

Why are quite a few of the benchmarks slower?
I don't know, but my hypothesis is that the larger dict-keys is slowing down lookup due to poorer locality.
We may need to revisit static analysis, to provide better hints for the initial size of the dict keys.

https://bugs.python.org/issue46675

@markshannon markshannon requested a review from methane as a code owner February 7, 2022 13:08
@markshannon markshannon changed the title Allow object value arrays and split key dictionaries larger than 16 bpo-46675: Allow object value arrays and split key dictionaries larger than 16 Feb 7, 2022
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 3cbdf70 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 7, 2022
Copy link
Member

@methane methane left a comment

Choose a reason for hiding this comment

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

LGTM, but please fix many warning from MSVC.

@markshannon markshannon merged commit 25db2b3 into python:main Feb 8, 2022
@@ -0,0 +1,2 @@
Allow more than 16 items in a split dict before it is combined. The limit is
now 254.
Copy link
Member

Choose a reason for hiding this comment

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

@markshannon markshannon deleted the larger-dict-values branch September 26, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants