Skip to content
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

gc: improve mallocarrays locality #56801

Merged
merged 1 commit into from
Dec 13, 2024
Merged

gc: improve mallocarrays locality #56801

merged 1 commit into from
Dec 13, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Dec 11, 2024

small_arraylist_t has much better memory locality and space utilization than a linked list with individually malloc'd elements

@vtjnash vtjnash added the GC Garbage collector label Dec 11, 2024
@vtjnash vtjnash requested a review from gbaraldi December 11, 2024 14:19
src/gc-stock.c Outdated Show resolved Hide resolved
@vtjnash vtjnash force-pushed the jn/mallocarrays-list branch from 8afa0e6 to 725adba Compare December 11, 2024 20:17
@gbaraldi
Copy link
Member

This seems to fix #56759 for me. So we probably want to backport this

@gbaraldi gbaraldi added the backport 1.11 Change should be backported to release-1.11 label Dec 11, 2024
@d-netto
Copy link
Member

d-netto commented Dec 11, 2024

This seems to fix #56759 for me. So we probably want to backport this

IIRC we're still using this data structure in 1.10.

If this is indeed a source of a memory leak, then we should consider backporting it to 1.10 as well.

@vtjnash vtjnash force-pushed the jn/mallocarrays-list branch 3 times, most recently from ed028da to b02629a Compare December 12, 2024 21:30
small_arraylist_t has much better memory locality and space utilization
than a linked list with individually malloc'd elements. However, seemed
that it needed alignment of small_arraylist_t to both hold all elements
the user might create and avoid gcc bugs.
@vtjnash vtjnash force-pushed the jn/mallocarrays-list branch from b02629a to 6e2c0cb Compare December 13, 2024 00:52
@MilesCranmer
Copy link
Member

Can confirm this fixes #56759. Nice work!

@vtjnash vtjnash merged commit 03a0247 into master Dec 13, 2024
5 of 7 checks passed
@vtjnash vtjnash deleted the jn/mallocarrays-list branch December 13, 2024 02:54
@vchuravy
Copy link
Member

Do we know why it fixes the memory leak? Were we simply not freeing the elements of the linked list?

@oscardssmith
Copy link
Member

yes

@vtjnash
Copy link
Member Author

vtjnash commented Dec 14, 2024

No idea

@MilesCranmer
Copy link
Member

Could this also be backported to 1.10?

@oscardssmith oscardssmith added the backport 1.10 Change should be backported to the 1.10 release label Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants