danielaparker / jsoncons

A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSON Schema, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON
https://danielaparker.github.io/jsoncons
Other
697 stars 160 forks source link

avoid over-allocatng memory due to alignment constraints #509

Closed romange closed 3 months ago

romange commented 3 months ago

in heap_string_factory::create we over-allocate memory to be able to align the pointer withing the allocated block.

However, the default, usual case is that we allocate strings with alignment 8 (= alignof(storage_type). But all the standard allocators available today in any os already return 8 byte-aligned pointers, so overallocating by 7 bytes is unnecessary. One can think that 7 bytes is not significant but it could push the underlying allocator to select the next size class for the allocation, effectively reserving dozens of bytes more.

To summarize: standard allocators already return 8 bytes aligned pointers, so we pay penalty for supporting the theoretic edge cases that return non-aligned adresses.

My suggestion is that for alignments <= 8 we wont allocate exacly the requested size and check if the returned address is aligned. And if it hits the unlikely scenario of not being aligned, then deallocate and retry again with the current approach.

danielaparker commented 3 months ago

Thanks for the suggestion. It seems the pmr allocators don't return maximally aligned pointers, which is where the issue first arose, see (https://github.com/danielaparker/jsoncons/issues/441), and the reason we added the alignment code. I agree that for most of our users it's wasted space.

romange commented 3 months ago

Yes, indeed for alignments > 8, the original code did not work, as @chakaz pointed out. Would you mind if I send the fix that solves it for both edge cases?

danielaparker commented 3 months ago

@romange Feel free to submit a PR