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 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
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