Date: Mon, 13 Mar 2023 14:40:27 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Ankit Kumar Pandey <itsankitkp@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: optimize several list functions with SIMD intrinsics
Message-ID: <20230313214027.GB423713@nathanxps13>
References: <20230308002502.GA3378449@nathanxps13>
 <20230308185809.GA3552675@nathanxps13>
 <167852767890.628976.10354523915450248589.pgcf@coridan.postgresql.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <167852767890.628976.10354523915450248589.pgcf@coridan.postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/20230313214027.GB423713%40nathanxps13>
Precedence: bulk

Thanks for taking a look.

On Sat, Mar 11, 2023 at 09:41:18AM +0000, Ankit Kumar Pandey wrote:
> 1. In list_member_ptr, will it be okay to bring `const ListCell *cell` from 
> #ifdef USE_NO_SIMD
> 	const ListCell *cell;
> #endif
> to #else like as mentioned below? This will make visual separation between #if cases more cleaner

I would expect to see -Wdeclaration-after-statement warnings if we did
this.

> 2. Lots of duplicated
> if (list == NIL) checks before calling  list_member_inline_internal(list, datum)
> Can we not add this check in list_member_inline_internal itself?

We probably could.  I only extracted the NIL checks to simplify the code in
list_member_inline_internal() a bit.

> 3. if (cell)
> 		return list_delete_cell(list, cell) in list_delete_int/oid
> can we change if (cell) to if (cell != NULL) as used elsewhere?

Sure.

> 4. list_member_inline_interal_idx , there is typo in name.

Will fix.

> 5. list_member_inline_interal_idx and list_member_inline_internal are practically same with almost 90+ % duplication.
> can we not refactor both into one and allow cell or true/false as return? Although I see usage of const ListCell so this might be problematic.

The idea was to skip finding the exact match if all we care about is
whether the element exists.  This micro-optimization might be negligible,
in which case we could use list_member_inline_internal_idx() for both
cases.

> 6. Loop for (i = 0; i < tail_idx; i += nelem_per_iteration) for Vector32 in list.c looks duplicated from pg_lfind32 (in pg_lfind.h), can we not reuse that?

The list.c version is slightly different because we need to disregard any
matches that we find in between the data.  For example, in an integer List,
the integer will take up 4 bytes of the 8-byte ListCell (for 64-bit
platforms).

	typedef union ListCell
	{
		void	   *ptr_value;
		int			int_value;
		Oid			oid_value;
		TransactionId xid_value;
	} ListCell;

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com