On Mon, Nov 28, 2022 at 09:08:36PM -0500, Melanie Plageman wrote:
> > +pgstat_io_op_stats_collected(BackendType bktype)
> > +{
> > + return bktype != B_INVALID && bktype != B_ARCHIVER && bktype != B_LOGGER &&
> > + bktype != B_WAL_RECEIVER && bktype != B_WAL_WRITER;
> >
> > Similar: I'd prefer to see this as 5 "ifs" or a "switch" to return
> > false, else return true. But YMMV.
>
> I don't know that separating it into multiple if statements or a switch
> would make it more clear to me or help me with debugging here.
>
> Separately, since this is used in non-assert builds, I would like to
> ensure it is efficient. Do you know if a switch or if statements will
> be compiled to the exact same thing as this at useful optimization
> levels?
This doesn't seem like a detail worth much bother, but I did a test.
With -O2 (but not -O1 nor -Og) the assembly (gcc 9.4) is the same when
written like:
+ if (bktype == B_INVALID)
+ return false;
+ if (bktype == B_ARCHIVER)
+ return false;
+ if (bktype == B_LOGGER)
+ return false;
+ if (bktype == B_WAL_RECEIVER)
+ return false;
+ if (bktype == B_WAL_WRITER)
+ return false;
+
+ return true;
objdump --disassemble=pgstat_io_op_stats_collected src/backend/postgres_lib.a.p/utils_activity_pgstat_io_ops.c.o
0000000000000110 <pgstat_io_op_stats_collected>:
110: f3 0f 1e fa endbr64
114: b8 01 00 00 00 mov $0x1,%eax
119: 83 ff 0d cmp $0xd,%edi
11c: 77 10 ja 12e <pgstat_io_op_stats_collected+0x1e>
11e: b8 03 29 00 00 mov $0x2903,%eax
123: 89 f9 mov %edi,%ecx
125: 48 d3 e8 shr %cl,%rax
128: 48 f7 d0 not %rax
12b: 83 e0 01 and $0x1,%eax
12e: c3 retq
I was surprised, but the assembly is *not* the same when I used a switch{}.
I think it's fine to write however you want.
> > pgstat_count_io_op() has a superflous newline before "}".
>
> I couldn't find the one you are referencing.
> Do you mind pasting in the code?
+ case IOOP_WRITE:
+ pending_counters->writes++;
+ break;
+ }
+ --> here <--
+}
--
Justin
