Re: Proposal: add new API to stringinfo

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 initStringInfoExt() is not getting inlined?

Yes, but I thought additional inlining is not necessary if we only
care performance. But I haven't run benchmark yet. So this time I
tried it.

I created a small function:

Datum
stringinfo_test(PG_FUNCTION_ARGS)
{
StringInfo s;
int n;
int i;

n = PG_GETARG_INT32(0);

for (i = 0; i < n; i++)
{
s = makeStringInfo();
destroyStringInfo(s);
}
PG_RETURN_VOID();
}

and called the function with the iteration number 1000000 (1M) from
pgbench, repeating 30 seconds (if you are interested in the function,
please look into attached file). Then script given to pgbench is:

select stringinfo_test(1000000);

The result (average of 3 runs) are:

master: 76.993034 tps

with v2 patch: 75.945942 tps

So the v2 patch version is 1.3787% slower than master. Do you think we
need further inlining?

> I
> wonder if you need to create an inlined helper function that both the
> original and the new "ext" versions use to stop gcc from emitting these
> CALL instructions. For example:
>
> static inline void
> initStringInfoInternal(StringInfo str, int initsize)
> {
> ...
> }
>
> static inline StringInfo
> makeStringInfoInternal(int initsize)
> {
> StringInfo res = (StringInfo) palloc(sizeof(StringInfoData));
>
> initStringInfoInternal(res, initsize);
> return res;
> }
>
> StringInfo
> makeStringInfo(void)
> {
> return makeStringInfoInternal(STRINGINFO_DEFAULT_SIZE);
> }
>
> StringInfo
> makeStringInfoExt(int initsize)
> {
> return makeStringInfoInternal(initsize);
> }
>
> void
> initStringInfo(StringInfo str)
> {
> initStringInfoInternal(str, STRINGINFO_DEFAULT_SIZE);
> }
>
> void
> initStringInfoExt(StringInfo str, int initsize)
> {
> initStringInfoInternal(str, initsize);
> }
>
> That's admittedly quite verbose, but it ensures that all of these functions
> only use the inlined versions of each other. If that works, it might be
> worth doing something similar to resetStringInfo() (assuming it is not
> already getting inlined).

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp