> No objections here.
V4 patch pushed. Thanks.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
On Fri, Jan 10, 2025 at 11:31:34AM +0900, Tatsuo Ishii wrote:
> If there's no objection, I am going to commit the patch.
No objections here.
--
nathan
> Looks generally reasonable to me.
Thanks for looking into the patch.
> +/*
> + * initStringInfoInternal
> + *
> + * Initialize a StringInfoData struct (with previously undefined contents)
> + * to describe an empty string.
> + * The initial memory allocation size is specified by 'initsize'.
> +
On Thu, Jan 09, 2025 at 03:21:41PM +0900, Tatsuo Ishii wrote:
> Ok, I have created v3 patch to do more inlining as you suggested. With
> the patch I confirmed that there's no call to functions except palloc
> in makeStringInfo, makeStringInfoExt, initStringInfo and
> initStringInfoExt in the asm co
> If it's not too much work to coax the compiler to do so, then I don't see a
> strong reason to avoid it. Granted, there are probably much more expensive
> parts of the StringInfo support functions (e.g., palloc()), but these are
> hot enough code paths that IMHO it's worth saving whatever cycles
On Tue, Jan 07, 2025 at 03:57:57PM +0900, Tatsuo Ishii wrote:
> So the v2 patch version is 1.3787% slower than master. Do you think we
> need further inlining?
If it's not too much work to coax the compiler to do so, then I don't see a
strong reason to avoid it. Granted, there are probably much m
Hi Nathan,
Thanks for looking into the patch.
>> 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.
>
> Wouldn't that mean that initStringInf
On Mon, Jan 06, 2025 at 10:57:23AM +0900, Tatsuo Ishii wrote:
>> 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 w
Hi David,
Thanks for the review.
> On Sun, 5 Jan 2025 at 21:03, Tatsuo Ishii 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
On Sun, 5 Jan 2025 at 21:03, Tatsuo Ishii 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 menti
> Hi Gurjeet,
>
> Thanks for looking into my patch.
>
>> On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii wrote:
>>>
>>> Attached is the patch for this.
>>
>> Hi Tatsuo,
>>
>> I believe the newline endings in your patches are causing the patch
>> application
>> to fail. I experienced this with you
Hi Gurjeet,
Thanks for looking into my patch.
> On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii wrote:
>>
>> Attached is the patch for this.
>
> Hi Tatsuo,
>
> I believe the newline endings in your patches are causing the patch
> application
> to fail. I experienced this with your initial patch,
On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii wrote:
>
> Attached is the patch for this.
Hi Tatsuo,
I believe the newline endings in your patches are causing the patch application
to fail. I experienced this with your initial patch, as well as with the latest
patch. Converting it to unix-style lin
> If you have trouble convincing people we need this for some new
> reason, then maybe you could review the existing callers to see if
> some of the existing call sites make it any more convincing.
>
> A very quick review, I found:
>
> send_message_to_frontend() initStringInfo(&buf) 1024 is proba
>> With opinions from Michael and David , what about following additional
>> APIs?
>>
>> #define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size
>> #*/
>> #define STRINGINFO_SMALL_SIZE 64 /* small initial allocation size */
>>
>> #define makeStringInfo makeStringInfoExtended(STRINGI
On Fri, Dec 27, 2024 at 9:46 PM Tatsuo Ishii wrote:
>
>
> With opinions from Michael and David , what about following additional APIs?
...
> #define makeStringInfo
> makeStringInfoExtended(STRINGINFO_DEFAULT_SIZE)
Minor nit: the definition of the macro should contain parentheses, like s
On 2024-12-28 Sa 12:45 AM, Tatsuo Ishii wrote:
Michael said:
New APIs are materials for HEAD, so recompilation needs to happen
anyway. Using a macro makes things slightly simpler and it would not
break unnecessarily the compilation of existing extensions.
Ok.
David said:
I didn't review th
Michael said:
> New APIs are materials for HEAD, so recompilation needs to happen
> anyway. Using a macro makes things slightly simpler and it would not
> break unnecessarily the compilation of existing extensions.
Ok.
David said:
> I didn't review the patch in detail, but I think "initsize" wou
On Thu, Dec 26, 2024 at 09:53:09AM +0900, Tatsuo Ishii wrote:
>> Saying that, having more control over the initial
>> size used for a StringInfo could provide better practices in some
>> cases. This reminds me of the ALLOCSET_SMALL_* fields in memutils.h,
>> hence an idea would be an initStringInf
On Wed, 25 Dec 2024 at 16:37, Tatsuo Ishii wrote:
> Currently the StringInfo package provides StringInfo object creation
> API with fixed length initial allocation size (1024 bytes). However,
> if we want to allocate much smaller size of initial allocation, this
> is waste of space.
I think it wo
> On Wed, Dec 25, 2024 at 12:37:04PM +0900, Tatsuo Ishii wrote:
>> Attached is a patch to implement it. In the patch I add two new APIs.
>>
>> extern StringInfo makeStringInfoWithSize(int size);
>> extern void initStringInfoWithSize(StringInfo str, int size);
>>
>> Maybe I could re-invent the whe
On Wed, Dec 25, 2024 at 12:37:04PM +0900, Tatsuo Ishii wrote:
> Attached is a patch to implement it. In the patch I add two new APIs.
>
> extern StringInfo makeStringInfoWithSize(int size);
> extern void initStringInfoWithSize(StringInfo str, int size);
>
> Maybe I could re-invent the wheel by co
Currently the StringInfo package provides StringInfo object creation
API with fixed length initial allocation size (1024 bytes). However,
if we want to allocate much smaller size of initial allocation, this
is waste of space.
Background: While working on this:
https://www.postgresql.org/message-id
23 matches
Mail list logo
