On Sat, 18 Jun 2022 at 08:06, Andres Freund <and...@anarazel.de> wrote: > I also attached my heavily-WIP patches for the ExprEvalStep issues, I > accidentally had only included a small part of the contents of the json fix.
I've now looked at the 0003 patch. I like the idea you have about moving some of the additional fields into ScalarArrayOpExprHashTable. I think the patch can even go a little further and move the hash_finfo into there too. This means we don't need to dereference the "op" in saop_element_hash(). To make this work, I did need to tag the ScalarArrayOpExpr into the ExprEvalStep. That's required now since some of the initialization of the hash function fields is delayed until ExecEvalHashedScalarArrayOp(). We need to know the ScalarArrayOpExpr's hashfuncid and inputcollid. Your v2 patch did shift off some of this initialization work to ExecEvalHashedScalarArrayOp(). The attached v3 takes that a bit further. This saves a bit more work for ScalarArrayOpExprs that are evaluated 0 times. Another small thing which I considered doing was to put the hash_fcinfo_data field as the final field in ScalarArrayOpExprHashTable so that we could allocate the memory for the hash_fcinfo_data in the same allocation as the ScalarArrayOpExprHashTable. This would reduce the pointer dereferencing done in saop_element_hash() a bit further. I just didn't notice anywhere else where we do that for FunctionCallInfo, so I resisted doing this. (There was also a small bug in your patch where you mistakenly cast to an OpExpr instead of ScalarArrayOpExpr when you were fetching the inputcollid) David
From 7a1e71a6b622eff72537ae86187b312d01a5e11d Mon Sep 17 00:00:00 2001 From: David Rowley <drow...@postgresql.org> Date: Wed, 29 Jun 2022 10:43:41 +1200 Subject: [PATCH v3] Remove size increase in ExprEvalStep caused by hashed saops 50e17ad28 increased the size of ExprEvalStep from 64 bytes up to 88 bytes. Lots of effort was spent during the development of the current expression evaluation code to make an instance of this struct fit on a single cache line. Here we remove the fn_addr field. The values from this field can be obtained via fcinfo, just with some extra pointer dereferencing. The extra indirection does not seem to cause any noticeable slowdowns. Various other fields have been been moved into the ScalarArrayOpExprHashTable struct. These fields are only used when the ScalarArrayOpExprHashTable pointer has already been dereferenced, so no additional pointer dereferences occur for these. Author: Andres Freund Discussion: https://postgr.es/m/20220616233130.rparivafipt6d...@alap3.anarazel.de --- src/backend/executor/execExpr.c | 19 +------------------ src/backend/executor/execExprInterp.c | 25 +++++++++++++++++++++---- src/include/executor/execExpr.h | 7 +------ 3 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 2831e7978b..2e75f3204d 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -1203,8 +1203,6 @@ ExecInitExprRec(Expr *node, ExprState *state, FmgrInfo *finfo; FunctionCallInfo fcinfo; AclResult aclresult; - FmgrInfo *hash_finfo; - FunctionCallInfo hash_fcinfo; Oid cmpfuncid; /* @@ -1262,18 +1260,6 @@ ExecInitExprRec(Expr *node, ExprState *state, */ if (OidIsValid(opexpr->hashfuncid)) { - hash_finfo = palloc0(sizeof(FmgrInfo)); - hash_fcinfo = palloc0(SizeForFunctionCallInfo(1)); - fmgr_info(opexpr->hashfuncid, hash_finfo); - fmgr_info_set_expr((Node *) node, hash_finfo); - InitFunctionCallInfoData(*hash_fcinfo, hash_finfo, - 1, opexpr->inputcollid, NULL, - NULL); - - scratch.d.hashedscalararrayop.hash_finfo = hash_finfo; - scratch.d.hashedscalararrayop.hash_fcinfo_data = hash_fcinfo; - scratch.d.hashedscalararrayop.hash_fn_addr = hash_finfo->fn_addr; - /* Evaluate scalar directly into left function argument */ ExecInitExprRec(scalararg, state, &fcinfo->args[0].value, &fcinfo->args[0].isnull); @@ -1292,11 +1278,8 @@ ExecInitExprRec(Expr *node, ExprState *state, scratch.d.hashedscalararrayop.inclause = opexpr->useOr; scratch.d.hashedscalararrayop.finfo = finfo; scratch.d.hashedscalararrayop.fcinfo_data = fcinfo; - scratch.d.hashedscalararrayop.fn_addr = finfo->fn_addr; + scratch.d.hashedscalararrayop.saop = opexpr; - scratch.d.hashedscalararrayop.hash_finfo = hash_finfo; - scratch.d.hashedscalararrayop.hash_fcinfo_data = hash_fcinfo; - scratch.d.hashedscalararrayop.hash_fn_addr = hash_finfo->fn_addr; ExprEvalPushStep(state, &scratch); } diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index eaec697bb3..399bdfd861 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -217,6 +217,8 @@ typedef struct ScalarArrayOpExprHashTable { saophash_hash *hashtab; /* underlying hash table */ struct ExprEvalStep *op; + FunctionCallInfo hash_fcinfo_data; /* arguments etc */ + FmgrInfo *hash_finfo; /* function's lookup data */ } ScalarArrayOpExprHashTable; /* Define parameters for ScalarArrayOpExpr hash table code generation. */ @@ -3474,13 +3476,13 @@ static uint32 saop_element_hash(struct saophash_hash *tb, Datum key) { ScalarArrayOpExprHashTable *elements_tab = (ScalarArrayOpExprHashTable *) tb->private_data; - FunctionCallInfo fcinfo = elements_tab->op->d.hashedscalararrayop.hash_fcinfo_data; + FunctionCallInfo fcinfo = elements_tab->hash_fcinfo_data; Datum hash; fcinfo->args[0].value = key; fcinfo->args[0].isnull = false; - hash = elements_tab->op->d.hashedscalararrayop.hash_fn_addr(fcinfo); + hash = elements_tab->hash_finfo->fn_addr(fcinfo); return DatumGetUInt32(hash); } @@ -3502,7 +3504,7 @@ saop_hash_element_match(struct saophash_hash *tb, Datum key1, Datum key2) fcinfo->args[1].value = key2; fcinfo->args[1].isnull = false; - result = elements_tab->op->d.hashedscalararrayop.fn_addr(fcinfo); + result = elements_tab->op->d.hashedscalararrayop.finfo->fn_addr(fcinfo); return DatumGetBool(result); } @@ -3549,6 +3551,7 @@ ExecEvalHashedScalarArrayOp(ExprState *state, ExprEvalStep *op, ExprContext *eco /* Build the hash table on first evaluation */ if (elements_tab == NULL) { + ScalarArrayOpExpr *saop; int16 typlen; bool typbyval; char typalign; @@ -3560,6 +3563,8 @@ ExecEvalHashedScalarArrayOp(ExprState *state, ExprEvalStep *op, ExprContext *eco MemoryContext oldcontext; ArrayType *arr; + saop = op->d.hashedscalararrayop.saop; + arr = DatumGetArrayTypeP(*op->resvalue); nitems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr)); @@ -3575,6 +3580,18 @@ ExecEvalHashedScalarArrayOp(ExprState *state, ExprEvalStep *op, ExprContext *eco op->d.hashedscalararrayop.elements_tab = elements_tab; elements_tab->op = op; + elements_tab->hash_finfo = palloc0(sizeof(FmgrInfo)); + fmgr_info(saop->hashfuncid, elements_tab->hash_finfo); + fmgr_info_set_expr((Node *) saop, elements_tab->hash_finfo); + + elements_tab->hash_fcinfo_data = palloc0(SizeForFunctionCallInfo(1)); + InitFunctionCallInfoData(*elements_tab->hash_fcinfo_data, + elements_tab->hash_finfo, + 1, + saop->inputcollid, + NULL, + NULL); + /* * Create the hash table sizing it according to the number of elements * in the array. This does assume that the array has no duplicates. @@ -3669,7 +3686,7 @@ ExecEvalHashedScalarArrayOp(ExprState *state, ExprEvalStep *op, ExprContext *eco fcinfo->args[1].value = (Datum) 0; fcinfo->args[1].isnull = true; - result = op->d.hashedscalararrayop.fn_addr(fcinfo); + result = op->d.hashedscalararrayop.finfo->fn_addr(fcinfo); resultnull = fcinfo->isnull; /* diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index e34db8c93c..e5e4d7fc1f 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -582,12 +582,7 @@ typedef struct ExprEvalStep struct ScalarArrayOpExprHashTable *elements_tab; FmgrInfo *finfo; /* function's lookup data */ FunctionCallInfo fcinfo_data; /* arguments etc */ - /* faster to access without additional indirection: */ - PGFunction fn_addr; /* actual call address */ - FmgrInfo *hash_finfo; /* function's lookup data */ - FunctionCallInfo hash_fcinfo_data; /* arguments etc */ - /* faster to access without additional indirection: */ - PGFunction hash_fn_addr; /* actual call address */ + ScalarArrayOpExpr *saop; } hashedscalararrayop; /* for EEOP_XMLEXPR */ -- 2.34.1
