GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-100240: Generic freelist, applied to ints by iritkatriel · Pull Request #101453 · python/cpython

Conversation

@iritkatriel

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

Uh oh!

There was an error while loading. Please reload this page.

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).

  • Issue: Use a principled, and consistent, implementation of freelists. #100240

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iritkatriel
freelist for ints
e953c57
@iritkatriel
iritkatriel requested a review from markshannon January 31, 2023 10:36
@bedevere-bot bedevere-bot added the awaiting core review label Jan 31, 2023
@iritkatriel
iritkatriel marked this pull request as draft January 31, 2023 10:36
@iritkatriel iritkatriel changed the title freelist for ints generic freelist, applied to ints Jan 31, 2023
markshannon
markshannon reviewed Jan 31, 2023
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/longobject.c Outdated
result = (PyLongObject *)_PyInterpreterState_FreelistAlloc(interp, sizeof(PyLongObject));
}
#else
if (size == 0) {

@markshannon markshannon Jan 31, 2023

Copy link
Copy Markdown
Member

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.

Don't change it for this PR, but this shouldn't be needed, as there should only be one zero. I think this indicates a bug in the caller.
The above assert, assert(size >= 0) should then be assert(size > 0)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_interp.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_interp.h Outdated
}

static inline void
_PyInterpreterState_FreelistFree(PyInterpreterState * interp, PyObject *op, int size) {

@markshannon markshannon Jan 31, 2023

Copy link
Copy Markdown
Member

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.

What is size?. The assumption is that it is size in bytes, but I think you are using size in machine words.
Also needs to be Py_ssize_t.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Jan 31, 2023

Copy link
Copy Markdown
Member Author

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.

it's the results of sizeof(PyLongObject), that should be bytes no?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_pymem.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/longobject.c Outdated
static void
int_dealloc(PyLongObject *op)
{
#if WITH_FREELISTS

@markshannon markshannon Jan 31, 2023

Copy link
Copy Markdown
Member

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.

Should we move the #if WITH_FREELISTS into PyInterpreterState_FreelistFree to keep it localized?

Personally, I think we should just remove WITH_FREELISTS, but that needs a wider discussion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/obmalloc.c
}
uint32_t i = 0;
for (; i < list->space>>1; i++) {
void* ptr = PyObject_Malloc(list->size);

@markshannon markshannon Jan 31, 2023

Copy link
Copy Markdown
Member

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.

Note for future PR: we should add the bulk allocate/free capability to the allocator.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/obmalloc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/obmalloc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 3 commits January 31, 2023 15:36
@iritkatriel
moved things around a little
cf99ee4
@iritkatriel
remove WITH_FREELISTS in longobject.c
8a6e6a3
@iritkatriel
FreeToFull leaves the list empty
fe65f49
iritkatriel
iritkatriel commented Jan 31, 2023
View reviewed changes
Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Feb 3, 2023
View reviewed changes
Comment thread Include/internal/pycore_interp.h Outdated
PyAPI_FUNC(int) _PyInterpreterState_IDIncref(PyInterpreterState *);
PyAPI_FUNC(void) _PyInterpreterState_IDDecref(PyInterpreterState *);

#define SIZE_TO_FREELIST_INDEX(size) ((size-4)/2)

@markshannon markshannon Feb 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Why divide by 2 and not 2 * sizeof(void *), which is the quantum of allocation for malloc and ob_malloc?
(Assuming that the size is in bytes)
Could you use the term "size class" or equivalent, instead of "freelist index" The mapping here is from sizes to classes (not all size classes get free lists).

Any reason not to make this an inline function?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@markshannon markshannon Feb 3, 2023

Copy link
Copy Markdown
Member

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.

Alternatively, put an assert in this function, and put a size check in the caller.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
markshannon
markshannon reviewed Feb 3, 2023
View reviewed changes
Comment thread Include/internal/pycore_interp.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@markshannon markshannon changed the title generic freelist, applied to ints GH-100240: Generic freelist, applied to ints Feb 3, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 3, 2023
Use a principled, and consistent, implementation of freelists. #100240
Closed
iritkatriel added 6 commits February 3, 2023 14:31
@iritkatriel
reuse _PyFreeList_Clear
a33061f
@iritkatriel
fix free bug and remove ifdef in a few places
6b312e0
@iritkatriel
use _PyLong_Free in specialized bytecodes
1d81198
@iritkatriel
use _PyLong_Free in _Py_DECREF_INT
5609e30
@iritkatriel
stats for generic freelist
b8b1879
@iritkatriel
print new stats
460d12c
@markshannon

markshannon commented Feb 6, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Feb 6, 2023
View reviewed changes
Comment thread Include/internal/pycore_interp.h Outdated
PyAPI_FUNC(void) _PyInterpreterState_IDDecref(PyInterpreterState *);

#define FREELIST_QUANTUM (2*sizeof(void*))
#define SIZE_TO_FREELIST_INDEX(size) ((size-4)/FREELIST_QUANTUM)

@markshannon markshannon Feb 6, 2023

Copy link
Copy Markdown
Member

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.

Why this formula?

You want to allocate all sizes from (n-1)*FREELIST_QUANTUM + 1 to n*FREELIST_QUANTUM (inclusive) from the same freelist.
I would expect the formula to be (size + FREELIST_QUANTUM -1)/FREELIST_QUANTUM, or (size-1)/FREELIST_QUANTUM.

And because C division rounds towards 0, not -infinity, you want to use >>LOG_BASE_2_OF_FREELIST_QUANTUM, not /FREELIST_QUANTUM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Feb 6, 2023

Copy link
Copy Markdown
Member Author

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.

I haven't changed this yet. I'm trying to get it to work with just ints.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Feb 7, 2023

Copy link
Copy Markdown
Member Author

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.

There already are some macros in pycore_obmalloc.h that seem to do something like this:

/* 
 * Alignment of addresses returned to the user. 8-bytes alignment works
 * on most current architectures (with 32-bit or 64-bit address buses).
 * The alignment value is also used for grouping small requests in size
 * classes spaced ALIGNMENT bytes apart.
 *
 * You shouldn't change this unless you know what you are doing.
 */

#if SIZEOF_VOID_P > 4
#define ALIGNMENT              16               /* must be 2^N */
#define ALIGNMENT_SHIFT         4
#else
#define ALIGNMENT               8               /* must be 2^N */
#define ALIGNMENT_SHIFT         3
#endif
   
/* Return the number of bytes in size class I, as a uint. */
#define INDEX2SIZE(I) (((pymem_uint)(I) + 1) << ALIGNMENT_SHIFT)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
markshannon
markshannon reviewed Feb 6, 2023
View reviewed changes
Comment thread Include/internal/pycore_interp.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
fix index mapping
2e2a861
@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel commented Feb 7, 2023
View reviewed changes
Comment thread Include/internal/pycore_pymem.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
tweak the stats
42ee27f
iritkatriel added 2 commits February 8, 2023 18:25
@iritkatriel
shift instead of div
a9e76ad
@iritkatriel
Merge remote-tracking branch 'upstream/main' into int-freelist
1134727
@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 2 commits February 8, 2023 19:17
@iritkatriel
int --> Py_ssize_t
78592b9
@iritkatriel
disable the freelist after clearing it in interpreter_clear
044743e
@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 3 commits February 9, 2023 13:26
@iritkatriel
add _PyFreeList_Disable
827b81b
@iritkatriel
rename INDEX --> SIZE_CLASS
51c6c3c
@iritkatriel
stats per size class
cfb886d
@iritkatriel

iritkatriel commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

Dropping this for now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel iritkatriel closed this Feb 17, 2023
This was referenced Nov 11, 2024
Use freelist for range object, iterator objects and other often used objects #126703
Closed
gh-126868: Add freelist for compact int objects #126865
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 will be requested when the pull request is marked ready for review 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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@iritkatriel @markshannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.