we might start by picking some cpu-bound workload to measure the difference with ceph_asserts compiled out. encode/decode would probably be a good target, given all the assertions in bufferlist itself. that could give us some data to motivate further efforts here On Fri, Nov 17, 2023 at 2:00 PM Matt Benjamin <mbenjami@xxxxxxxxxx> wrote: > > This might seem in the weeds for a lot of folks, but the density of (currently always-expanded) ceph_assert() calls in fast paths seems really significant, to me. > > Matt > > On Mon, Oct 2, 2023 at 3:01 PM Casey Bodley <cbodley@xxxxxxxxxx> wrote: >> >> Matt recently raised the issue of ceph assertions in production code, >> which reminded me of Sage's 2016 pr >> https://github.com/ceph/ceph/pull/9969 that added a >> ceph_assert_always(). the idea was to eventually make ceph_assert() >> conditional on NDEBUG to match the behavior of libc's assert(), >> leaving ceph_assert_always() as the marked unconditional case >> >> i would love to see this finally happen, but there are some potential risks: >> >> * ceph_assert()s with side effects won't behave as expected in release >> builds. assert() documents this same issue at >> https://www.man7.org/linux/man-pages/man3/assert.3.html#BUGS. if we >> could at least identify these cases, we can switch them to >> ceph_assert_always() >> >> * in teuthology, we test the same release builds that we ship to >> users. that means teuthology won't catch the code paths that trigger >> debug assertions. if those lead to crashes, they could be much harder >> to debug without the assertions and backtraces >> >> * conversely, merging pull requests after a successful teuthology run >> may introduce new assertions in debug builds. it could be annoying for >> developers to track down and fix new assertions after pulling the >> latest main or stable release branch >> >> * unused variable warnings in release builds where ceph_assert() was >> the only reference. at least the compiler will catch all of these for >> us, and [[maybe_unused]] annotations can clear them up >> >> >> in general, do folks agree that this is a change worth making? if so, >> what can we do to mitigate the risks? >> >> if not, how should we handle the use of ceph_assert() vs raw assert() >> in new code? should there be some guidance in CodingStyle? >> >> as a half-measure, we might introduce a new ceph_assert_debug() as an >> alternative to raw assert(), then convert some existing uses of >> ceph_assert() on a case-by-case basis >> > > > -- > > Matt Benjamin > Red Hat, Inc. > 315 West Huron Street, Suite 140A > Ann Arbor, Michigan 48103 > > http://www.redhat.com/en/technologies/storage > > tel. 734-821-5101 > fax. 734-769-8938 > cel. 734-216-5309 _______________________________________________ Dev mailing list -- dev@xxxxxxx To unsubscribe send an email to dev-leave@xxxxxxx
