search

lnx-search / lnx

⚡ Insanely fast, 🌟 Feature-rich searching. lnx is the adaptable, typo tollerant deployment of the tantivy search engine.
https://lnx.rs
MIT License
1.25k stars 46 forks source link

lnx-server: use mimalloc only on msvc. #92

Closed michaelgrigoryan25 closed 2 years ago

michaelgrigoryan25 commented 2 years ago

This PR adds a compiler attribute into lnx-server/main.rs to only use mimalloc in MSVC environments.

ChillFish8 commented 2 years ago

What's the motivation from removing this from non-msvc environments?

michaelgrigoryan25 commented 2 years ago

It may probably impact the performance for the worse. For Linux, I think the default system allocator is good enough. Even if it isn't, there are always allocators like jemalloc. Plus, jemalloc is optimized for (multithreaded) speed, not memory usage.

ChillFish8 commented 2 years ago

Although this was a while ago, I do remember the allocator switch improved the performance across the board including the docker containers especially (this is possibly slightly different now as some of the docker images have changed)

And yes there is Jemalloc although something in lnx (either a dependency or some implementation) causes a segfault at runtime which we obviously dont want to run into 😅 What's interesting is this only happens with Jemalloc.

michaelgrigoryan25 commented 2 years ago

I see. I'll close this PR then.

ChillFish8 commented 2 years ago

I think i'd probably want to do some performance checks regarding this change before merging it. If it doesn't have any effect or improve it on the debian images then, I'm reasonably happy to merge this. If it negatively impacts it I'm probably going to stick with it, so at least that way it's consistent performance across operating systems.

ChillFish8 commented 2 years ago

Okay, sorry for the long delay!

I've run through some benchmarks comparing it against MiMalloc, and overall MiMalloc is considerably faster ~10-20% performance loss when switching back to the default allocator as a best case.

Because of this, I'm going to reject this change as I don't think it's worth it. (MiMalloc is a pretty insignificant dependency to include but has a big affect)