feat: Custom OStream replacement

[bnbajwa]

SDB-8506

EDIT: Have run Log.bm.cpp and Net.bm.cpp benchmarks

Improvements ranging from 10% to 252%

BEFORE:

------------------------------------------------------------------------------------------------------------------------
sync_null_logger                                   426146200     0.055     0.069     0.072     0.093     0.121     0.154
unformatted_null_logger                            552838000     0.048     0.054     0.056     0.063     0.086     0.104
async_logger                                         1417900     0.055     0.086      0.14     0.166     0.193     2.101
formatted_null_logger                              453439000      0.05     0.065     0.068     0.072      0.11     0.139
sync_file_logger                                   131022550     0.214     0.227     0.234     0.275     0.382     0.474
single_thread_async_null_logger                          600     0.279     1.502     1.616     1.686     1.686     1.686

------------------------------------------------------------------------------------------------------------------------
format_ipv4_endpoint                              1724005376     0.013     0.017     0.017      0.02     0.029     0.037
format_ipv4_endpoint_libc                          249794560     0.105     0.119     0.124     0.128     0.205     0.275

AFTER:

------------------------------------------------------------------------------------------------------------------------
sync_null_logger                                   677027000     0.037     0.043     0.046     0.058     0.077     0.094
unformatted_null_logger                            632185000     0.044     0.047     0.049     0.052     0.073     0.087
async_logger                                         1420650     0.039      0.04     0.077      0.09     0.199      1.63
formatted_null_logger                              514780000     0.046     0.058      0.06     0.066     0.096     0.112
sync_file_logger                                   147351350     0.188     0.201     0.208     0.249     0.319     0.373
single_thread_async_null_logger                          600     0.216     0.713     0.811     0.845     0.845     0.845

------------------------------------------------------------------------------------------------------------------------
format_ipv4_endpoint                              4247213056     0.006     0.006     0.006     0.006     0.006     0.006
format_ipv4_endpoint_libc                          287428608     0.081     0.103     0.108      0.11     0.177     0.249

[steverough]

Whilst I realise this is probably a hangover from the previous impl, why is there a need for a nullptr storage_ ? If we could avoid supporting this we wouldn't need to check the storage_ ptr for nullptr-ness.

[bnbajwa]

Yes, hangover from prev impl.

If we could avoid supporting this we wouldn't need to check the storage_ ptr for nullptr-ness.

Yes, good point. I will try to address this in a follow-up PR. Existing code may rely on this behaviour, so will have to investigate.

[steverough]

asserting prepare_space() always returns non-nullptr means all implementations of do_prepare_space() will adhere to this contract. Do they?

[steverough]

... this is not clear to me. Sometimes you assert() non-nullptr-ness, sometime you check as if it's not a given that non-nullptrs are returned.

[bnbajwa]

asserting prepare_space() always returns non-nullptr means all implementations of do_prepare_space() will adhere to this contract. Do they?

No, some implementations have buffers of fixed size which can be fully used up -- in which case they return nullptr (e.g. OStaticStream uses a stack allocated buffer of N bytes)
The assert here is mainly to catch accidental small buffer sizes. Some lpfixgw codecs use ostreams with a fixed buffer size. If that buffer size was too small, it should ideally be caught before it reaches prod.

But yes, it is not part of the contract, implementations are allowed to return nullptr, so strictly speaking the assert should not be there.

[steverough]

I would suggest removing the assert as it's confusing. The assert is only applicable for certain Derived and not all yet this is a base class implementation and must form a contract which is applicable to all.

[bnbajwa]

Fixed

[Shivix]

Why must these be excluded?

[bnbajwa]

I've excluded them because they are also excluded for std::ostream -- and so for consistency I did the same.

[Shivix]

Having these functions declared out of the class was quite jarring for me. Could just be that I'm not used to it, but definitely had to think twice about it.

What do other readers think about it? Is it worth it to get the clean class declaration?