Hi David, Thanks for the review.
> On Sun, 5 Jan 2025 at 21:03, Tatsuo Ishii <is...@postgresql.org> wrote:
>> Attached is the v2 patch to reflect above.
>
> A quick review.
>
> I don't see any users of STRINGINFO_SMALL_SIZE, so I question the
> value in defining it.
>
> I think it might be worth adding a comment to initStringInfoExt and
> makeStringInfoExt which mentions the valid ranges of initsize. 1 to
> MaxAllocSize by the looks of it. The reason that it's good to document
> this is that if we ever get any bug reports where some code is passing
> something like 0 as the size, we'll know right away that it's the
> caller's fault rather than the callee.
Agreed.
> I'm also curious if this change has much of a meaningful impact in the
> size of the compiled binary. The macro idea isn't quite how I'd have
> done it as it does mean passing an extra parameter for every current
> callsite. If I were doing this, I'd just have created the new
> functions in stringinfo.c and have the current functions call the new
> external function and rely on the compiler inlining, basically, that's
> the same as what makeStringInfo() does today with initStringInfo().
>
> I'd have done it like:
>
> StringInfo
> makeStringInfo(void)
> {
> return makeStringInfoExt(STRINGINFO_DEFAULT_SIZE);
> }
>
> Using current master, you can see from the following that
> initStringInfo is inlined, despite being an external function. I've no
> reason to doubt the compiler won't do the same for makeStringInfo()
> and makeStringInfoExt().
>
> $ gcc src/common/stringinfo.c -I src/include -O2 -S | cat stringinfo.s
> | grep makeStringInfo -A 10
[snip]
> Note, there is no "call initStringInfo"
I have tried with this direction. See attached v2 patch. In the asm
code I don't see "call makeStringInfoExt" as expected. But I see
"call initStringInfoExt". I assume this is an expected behavior.
$ gcc src/common/stringinfo.c -I src/include -O2 -S; cat stringinfo.s|grep
makeStringInfo -A 20
.globl makeStringInfoExt
.type makeStringInfoExt, @function
makeStringInfoExt:
.LFB99:
.cfi_startproc
endbr64
pushq %r12
.cfi_def_cfa_offset 16
.cfi_offset 12, -16
pushq %rbp
.cfi_def_cfa_offset 24
.cfi_offset 6, -24
subq $8, %rsp
.cfi_def_cfa_offset 32
testl %edi, %edi
jle .L11
movl %edi, %ebp
movl $24, %edi
call palloc@PLT
movl %ebp, %esi
movq %rax, %rdi
movq %rax, %r12
call initStringInfoExt
--
.size makeStringInfoExt, .-makeStringInfoExt
.p2align 4
.globl makeStringInfo
.type makeStringInfo, @function
makeStringInfo:
.LFB98:
.cfi_startproc
endbr64
pushq %r12
.cfi_def_cfa_offset 16
.cfi_offset 12, -16
movl $24, %edi
call palloc@PLT
movl $1024, %esi
movq %rax, %r12
movq %rax, %rdi
call initStringInfoExt
movq %r12, %rax
popq %r12
.cfi_def_cfa_offset 8
ret
.cfi_endproc
.LFE98:
.size makeStringInfo, .-makeStringInfo
.section .rodata.str1.1
.LC2:
.string "str->maxlen != 0"
.text
.p2align 4
.globl resetStringInfo
.type resetStringInfo, @function
resetStringInfo:
.LFB102:
.cfi_startproc
endbr64
movl 12(%rdi), %edx
testl %edx, %edx
je .L19
movq (%rdi), %rax
movb $0, (%rax)
movl $0, 8(%rdi)
movl $0, 16(%rdi)
ret
.L19:
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
v2-0001-Add-new-StringInfo-APIs-to-allow-callers-to-speci.patch
Description: Binary data
