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

iritkatriel · 2023-01-31T10:36:22Z

@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

markshannon

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

markshannon · 2023-01-31T10:53:48Z

Objects/longobject.c

+        result = (PyLongObject *)_PyInterpreterState_FreelistAlloc(interp, sizeof(PyLongObject));
+    }
+#else
+    if (size == 0) {


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)

Include/internal/pycore_interp.h

markshannon · 2023-01-31T11:00:55Z

Include/internal/pycore_interp.h

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


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.

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

Include/internal/pycore_pymem.h

markshannon · 2023-01-31T11:04:44Z

Objects/longobject.c

+static void
+int_dealloc(PyLongObject *op)
+{
+#if WITH_FREELISTS


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.

markshannon · 2023-01-31T11:06:28Z

Objects/obmalloc.c

+    }
+    uint32_t i = 0;
+    for (; i < list->space>>1; i++) {
+        void* ptr = PyObject_Malloc(list->size);


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

Objects/obmalloc.c

Python/pystate.c

iritkatriel · 2023-02-03T00:39:05Z

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 · 2023-02-03T10:28:38Z

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 · 2023-02-03T10:35:30Z

Include/internal/pycore_interp.h

@@ -230,6 +237,34 @@ PyAPI_FUNC(int) _PyInterpreterState_IDInitref(PyInterpreterState *);
 PyAPI_FUNC(int) _PyInterpreterState_IDIncref(PyInterpreterState *);
 PyAPI_FUNC(void) _PyInterpreterState_IDDecref(PyInterpreterState *);

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


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?

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

Include/internal/pycore_interp.h

markshannon · 2023-02-06T16:33:58Z

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

iritkatriel · 2023-02-06T16:37:20Z

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.

markshannon · 2023-02-06T16:47:25Z

Include/internal/pycore_interp.h

@@ -230,6 +235,27 @@ PyAPI_FUNC(int) _PyInterpreterState_IDInitref(PyInterpreterState *);
 PyAPI_FUNC(int) _PyInterpreterState_IDIncref(PyInterpreterState *);
 PyAPI_FUNC(void) _PyInterpreterState_IDDecref(PyInterpreterState *);

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


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.

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

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)

Include/internal/pycore_interp.h

iritkatriel · 2023-02-06T22:49:34Z

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

Include/internal/pycore_pymem.h

iritkatriel · 2023-02-08T18:56:27Z

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

iritkatriel · 2023-02-08T22:08:57Z

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 · 2023-02-17T14:16:34Z

Dropping this for now.

freelist for ints

e953c57

iritkatriel requested a review from markshannon January 31, 2023 10:36

bedevere-bot added the awaiting core review label Jan 31, 2023

iritkatriel marked this pull request as draft January 31, 2023 10:36

iritkatriel changed the title ~~freelist for ints~~ generic freelist, applied to ints Jan 31, 2023

markshannon reviewed Jan 31, 2023

View reviewed changes

iritkatriel added 3 commits January 31, 2023 15:36

moved things around a little

cf99ee4

remove WITH_FREELISTS in longobject.c

8a6e6a3

FreeToFull leaves the list empty

fe65f49

iritkatriel commented Jan 31, 2023

View reviewed changes

Python/pystate.c Outdated Show resolved Hide resolved

markshannon reviewed Feb 3, 2023

View reviewed changes

Include/internal/pycore_interp.h Outdated Show resolved Hide resolved

markshannon changed the title ~~generic freelist, applied to ints~~ GH-100240: Generic freelist, applied to ints Feb 3, 2023

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

reuse _PyFreeList_Clear

a33061f

fix free bug and remove ifdef in a few places

6b312e0

use _PyLong_Free in specialized bytecodes

1d81198

use _PyLong_Free in _Py_DECREF_INT

5609e30

stats for generic freelist

b8b1879

print new stats

460d12c

markshannon reviewed Feb 6, 2023

View reviewed changes

Include/internal/pycore_interp.h Outdated Show resolved Hide resolved

fix index mapping

2e2a861

iritkatriel commented Feb 7, 2023

View reviewed changes

Include/internal/pycore_pymem.h Show resolved Hide resolved

tweak the stats

42ee27f

iritkatriel added 2 commits February 8, 2023 18:25

shift instead of div

a9e76ad

Merge remote-tracking branch 'upstream/main' into int-freelist

1134727

iritkatriel added 2 commits February 8, 2023 19:17

int --> Py_ssize_t

78592b9

disable the freelist after clearing it in interpreter_clear

044743e

iritkatriel added 3 commits February 9, 2023 13:26

add _PyFreeList_Disable

827b81b

rename INDEX --> SIZE_CLASS

51c6c3c

stats per size class

cfb886d

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

Open

gh-126868: Add freelist for compact int objects #126865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iritkatriel commented Jan 31, 2023 •

edited by bedevere-bot

Loading

markshannon left a comment

markshannon Jan 31, 2023

markshannon Jan 31, 2023

iritkatriel Jan 31, 2023

markshannon Jan 31, 2023

markshannon Jan 31, 2023

iritkatriel commented Feb 3, 2023

markshannon commented Feb 3, 2023

markshannon Feb 3, 2023 •

edited

Loading

markshannon Feb 3, 2023

markshannon commented Feb 6, 2023

iritkatriel commented Feb 6, 2023

markshannon Feb 6, 2023

iritkatriel Feb 6, 2023

iritkatriel Feb 7, 2023

iritkatriel commented Feb 6, 2023

iritkatriel commented Feb 8, 2023

iritkatriel commented Feb 8, 2023

iritkatriel commented Feb 17, 2023

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

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

Conversation

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

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel commented Feb 3, 2023

markshannon commented Feb 3, 2023

markshannon Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Feb 6, 2023

iritkatriel commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel commented Feb 6, 2023

iritkatriel commented Feb 8, 2023

iritkatriel commented Feb 8, 2023

iritkatriel commented Feb 17, 2023

iritkatriel commented Jan 31, 2023 •

edited by bedevere-bot

Loading

markshannon Feb 3, 2023 •

edited

Loading