Attention is currently required from: neels, fixeria, pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169 ) Change subject: fix umts_cell_id_name(): show CID ...................................................................... Patch Set 4: Code-Review+1 (1 comment) File src/osmo-hnbgw/hnbgw.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169/comment/a729128b_401e0744 PS2, 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 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 https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: Id903b8579ded8b908e644808e440d373bcca8da4 Gerrit-Change-Number: 33169 Gerrit-PatchSet: 4 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 14 Jun 2023 15:23:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment