public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v9 05/17] Update BitmapAdjustPrefetchIterator parameter type to BlockNumber 4+ messages / 4 participants [nested] [flat]
* [PATCH v9 05/17] Update BitmapAdjustPrefetchIterator parameter type to BlockNumber @ 2024-02-13 00:04 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Melanie Plageman @ 2024-02-13 00:04 UTC (permalink / raw) BitmapAdjustPrefetchIterator() only used the blockno member of the passed in TBMIterateResult to ensure that the prefetch iterator and regular iterator stay in sync. Pass it the BlockNumber only. This will allow us to move away from using the TBMIterateResult outside of table AM specific code. --- src/backend/executor/nodeBitmapHeapscan.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 5df3b5ca46..404de0595e 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -52,7 +52,7 @@ static TupleTableSlot *BitmapHeapNext(BitmapHeapScanState *node); static inline void BitmapDoneInitializingSharedState(ParallelBitmapHeapState *pstate); static inline void BitmapAdjustPrefetchIterator(BitmapHeapScanState *node, - TBMIterateResult *tbmres); + BlockNumber blockno); static inline void BitmapAdjustPrefetchTarget(BitmapHeapScanState *node); static inline void BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan); @@ -230,7 +230,7 @@ BitmapHeapNext(BitmapHeapScanState *node) break; } - BitmapAdjustPrefetchIterator(node, tbmres); + BitmapAdjustPrefetchIterator(node, tbmres->blockno); valid = table_scan_bitmap_next_block(scan, tbmres); @@ -341,7 +341,7 @@ BitmapDoneInitializingSharedState(ParallelBitmapHeapState *pstate) */ static inline void BitmapAdjustPrefetchIterator(BitmapHeapScanState *node, - TBMIterateResult *tbmres) + BlockNumber blockno) { #ifdef USE_PREFETCH ParallelBitmapHeapState *pstate = node->pstate; @@ -360,7 +360,7 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node, /* Do not let the prefetch iterator get behind the main one */ TBMIterateResult *tbmpre = tbm_iterate(prefetch_iterator); - if (tbmpre == NULL || tbmpre->blockno != tbmres->blockno) + if (tbmpre == NULL || tbmpre->blockno != blockno) elog(ERROR, "prefetch and main iterators are out of sync"); } return; -- 2.40.1 --7mdtsjmrzitrgzgx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0006-table_scan_bitmap_next_block-returns-lossy-or-exa.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: More speedups for tuple deformation @ 2026-02-26 13:52 David Rowley <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: David Rowley @ 2026-02-26 13:52 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Andres Freund <[email protected]>; John Naylor <[email protected]>; Chao Li <[email protected]>; PostgreSQL Developers <[email protected]> Thanks for having a look at this. On Wed, 25 Feb 2026 at 22:49, Álvaro Herrera <[email protected]> wrote: > With all these patches applied, I get this from pahole for > CompactAttribute, > The 'attnullability' thing stands out. It probably doesn't make much of > a difference -- considering that it's down to 8 bytes already and I > doubt anything will change if it's instead 6 -- but given these > definitions We could get it to 6 bytes. The alignment is 2, so there's certainly a memory saving there. I think the primary reason to make it 8 was that the LEA instruction can be used without any other instructions to calculate the address of the first CompactAttribute element to use. LEA can shift the array index to multiply it by 1,2,4 or 8, and because it can do shift and add concurrently, it can utilise the add to add the offset of compact_attrs in the TupleDesc struct. I see gcc doing this when I make attnum a size_t. This is effectively what I described above: 1c46: 48 8d 74 c2 20 lea rsi,[rdx+rax*8+0x20] The 0x20 is the offset to the start of compact_attrs in TupleDescData. rax is attnum. rdx is the tupledesc. Effectively, that's: rsi = ((char *) tupledesc) + (attnum << 3) + offsetof(TupleDescData, compact_attrs); If I make the CompactAttribute 6 bytes, I see gcc producing two LEA instructions instead of one: 1c80: 48 8d 34 40 lea rsi,[rax+rax*2] 1c89: 49 8d 74 74 20 lea rsi,[r12+rsi*2+0x20] rax is attnum again. r12 is the tupledesc. Effectively that's: /* multiply attnum by 3, store in rdi */ rsi = attnum + (attnum << 1); /* multiply rdi by 2 to make multiply by 6 and at that plus 32 to the tupledesc's addr. */ rsi = ((char *) tupledesc) + (rsi << 1) + offsetof(TupleDescData, compact_attrs); I expect that the extra address calculation costs plus the extra bitwise-AND for checking attbyval will cost more than it will save. > Regarding deform_bench, I wonder if we shouldn't start a proper > benchmarking suite (of which this would the first participant) by > putting this in src/benchmark/tuple_deform instead (or something like > that) instead of relegating it to src/test/modules. It doesn't make > much of a technical difference, but it is a bit of a statement that more > tools are welcome. Yeah. Andres was talking about this in [1]. I'd originally not planned on committing deform_bench at all and only put it in the current location in the patch as that seemed like the best location for it that we have today. Andres mentioned just having 1 extension and adding functions to it. That seems fine to me. I have some other things that could go in there. If it's just one extension, then there's likely no need to make a special directory for it. Maybe src/test/modules is fine. For now, I don't want to focus too much as I'd rather get the deformation speedups ready and in. David [1] https://postgr.es/m/rbxc2qqhsvzxpukgd36caoa4ydgn5r22fxktxanrkn6nobg7j6%4027b4vogohgu2 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: More speedups for tuple deformation @ 2026-02-26 15:01 Álvaro Herrera <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Álvaro Herrera @ 2026-02-26 15:01 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Andres Freund <[email protected]>; John Naylor <[email protected]>; Chao Li <[email protected]>; PostgreSQL Developers <[email protected]> On 2026-Feb-27, David Rowley wrote: > > The 'attnullability' thing stands out. It probably doesn't make much of > > a difference -- considering that it's down to 8 bytes already and I > > doubt anything will change if it's instead 6 -- but given these > > definitions > > We could get it to 6 bytes. [...] > If I make the CompactAttribute 6 bytes, I see gcc producing two LEA > instructions instead of one: [...] > I expect that the extra address calculation costs plus the extra > bitwise-AND for checking attbyval will cost more than it will save. Ah, that makes sense. Interesting, thanks for explaining. > > Regarding deform_bench, I wonder if we shouldn't start a proper > > benchmarking suite [...] > > Yeah. Andres was talking about this in [1]. I'd originally not planned > on committing deform_bench at all and only put it in the current > location in the patch as that seemed like the best location for it > that we have today. Andres mentioned just having 1 extension and > adding functions to it. That seems fine to me. I have some other > things that could go in there. If it's just one extension, then > there's likely no need to make a special directory for it. Maybe > src/test/modules is fine. For now, I don't want to focus too much as > I'd rather get the deformation speedups ready and in. Hmm, ok. I'd rather not get it in the tree at all for now, and see what else can we get in there later. Then we might have better ideas of what to do; if we do get it in src/test/modules now, we might not want to move it elsewhere later as it might be more painful. As you say, if it's just one extension then it doesn't matter much where it is. But we might get other ideas that require more than one extension, or more than one benchmarking suite, and then src/test/modules doesn't sound so great a location. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: More speedups for tuple deformation @ 2026-02-26 15:08 Andres Freund <[email protected]> parent: Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Andres Freund @ 2026-02-26 15:08 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: David Rowley <[email protected]>; Zsolt Parragi <[email protected]>; John Naylor <[email protected]>; Chao Li <[email protected]>; PostgreSQL Developers <[email protected]> Hi, On 2026-02-26 16:01:26 +0100, Álvaro Herrera wrote: > On 2026-Feb-27, David Rowley wrote: > > > Regarding deform_bench, I wonder if we shouldn't start a proper > > > benchmarking suite [...] > > > > Yeah. Andres was talking about this in [1]. I'd originally not planned > > on committing deform_bench at all and only put it in the current > > location in the patch as that seemed like the best location for it > > that we have today. Andres mentioned just having 1 extension and > > adding functions to it. That seems fine to me. I have some other > > things that could go in there. If it's just one extension, then > > there's likely no need to make a special directory for it. Maybe > > src/test/modules is fine. For now, I don't want to focus too much as > > I'd rather get the deformation speedups ready and in. > > Hmm, ok. I'd rather not get it in the tree at all for now, and see what > else can we get in there later. Then we might have better ideas of what > to do; if we do get it in src/test/modules now, we might not want to > move it elsewhere later as it might be more painful. As you say, if > it's just one extension then it doesn't matter much where it is. But we > might get other ideas that require more than one extension, or more than > one benchmarking suite, and then src/test/modules doesn't sound so great > a location. I don't get this. We'll approximately never have a full suite of benchmark tools that make sense to integrate at once. The reason we're here is because over and over we've not actually merged quite useful tools, making do with benchmarking-by-proxy and one off tools. This seems like a recipe for never getting anywhere better. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-02-26 15:08 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-02-13 00:04 [PATCH v9 05/17] Update BitmapAdjustPrefetchIterator parameter type to BlockNumber Melanie Plageman <[email protected]> 2026-02-26 13:52 Re: More speedups for tuple deformation David Rowley <[email protected]> 2026-02-26 15:01 ` Re: More speedups for tuple deformation Álvaro Herrera <[email protected]> 2026-02-26 15:08 ` Re: More speedups for tuple deformation Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox