> 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
22 matches
Mail list logo
