Tidy fill hstv array (src/backend/access/heap/pruneheap.c)
Hi,
The function heap_page_prune (src/backend/access/heap/pruneheap.c)
Has a comment:
"/*
* presult->htsv is not initialized here because all ntuple spots in the
* array will be set either to a valid HTSV_Result value or -1.
*/
IMO, this is a little bogus and does not make sense.
I think it would be more productive to initialize the entire array with -1,
and avoid flagging, one by one, the items that should be -1.
Patch attached.
best regards,
Ranier Vilela
Attachments:
001-tidy-fill-htsv-array.patchapplication/octet-stream; name=001-tidy-fill-htsv-array.patchDownload
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index c5f1abd95a..12b9b5bed9 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -231,12 +231,9 @@ heap_page_prune(Relation relation, Buffer buffer,
prstate.nredirected = prstate.ndead = prstate.nunused = 0;
memset(prstate.marked, 0, sizeof(prstate.marked));
- /*
- * presult->htsv is not initialized here because all ntuple spots in the
- * array will be set either to a valid HTSV_Result value or -1.
- */
presult->ndeleted = 0;
presult->nnewlpdead = 0;
+ memset(presult->htsv, -1, sizeof(presult->htsv));
maxoff = PageGetMaxOffsetNumber(page);
tup.t_tableOid = RelationGetRelid(prstate.rel);
@@ -270,10 +267,7 @@ heap_page_prune(Relation relation, Buffer buffer,
/* Nothing to do if slot doesn't contain a tuple */
if (!ItemIdIsNormal(itemid))
- {
- presult->htsv[offnum] = -1;
continue;
- }
htup = (HeapTupleHeader) PageGetItem(page, itemid);
tup.t_data = htup;
On Thu, Dec 28, 2023 at 7:58 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
I think it would be more productive to initialize the entire array with -1, and avoid flagging, one by one, the items that should be -1.
This just moves an operation from one place to the other, while
obliterating the explanatory comment, so I don't see an advantage.
Em ter., 9 de jan. de 2024 às 06:31, John Naylor <johncnaylorls@gmail.com>
escreveu:
On Thu, Dec 28, 2023 at 7:58 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
I think it would be more productive to initialize the entire array with
-1, and avoid flagging, one by one, the items that should be -1.
This just moves an operation from one place to the other, while
obliterating the explanatory comment, so I don't see an advantage.
Well, I think that is precisely the case for using memset.
The way initialization is currently done is much slower and harmful to the
branch.
Of course, the gain should be small, but it is fully justified for
switching to memset.
Regarding the comment, once initialization is done via memset, such as
prstate.marked, it becomes irrelevant and unnecessary.
Best regards,
Ranier Vilela
On Sat, Jan 13, 2024 at 9:36 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Em ter., 9 de jan. de 2024 às 06:31, John Naylor <johncnaylorls@gmail.com> escreveu:
This just moves an operation from one place to the other, while
obliterating the explanatory comment, so I don't see an advantage.Well, I think that is precisely the case for using memset.
The way initialization is currently done is much slower and harmful to the branch.
Of course, the gain should be small, but it is fully justified for switching to memset.
We haven't seen any evidence or reasoning for that. Simple
rules-of-thumb are not enough.
On 14 Jan 2024, at 18:55, John Naylor <johncnaylorls@gmail.com> wrote:
On Sat, Jan 13, 2024 at 9:36 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Em ter., 9 de jan. de 2024 às 06:31, John Naylor <johncnaylorls@gmail.com> escreveu:
This just moves an operation from one place to the other, while
obliterating the explanatory comment, so I don't see an advantage.Well, I think that is precisely the case for using memset.
The way initialization is currently done is much slower and harmful to the branch.
Of course, the gain should be small, but it is fully justified for switching to memset.We haven't seen any evidence or reasoning for that. Simple
rules-of-thumb are not enough.
Hi Ranier,
I’ll mark CF entry [0]https://commitfest.postgresql.org/47/4734/ as “Returned with Feedback”. Feel free to reopen item in this CF or submit to the next, if you want to continue working on this.
I took a glance into the patch, and I would agree that setting field nonzero values with memset() is somewhat unusual. Please provide stronger evidence to do so.
Thanks!
Best regards, Andrey Borodin.
Em seg., 4 de mar. de 2024 às 03:18, Andrey M. Borodin <x4mmm@yandex-team.ru>
escreveu:
On 14 Jan 2024, at 18:55, John Naylor <johncnaylorls@gmail.com> wrote:
On Sat, Jan 13, 2024 at 9:36 PM Ranier Vilela <ranier.vf@gmail.com>
wrote:
Em ter., 9 de jan. de 2024 às 06:31, John Naylor <
johncnaylorls@gmail.com> escreveu:
This just moves an operation from one place to the other, while
obliterating the explanatory comment, so I don't see an advantage.Well, I think that is precisely the case for using memset.
The way initialization is currently done is much slower and harmful tothe branch.
Of course, the gain should be small, but it is fully justified for
switching to memset.
We haven't seen any evidence or reasoning for that. Simple
rules-of-thumb are not enough.Hi Ranier,
I’ll mark CF entry [0] as “Returned with Feedback”. Feel free to reopen
item in this CF or submit to the next, if you want to continue working on
this.I took a glance into the patch, and I would agree that setting field
nonzero values with memset() is somewhat unusual. Please provide stronger
evidence to do so.
I counted the calls with non-zero memset in the entire postgres code and
they are about 183 calls.
I counted the calls with non-zero memset in the entire postgres code and
they are about 183 calls.
At least 4 calls with -1
File contrib\pg_trgm\trgm_op.c:
memset(lastpos, -1, sizeof(int) * len);
File src\backend\executor\execPartition.c:
memset(pd->indexes, -1, sizeof(int) * partdesc->nparts);
File src\backend\partitioning\partprune.c:
memset(subplan_map, -1, nparts * sizeof(int));
memset(subpart_map, -1, nparts * sizeof(int));
Does filling a memory area, one by one, with branches, need strong evidence
to prove that it is better than filling a memory area, all at once, without
branches?
best regards,
Ranier Vilela
On 4 Mar 2024, at 16:47, Ranier Vilela <ranier.vf@gmail.com> wrote:
Does filling a memory area, one by one, with branches, need strong evidence to prove that it is better than filling a memory area, all at once, without branches?
I apologise for being too quick to decide to mark the patch RwF. Wold you mind if restore patch as "Needs review" so we can have more feedback?
Best regards, Andrey Borodin.
Hi,
On 2024-03-04 08:47:11 -0300, Ranier Vilela wrote:
Does filling a memory area, one by one, with branches, need strong evidence
to prove that it is better than filling a memory area, all at once, without
branches?
That's a bogus comparison:
a) the memset version modifies the whole array, rather than just elements that
correspond to an item - often there will be far fewer items than the
maximally possible number
b) the memset version initializes array elements that will be set to an actual
value
c) switching to memset does not elide any branches, as the branch is still
needed
And even without those, it'd still not be obviously better to use an
ahead-of-time memset(), as storing lots of values at once is more likely to be
bound by memory bandwidth than interleaving different work.
Andres