Skip to content

Commit 1645d3a

Browse files
committed
Fix bug with reclaiming pages and refactor code.
1 parent 89d8cd6 commit 1645d3a

File tree

3 files changed

+35
-45
lines changed

3 files changed

+35
-45
lines changed

Objects/mimalloc/page.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,9 @@ void _mi_page_retire(mi_page_t* page) mi_attr_noexcept {
460460

461461
mi_page_set_has_aligned(page, false);
462462

463+
// any previous QSBR goals are no longer valid because we reused the page
464+
_PyMem_mi_page_clear_qsbr(page);
465+
463466
// don't retire too often..
464467
// (or we end up retiring and re-allocating most of the time)
465468
// NOTE: refine this more: we should not retire if this
@@ -496,6 +499,9 @@ void _mi_heap_collect_retired(mi_heap_t* heap, bool force) {
496499
if (mi_page_all_free(page)) {
497500
page->retire_expire--;
498501
if (force || page->retire_expire == 0) {
502+
#ifdef Py_GIL_DISABLED
503+
mi_assert_internal(page->qsbr_goal == 0);
504+
#endif
499505
_PyMem_mi_page_maybe_free(page, pq, force);
500506
}
501507
else {

Objects/mimalloc/segment.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1278,6 +1278,9 @@ static bool mi_segment_check_free(mi_segment_t* segment, size_t slices_needed, s
12781278
// if this page is all free now, free it without adding to any queues (yet)
12791279
mi_assert_internal(page->next == NULL && page->prev==NULL);
12801280
_mi_stat_decrease(&tld->stats->pages_abandoned, 1);
1281+
#ifdef Py_GIL_DISABLED
1282+
page->qsbr_goal = 0;
1283+
#endif
12811284
segment->abandoned--;
12821285
slice = mi_segment_page_clear(page, tld); // re-assign slice due to coalesce!
12831286
mi_assert_internal(!mi_slice_is_used(slice));
@@ -1350,13 +1353,16 @@ static mi_segment_t* mi_segment_reclaim(mi_segment_t* segment, mi_heap_t* heap,
13501353
_mi_page_free_collect(page, false); // ensure used count is up to date
13511354
if (mi_page_all_free(page) && _PyMem_mi_page_is_safe_to_free(page)) {
13521355
// if everything free by now, free the page
1356+
#ifdef Py_GIL_DISABLED
1357+
page->qsbr_goal = 0;
1358+
#endif
13531359
slice = mi_segment_page_clear(page, tld); // set slice again due to coalesceing
13541360
}
13551361
else {
13561362
// otherwise reclaim it into the heap
13571363
_mi_page_reclaim(target_heap, page);
13581364
if (requested_block_size == page->xblock_size && mi_page_has_any_available(page) &&
1359-
heap == target_heap) {
1365+
requested_block_size <= MI_MEDIUM_OBJ_SIZE_MAX && heap == target_heap) {
13601366
if (right_page_reclaimed != NULL) { *right_page_reclaimed = true; }
13611367
}
13621368
}

Objects/obmalloc.c

Lines changed: 22 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -104,65 +104,42 @@ _PyMem_mi_page_clear_qsbr(mi_page_t *page)
104104
#endif
105105
}
106106

107-
// Is the empty page safe to free? It's safe if there was a QSBR goal set and
108-
// the goal has been reached.
107+
// Check if an empty, newly reclaimed page is safe to free now.
109108
static bool
110109
_PyMem_mi_page_is_safe_to_free(mi_page_t *page)
111110
{
112111
assert(mi_page_all_free(page));
113112
#ifdef Py_GIL_DISABLED
114-
if (page->use_qsbr) {
115-
if (page->qsbr_goal == 0) {
116-
// No QSBR goal set, so we can't safely free the page yet.
117-
return false;
118-
}
113+
assert(page->qsbr_node.next == NULL);
114+
if (page->use_qsbr && page->qsbr_goal != 0) {
119115
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET();
120116
if (tstate == NULL) {
121117
return false;
122118
}
123-
if (!_Py_qbsr_goal_reached(tstate->qsbr, page->qsbr_goal)) {
124-
return false;
125-
}
126-
_PyMem_mi_page_clear_qsbr(page);
127-
return true;
119+
return _Py_qbsr_goal_reached(tstate->qsbr, page->qsbr_goal);
128120
}
129121
#endif
130122
return true;
131-
}
132-
133-
#ifdef Py_GIL_DISABLED
134-
// Enqueue a page to be freed later when it's safe to do so (using QSBR).
135-
// Note that we may still allocate from the page.
136-
static void
137-
enqueue_page_qsbr(mi_page_t *page, bool reclaimed)
138-
{
139-
assert(mi_page_all_free(page));
140-
assert(reclaimed || page->qsbr_goal == 0);
141-
142-
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)PyThreadState_GET();
143-
page->retire_expire = 0;
144-
145-
// The goal may be set if we are reclaiming an empty abandoned page
146-
if (page->qsbr_goal == 0) {
147-
page->qsbr_goal = _Py_qsbr_deferred_advance(tstate->qsbr);
148-
}
149123

150-
llist_insert_tail(&tstate->mimalloc.page_list, &page->qsbr_node);
151124
}
152-
#endif
153125

154126
static bool
155127
_PyMem_mi_page_maybe_free(mi_page_t *page, mi_page_queue_t *pq, bool force)
156128
{
157129
#ifdef Py_GIL_DISABLED
158-
if (!_PyMem_mi_page_is_safe_to_free(page)) {
159-
// The page may already be in the QSBR linked list if we allocated from
160-
// it after all blocks were freed.
161-
if (page->qsbr_node.next != NULL) {
162-
llist_remove(&page->qsbr_node);
163-
page->qsbr_goal = 0;
130+
assert(mi_page_all_free(page));
131+
if (page->use_qsbr) {
132+
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)PyThreadState_GET();
133+
if (page->qsbr_goal != 0 && _Py_qbsr_goal_reached(tstate->qsbr, page->qsbr_goal)) {
134+
_PyMem_mi_page_clear_qsbr(page);
135+
_mi_page_free(page, pq, force);
136+
return true;
164137
}
165-
enqueue_page_qsbr(page, false);
138+
139+
_PyMem_mi_page_clear_qsbr(page);
140+
page->retire_expire = 0;
141+
page->qsbr_goal = _Py_qsbr_deferred_advance(tstate->qsbr);
142+
llist_insert_tail(&tstate->mimalloc.page_list, &page->qsbr_node);
166143
return false;
167144
}
168145
#endif
@@ -177,7 +154,10 @@ _PyMem_mi_page_reclaimed(mi_page_t *page)
177154
assert(page->qsbr_node.next == NULL);
178155
if (page->qsbr_goal != 0) {
179156
if (mi_page_all_free(page)) {
180-
enqueue_page_qsbr(page, true);
157+
assert(page->qsbr_node.next == NULL);
158+
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)PyThreadState_GET();
159+
page->retire_expire = 0;
160+
llist_insert_tail(&tstate->mimalloc.page_list, &page->qsbr_node);
181161
}
182162
else {
183163
page->qsbr_goal = 0;
@@ -205,17 +185,15 @@ _PyMem_mi_heap_collect_qsbr(mi_heap_t *heap)
205185
mi_page_t *page = llist_data(node, mi_page_t, qsbr_node);
206186
if (!mi_page_all_free(page)) {
207187
// We allocated from this page some point after the delayed free
208-
page->qsbr_goal = 0;
209-
llist_remove(node);
188+
_PyMem_mi_page_clear_qsbr(page);
210189
continue;
211190
}
212191

213192
if (!_Py_qsbr_poll(tstate->qsbr, page->qsbr_goal)) {
214193
return;
215194
}
216195

217-
page->qsbr_goal = 0;
218-
llist_remove(node);
196+
_PyMem_mi_page_clear_qsbr(page);
219197
_mi_page_free(page, mi_page_queue_of(page), false);
220198
}
221199
#endif

0 commit comments

Comments
 (0)