search

Elemental-IRCd / elemental-ircd

Continuation of ShadowIRCD to meet people's needs
GNU General Public License v2.0
41 stars 19 forks source link

Remove malloc() implementation rb_bh_alloc #65

Closed Xe closed 9 years ago

Xe commented 9 years ago

rb_bh_alloc and friends are a reimplementation of malloc(). This should be removed in favor of just using malloc().

kaniini commented 9 years ago

Can you provide an actual backtrace of the segfault? While generally removing the blockheap might be a good idea with the fact that malloc() these days is a lot more efficient, most likely you have a legitimate problem in your code that is related to memory corruption.

aji commented 9 years ago

I'm looking at the rb_bh_* functions and this strikes me as a simple free list. It's possible malloc would be faster, but not likely, and free lists are a pretty difficult thing to screw up. I'd advise against removing it until an obvious flaw can be found.

That said, libratbox DOES have a malloc-alike rb_malloc, but it's a simple calloc wrapper that invokes a function when allocation fails. Seems like a prudent thing to have to me.

EDIT: while we're removing free lists, libmowgli has an implementation (mowgli_heap) that is used throughout libmowgli and atheme.

kaniini commented 9 years ago

Yeah, rb_malloc should stay, I think they are just talking about the blockheap. I think in general modern system malloc should outperform the blockheap. However, it may not be faster than Tor's memorypools. Whether or not the IRCd actually needs the fastest allocator is of course something that could be discussed...

aji commented 9 years ago

It looks like the block heap can be replaced with calls to rb_malloc by defining NOBALLOC at compile time. Maybe it would be better to change the build process to do that instead of ripping out rb_bh completely.

kaniini commented 9 years ago

That might be a good first step, there is some advantage to keeping the blockheap subsystem even if it just wraps malloc, for example, it provides memory usage accounting.

aji commented 9 years ago

it also reduces the number of changes to the code, which makes fixing this issue less error-prone

kaniini commented 9 years ago

Well, the question really becomes "is malloc really more efficient", right now you can build libratbox with --disable-balloc and test. Most likely you would want to measure CPU time consumed by having say, 100k local clients exercise the ircd's code paths.

Xe commented 9 years ago

Eh, it's not harmful. There's bigger fish to fry.