◐ Shell
reader mode source ↗
Skip to content

GH-100240: Generic freelist, applied to ints#101453

Closed
iritkatriel wants to merge 19 commits into
python:mainfrom
iritkatriel:int-freelist
Closed

GH-100240: Generic freelist, applied to ints#101453
iritkatriel wants to merge 19 commits into
python:mainfrom
iritkatriel:int-freelist

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Jan 31, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@markshannon This is basically the freelist from your branch, made a bit more generic, and applied to ints. We should think about which sizes we want to include.

(I'm trying to run benchmarks, but there is some issue with the machine at the moment).

@iritkatriel iritkatriel marked this pull request as draft January 31, 2023 10:36
@iritkatriel iritkatriel changed the title freelist for ints Jan 31, 2023
markshannon
markshannon reviewed Jan 31, 2023
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I know this is a draft, but I've taken the liberty of reviewing anyway 🙂

@iritkatriel

iritkatriel commented Feb 3, 2023

Copy link
Copy Markdown
Member Author

benchmarks are showing this as 2% slower overall. I guess we need to tune it.

https://github.com/faster-cpython/benchmarking/tree/main/results/bm-20230131-3.12.0a4%2B-fe65f49

@markshannon

markshannon commented Feb 3, 2023

Copy link
Copy Markdown
Member

It looks like this PR is allocating from the freelist, but freeing to the underlying allocator in the specialized BINARY_OP instructions.
Which could explain the large slowdown in spectralnorm.

@markshannon markshannon changed the title generic freelist, applied to ints Feb 3, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 3, 2023
Closed
@markshannon

markshannon commented Feb 6, 2023

Copy link
Copy Markdown
Member

I see you've added stats, have you recorded the stats yet?

@iritkatriel

iritkatriel commented Feb 6, 2023

Copy link
Copy Markdown
Member Author

I see you've added stats, have you recorded the stats yet?

Not yet. I have a bug I'm trying to figure out. It crashes in tests that use subprocesses.

@iritkatriel

iritkatriel commented Feb 6, 2023

Copy link
Copy Markdown
Member Author

I see you've added stats, have you recorded the stats yet?

https://github.com/faster-cpython/benchmarking/blob/main/results/bm-20230206-3.12.0a4%2B-2e2a861/bm-20230206-linux-x86_64-iritkatriel-int_freelist-3.12.0a4%2B-2e2a861-pystats.md#object-stats

@iritkatriel

iritkatriel commented Feb 8, 2023

Copy link
Copy Markdown
Member Author

test_embed is failing because of a leak (I'll check why). The other tests pass.

@iritkatriel

iritkatriel commented Feb 8, 2023

Copy link
Copy Markdown
Member Author

test_embed is failing because of a leak (I'll check why). The other tests pass.

The leak was because an int was freed and inserted to the freelist after it has been cleared in interpreter_clear. I set space and capacity to 0 in interpreter_clear to prevent this.

@iritkatriel

iritkatriel commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

Dropping this for now.

This was referenced Nov 11, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments

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

@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity ZeroIntensity will be requested when the pull request is marked ready for review ZeroIntensity is a code owner

@FFY00 FFY00 Awaiting requested review from FFY00 FFY00 will be requested when the pull request is marked ready for review FFY00 is a code owner

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iritkatriel @markshannon @bedevere-bot