Skip to content

MiMalloc for sqllogictests - #20682

Closed
blaginin wants to merge 3 commits into
mainfrom
db/extended-allocator
Closed

blaginin wants to merge 3 commits into
mainfrom
db/extended-allocator

Conversation

@blaginin

@blaginin blaginin commented Mar 3, 2026 •

Copy link
Copy Markdown
Member

@blaginin blaginin self-assigned this Mar 3, 2026
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Mar 3, 2026
@blaginin
blaginin marked this pull request as ready for review March 3, 2026 21:39

@AdamGS AdamGS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, we've also found mimalloc making a huge difference on macos, which I assume is a very popular dev environment

@blaginin
blaginin requested a review from Jefffrey March 4, 2026 14:41
@blaginin
blaginin requested a review from comphead March 4, 2026 17:42
@github-actions github-actions Bot added the development-process Related to development process of DataFusion label Mar 4, 2026

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @blaginin however I'm not sure about mimalloc for tests tbh, we do mimalloc for benchmarks, but tests asserts correctness and mem usage for default user environment which is glibc malloc or musl malloc if we speaking for Linux and providing results from another mem allocator as a source of truth, WDYT?

@blaginin

blaginin commented Mar 4, 2026

Copy link
Copy Markdown
Member Author

hey @comphead! In this pr i change just sqlogictests, and they don't assert mem usage - just correctness. They're pretty high level so I'd be surprised if the allocator interferes in any way

@comphead

comphead commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

hey @comphead! In this pr i change just sqlogictests, and they don't assert mem usage - just correctness. They're pretty high level so I'd be surprised if the allocator interferes in any way

IMO I would still expect tests to be in the same environment, otherwise it would be a debugging nightmare in case of false positives or false negatives 🤔

@Dandandan

Copy link
Copy Markdown
Contributor

Thanks @blaginin however I'm not sure about mimalloc for tests tbh, we do mimalloc for benchmarks, but tests asserts correctness and mem usage for default user environment which is glibc malloc or musl malloc if we speaking for Linux and providing results from another mem allocator as a source of truth, WDYT?

I am not sure tests should depend on the allocator? If they would it might be possible to change the allocator just for those tests?

@Omega359

Omega359 commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Thanks @blaginin however I'm not sure about mimalloc for tests tbh, we do mimalloc for benchmarks, but tests asserts correctness and mem usage for default user environment which is glibc malloc or musl malloc if we speaking for Linux and providing results from another mem allocator as a source of truth, WDYT?

Do people actually use the default allocator for their df based apps? I've found in my app that MiMalloc gave the best overall behaviour and was significantly better than glibc.

@comphead

comphead commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Thanks @Omega359 and @Dandandan — the motivation mostly comes from DuckDB, which uses jemalloc by default.
In practice this means:

  • tests run with jemalloc
  • the shipped DuckDB distribution also uses jemalloc

Docs:
https://duckdb.org/docs/stable/guides/performance/environment#memory-allocator
https://duckdb.org/docs/stable/core_extensions/jemalloc

The advantage is that tests and the shipped binary run with the same allocator. If we think mimalloc works better (we also use it in Comet), it might make sense to make it the default allocator everywhere so tests and production match, also update documentation.

WDYT?

@AdamGS

AdamGS commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

what's production for DataFusion? Rust only allows you to set the global allocator once, setting it in a crate is a faux pas, and specifically here will block the use of the dhat allocator elsewhere

@Dandandan

Dandandan commented Mar 22, 2026 •

Copy link
Copy Markdown
Contributor

I think for tests it should be OK to switch to a different allocator.
They shouldn't fail when a different allocator would be used (we also expect now tests do not fail on user machines which use different system allocators).

You could always write tests that switch to a different allocator if necessary.

@github-actions

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the Stale PR has not had any activity for some time label May 22, 2026
@github-actions github-actions Bot closed this May 29, 2026
@alamb
alamb deleted the db/extended-allocator branch October 15, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of DataFusion sqllogictest SQL Logic Tests (.slt) Stale PR has not had any activity for some time

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants