Re: [PATCH] Add function to_oct
От | Nathan Bossart |
---|---|
Тема | Re: [PATCH] Add function to_oct |
Дата | |
Msg-id | 20230815171715.GA2312399@nathanxps13 обсуждение исходный текст |
Ответ на | Re: [PATCH] Add function to_oct (John Naylor <john.naylor@enterprisedb.com>) |
Ответы |
Re: [PATCH] Add function to_oct
|
Список | pgsql-hackers |
On Tue, Aug 15, 2023 at 01:53:25PM +0700, John Naylor wrote: > If we're not going to have a general SQL conversion function, here are some > comments on the current patch. I appreciate the review. > +static char *convert_to_base(uint64 value, int base); > > Not needed if the definition is above the callers. Done. > + * Workhorse for to_binary, to_oct, and to_hex. Note that base must be > either > + * 2, 8, or 16. > > Why wouldn't it work with any base <= 16? You're right. I changed this in v5. > - *ptr = '\0'; > + Assert(base == 2 || base == 8 || base == 16); > > + *ptr = '\0'; > > Spurious whitespace change? I think this might just be a weird artifact of the diff algorithm. > - char buf[32]; /* bigger than needed, but reasonable */ > + char *buf = palloc(sizeof(uint64) * BITS_PER_BYTE + 1); > > Why is this no longer allocated on the stack? Maybe needs a comment about > the size calculation. It really should be. IIRC I wanted to avoid passing a pre-allocated buffer to convert_to_base(), but I don't remember why. I fixed this in v5. > +static char * > +convert_to_base(uint64 value, int base) > > On my machine this now requires a function call and a DIV instruction, even > though the patch claims not to support anything but a power of two. It's > tiny enough to declare it inline so the compiler can specialize for each > call site. This was on my list of things to check before committing. I assumed that it would be automatically inlined, but given your analysis, I went ahead and added the inline keyword. My compiler took the hint and inlined the function, and it used SHR instead of DIV instructions. The machine code for to_hex32/64 is still a couple of instructions longer than before (probably because of the uint64 casts), but I don't know if we need to worry about micro-optimizing these functions any further. > +{ oid => '5101', descr => 'convert int4 number to binary', > > Needs to be over 8000. Done. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Вложения
В списке pgsql-hackers по дате отправления: