|
|
|
|
|
by musjleman
1570 days ago
|
|
I'm going to be skeptical on this. Shoving refcounting + SBO + non owning views into a single type is not a smart decision. Before going into why I think so, some general implementation things to look into: * Power of 2 exponent is not optimal. You will never be able to reallocate into the same stretch of memory that the arrays have previously occupied, as the sum of their sizes will always be smaller than the next one. C++ stdlibs have switched to values closer to 1.5.
* Extra care should be taken when using memcpy. It's possible to copy overlapping buffers which is not allowed. using memmove instead is a simple solution. Also note the case when appending the Buffet structure itself to its array.
* You can't yet count on all machines having ctlz/lzcnt instruction. There have been quite a few cases of this instruction crashing software because no fallback was provided lately.
* If someone asks for bft_new(bft, 0x1000) you'll end up allocating 0x2000 bytes even though an exact size was specified. IMO scaling should be done only on appends.
* If you force the structure alignment to 16 you might as well say that your type is 24 bytes in size, because you'll very likely be adding tons of padding to structures that include your buffer.
* Seems like quite a few checks for malloc failure are missing + proper reporting for that.
Now why I think this is a terrible idea: * The model of your reference counting is a minefield and will 100% lead to problems when used in real world.
- Free'ing OWN before all refs = leak (although this could be fixed by giving OWN refcount of 1 instead of 0)
- Reference count is non-atomic = potential corruption
- bft_view() has differing behaviors of output depending on what state it's in = hard to reproduce bugs.
* You won't be fast just because you fit into 16 bytes.
- Your code has branches on every single hot path that is virtually free on other implementations. There isn't really even anything you can optimize as all the branches essentially have equal weight, so you'll be having mispredictions by design.
- You have multiple levels of indirection for data pointers so you'll be stalling the pipeline on cache misses and dependent loads as well as bad speculation.
* SBO - OK, refcount - OK, but why add non-owning view into the equation? What's wrong with good old pointer+size or pointer start+pointer end for that. There already are enough footguns to worry about with 3 states.
|
|
Thank you a lot! I'm humbled by the attention you took. Please bear in mind I never intended to see this project publicized so early on the HN stage. I'm still experimenting in C, and it shows...
That's a lot to chew on. For the moment :
> Free'ing OWN before all refs = leak
More like UAF ? Also, you can't. That was the idea. But it seems a bad one, according to you and user rom1v.
> Reference count is non-atomic = potential corruption
I'll try to make it so. This aspect is new to me. Anyway I hear it's quite hard, beyond that, to have a thread-safe lib, so I'll advertize it as non-TS.
> branches
Maybe that's the price for compacting str+str_view into a single type ?
> enough footguns to worry about with 3 states.
I agree the VUE should maybe drop.