Attention is currently required from: neels, fixeria, pespin.
Patch set 4:Code-Review +1
1 comment:
File src/osmo-hnbgw/hnbgw.c:
Patch Set #2, Line 348: return talloc_asprintf(OTC_SELECT, "%u-%u-L%u-R%u-S%u-C%u", ucid->mcc, ucid->mnc, ucid->lac, ucid->rac,
> I hope this is not called everytime we log a line :) […]
I did a bit of synthetic benchmarking. In my benchmark (using fully asan enabled builds), I can run the static __thread buffer implementation 1294870 times per second on my laptop, while the OTC_SELECT variant runs "only" 808880 times per second.
I'm calling 5 prints for every osmo_select_main[_ctx]() iteration.
So there is a significant performance impact of 37%.
Of course, in a real-world use case there are more difficult to quantify implications such as potential talloc memory fragmentation, or probably more expensively the cache pollution by having to execute more code at different locations (talloc) and access more data structures (talloc data structures).
However, given the the overall unrealistic high execution count in the benchmark (we will rarely print more than 1000 per second and likely never hit a 10k/second in reality), I do agree that this 37% impact on the relatively small percentage of time we spend in printing overall is likely negligable.
I still don't get in general, why we should use a more expensive OTC_SELECT approach over a static buffer approach *without a clear requirement or reason*. It's not that it makes the code any easier to read or write.
So while I'm willing to unblock this change, I wouldn't want this to become standard practice all over the codebase, and I would certainly match any follow-up patch that converted it back to static buffers.
To view, visit change 33169. To unsubscribe, or for help writing mail filters, visit settings.