public inbox for [email protected]help / color / mirror / Atom feed
GenBKI emits useless open;close for catalogs without rows 4+ messages / 3 participants [nested] [flat]
* GenBKI emits useless open;close for catalogs without rows @ 2023-09-01 17:26 Matthias van de Meent <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Matthias van de Meent @ 2023-09-01 17:26 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, Whilst looking at PostgreSQL's bootstrapping process, I noticed that postgres.bki contains quite a few occurrances of the pattern "open $catname; close $catname". I suppose this pattern isn't too expensive, but according to my limited research a combined open+close cycle doens't do anything meaningful, so it does waste some CPU cycles in the process. The attached patch 1 removes the occurances of those combined open/close statements in postgresql.bki. Locally it passes check-world, so I assume that opening and closing a table is indeed not required for initializing a data-less catalog during bootstrapping. A potential addition to the patch would to stop manually closing relations: initdb and check-world succeed without manual 'close' operations because the 'open' command auto-closes the previous open relation (in boot_openrel). Testing also suggests that the last opened relation apparently doesn't need closing - check-world succeeds without issues (incl. with TAP enabled). That is therefore implemented in attached patch 2 - it removes the 'close' syntax in its entirety. Kind regards, Matthias van de Meent Neon (https://neon.tech) Attachments: [application/octet-stream] 0002-Remove-the-bki-close-command.patch (3.2K, ../../CAEze2Wh+uwUxKiDhYRBKuxin8A3nSuKf20LbSZwdmm1VKj_TWg@mail.gmail.com/2-0002-Remove-the-bki-close-command.patch) download | inline diff: From 65b21f740fc09dcf80ed60a77a3a64d85605a203 Mon Sep 17 00:00:00 2001 From: Matthias van de Meent <[email protected]> Date: Fri, 1 Sep 2023 19:23:17 +0200 Subject: [PATCH 2/2] Remove the bki 'close' command. Bootstrapping works without manually closing relations because they are auto-closed when a different relation is opened, and apparently leaking a single opened relation at the end of the runway isn't a problem in the currently available tests. --- src/backend/bootstrap/bootparse.y | 13 +------------ src/backend/bootstrap/bootscanner.l | 2 -- src/backend/catalog/genbki.pl | 12 ------------ 3 files changed, 1 insertion(+), 26 deletions(-) diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y index 81a1b7bfec..e405ed1c7b 100644 --- a/src/backend/bootstrap/bootparse.y +++ b/src/backend/bootstrap/bootparse.y @@ -99,7 +99,7 @@ static int num_columns_read = 0; /* NULLVAL is a reserved keyword */ %token NULLVAL /* All the rest are unreserved, and should be handled in boot_ident! */ -%token <kw> OPEN XCLOSE XCREATE INSERT_TUPLE +%token <kw> OPEN XCREATE INSERT_TUPLE %token <kw> XDECLARE INDEX ON USING XBUILD INDICES UNIQUE XTOAST %token <kw> OBJ_ID XBOOTSTRAP XSHARED_RELATION XROWTYPE_OID %token <kw> XFORCE XNOT XNULL @@ -120,7 +120,6 @@ Boot_Queries: Boot_Query : Boot_OpenStmt - | Boot_CloseStmt | Boot_CreateStmt | Boot_InsertStmt | Boot_DeclareIndexStmt @@ -138,15 +137,6 @@ Boot_OpenStmt: } ; -Boot_CloseStmt: - XCLOSE boot_ident - { - do_start(); - closerel($2); - do_end(); - } - ; - Boot_CreateStmt: XCREATE boot_ident oidspec optbootstrap optsharedrelation optrowtypeoid LPAREN { @@ -467,7 +457,6 @@ boot_column_val: boot_ident: ID { $$ = $1; } | OPEN { $$ = pstrdup($1); } - | XCLOSE { $$ = pstrdup($1); } | XCREATE { $$ = pstrdup($1); } | INSERT_TUPLE { $$ = pstrdup($1); } | XDECLARE { $$ = pstrdup($1); } diff --git a/src/backend/bootstrap/bootscanner.l b/src/backend/bootstrap/bootscanner.l index 6a9d4193f2..99cb7861c9 100644 --- a/src/backend/bootstrap/bootscanner.l +++ b/src/backend/bootstrap/bootscanner.l @@ -73,8 +73,6 @@ sid \'([^']|\'\')*\' open { boot_yylval.kw = "open"; return OPEN; } -close { boot_yylval.kw = "close"; return XCLOSE; } - create { boot_yylval.kw = "create"; return XCREATE; } OID { boot_yylval.kw = "OID"; return OBJ_ID; } diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl index 67d63864b3..814d945cb1 100644 --- a/src/backend/catalog/genbki.pl +++ b/src/backend/catalog/genbki.pl @@ -549,8 +549,6 @@ EOM { print $def $line; } - - my $opened = 0; # Open it, unless it's a bootstrap catalog (create bootstrap does this # automatically) @@ -559,13 +557,8 @@ EOM if (($catalog_data{$catname} || 0) > 0 || $catname eq 'pg_attribute') { print $bki "open $catname\n"; - $opened = 1; } } - else - { - $opened = 1; - } # For pg_attribute.h, we generate data entries ourselves. if ($catname eq 'pg_attribute') @@ -681,11 +674,6 @@ EOM } } - if ($opened eq 1) - { - print $bki "close $catname\n"; - } - printf $def "\n#endif\t\t\t\t\t\t\t/* %s_D_H */\n", uc $catname; # Close and rename definition header -- 2.40.1 [application/octet-stream] 0001-Stop-emitting-open-nodata-close-patterns-in-genbki.p.patch (1.3K, ../../CAEze2Wh+uwUxKiDhYRBKuxin8A3nSuKf20LbSZwdmm1VKj_TWg@mail.gmail.com/3-0001-Stop-emitting-open-nodata-close-patterns-in-genbki.p.patch) download | inline diff: From 4410ff8859e4239467ba29bda8ded0fd4603d175 Mon Sep 17 00:00:00 2001 From: Matthias van de Meent <[email protected]> Date: Wed, 11 Jan 2023 02:13:04 +0100 Subject: [PATCH 1/2] Stop emitting open/nodata/close patterns in genbki.pl Although opening and immediately closing the relation is not that expensive, it's still cheaper to not touch it at all. --- src/backend/catalog/genbki.pl | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl index 4a7205472c..67d63864b3 100644 --- a/src/backend/catalog/genbki.pl +++ b/src/backend/catalog/genbki.pl @@ -549,12 +549,22 @@ EOM { print $def $line; } + + my $opened = 0; # Open it, unless it's a bootstrap catalog (create bootstrap does this # automatically) if (!$catalog->{bootstrap}) { - print $bki "open $catname\n"; + if (($catalog_data{$catname} || 0) > 0 || $catname eq 'pg_attribute') + { + print $bki "open $catname\n"; + $opened = 1; + } + } + else + { + $opened = 1; } # For pg_attribute.h, we generate data entries ourselves. @@ -671,7 +681,11 @@ EOM } } - print $bki "close $catname\n"; + if ($opened eq 1) + { + print $bki "close $catname\n"; + } + printf $def "\n#endif\t\t\t\t\t\t\t/* %s_D_H */\n", uc $catname; # Close and rename definition header -- 2.40.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: GenBKI emits useless open;close for catalogs without rows @ 2023-09-01 17:43 Alvaro Herrera <[email protected]> parent: Matthias van de Meent <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Alvaro Herrera @ 2023-09-01 17:43 UTC (permalink / raw) To: Matthias van de Meent <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 2023-Sep-01, Matthias van de Meent wrote: > A potential addition to the patch would to stop manually closing > relations: initdb and check-world succeed without manual 'close' > operations because the 'open' command auto-closes the previous open > relation (in boot_openrel). Testing also suggests that the last opened > relation apparently doesn't need closing - check-world succeeds > without issues (incl. with TAP enabled). That is therefore implemented > in attached patch 2 - it removes the 'close' syntax in its entirety. Hmm, what happens with the last relation in the bootstrap process? Is closerel() called via some other path for that one? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Essentially, you're proposing Kevlar shoes as a solution for the problem that you want to walk around carrying a loaded gun aimed at your foot. (Tom Lane) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: GenBKI emits useless open;close for catalogs without rows @ 2023-09-01 17:50 Matthias van de Meent <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Matthias van de Meent @ 2023-09-01 17:50 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Fri, 1 Sept 2023 at 19:43, Alvaro Herrera <[email protected]> wrote: > > On 2023-Sep-01, Matthias van de Meent wrote: > > > A potential addition to the patch would to stop manually closing > > relations: initdb and check-world succeed without manual 'close' > > operations because the 'open' command auto-closes the previous open > > relation (in boot_openrel). Testing also suggests that the last opened > > relation apparently doesn't need closing - check-world succeeds > > without issues (incl. with TAP enabled). That is therefore implemented > > in attached patch 2 - it removes the 'close' syntax in its entirety. > > Hmm, what happens with the last relation in the bootstrap process? Is > closerel() called via some other path for that one? There is a final cleanup() call that closes the last open boot_reldesc relation (if any) at the end of BootstrapModeMain, after boot_yyparse has completed and its changes have been committed. - Matthias ^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v6 1/2] pg_lfind32(): add "overlap" code for remaining elements @ 2024-03-20 19:20 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Nathan Bossart @ 2024-03-20 19:20 UTC (permalink / raw) --- src/include/port/pg_lfind.h | 103 ++++++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 33 deletions(-) diff --git a/src/include/port/pg_lfind.h b/src/include/port/pg_lfind.h index b8dfa66eef..22a3711ab5 100644 --- a/src/include/port/pg_lfind.h +++ b/src/include/port/pg_lfind.h @@ -80,6 +80,49 @@ pg_lfind8_le(uint8 key, uint8 *base, uint32 nelem) return false; } +/* + * pg_lfind32_helper + * + * Searches one 4-register-block of integers. The caller is responsible for + * ensuring that there are at least 4-registers-worth of integers remaining. + */ +static inline bool +pg_lfind32_helper(const Vector32 keys, uint32 *base) +{ + const uint32 nelem_per_vector = sizeof(Vector32) / sizeof(uint32); + Vector32 vals1, + vals2, + vals3, + vals4, + result1, + result2, + result3, + result4, + tmp1, + tmp2, + result; + + /* load the next block into 4 registers */ + vector32_load(&vals1, base); + vector32_load(&vals2, &base[nelem_per_vector]); + vector32_load(&vals3, &base[nelem_per_vector * 2]); + vector32_load(&vals4, &base[nelem_per_vector * 3]); + + /* compare each value to the key */ + result1 = vector32_eq(keys, vals1); + result2 = vector32_eq(keys, vals2); + result3 = vector32_eq(keys, vals3); + result4 = vector32_eq(keys, vals4); + + /* combine the results into a single variable */ + tmp1 = vector32_or(result1, result2); + tmp2 = vector32_or(result3, result4); + result = vector32_or(tmp1, tmp2); + + /* return whether there was a match */ + return vector32_is_highbit_set(result); +} + /* * pg_lfind32 * @@ -119,46 +162,40 @@ pg_lfind32(uint32 key, uint32 *base, uint32 nelem) } #endif - for (i = 0; i < tail_idx; i += nelem_per_iteration) + /* + * If there aren't enough elements for the SIMD code, jump to the standard + * one-by-one linear search code. + */ + if (nelem <= nelem_per_iteration) + goto one_by_one; + + /* + * Process as many elements as possible with a block of 4 registers. + */ + do { - Vector32 vals1, - vals2, - vals3, - vals4, - result1, - result2, - result3, - result4, - tmp1, - tmp2, - result; - - /* load the next block into 4 registers */ - vector32_load(&vals1, &base[i]); - vector32_load(&vals2, &base[i + nelem_per_vector]); - vector32_load(&vals3, &base[i + nelem_per_vector * 2]); - vector32_load(&vals4, &base[i + nelem_per_vector * 3]); - - /* compare each value to the key */ - result1 = vector32_eq(keys, vals1); - result2 = vector32_eq(keys, vals2); - result3 = vector32_eq(keys, vals3); - result4 = vector32_eq(keys, vals4); - - /* combine the results into a single variable */ - tmp1 = vector32_or(result1, result2); - tmp2 = vector32_or(result3, result4); - result = vector32_or(tmp1, tmp2); - - /* see if there was a match */ - if (vector32_is_highbit_set(result)) + if (pg_lfind32_helper(keys, &base[i])) { Assert(assert_result == true); return true; } - } + + i += nelem_per_iteration; + + } while (i < tail_idx); + + /* + * Process the last 'nelem_per_iteration' elements in the array with a + * 4-register block. This will cause us to check some of the elements + * more than once, but that won't affect correctness, and testing has + * demonstrated that this helps more cases than it harms. + */ + Assert(assert_result == pg_lfind32_helper(keys, &base[nelem - nelem_per_iteration])); + return pg_lfind32_helper(keys, &base[nelem - nelem_per_iteration]); + #endif /* ! USE_NO_SIMD */ +one_by_one: /* Process the remaining elements one at a time. */ for (; i < nelem; i++) { -- 2.25.1 --3MwIy2ne0vdjdPXF Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0002-Add-support-for-AVX2-in-simd.h.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-03-20 19:20 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-09-01 17:26 GenBKI emits useless open;close for catalogs without rows Matthias van de Meent <[email protected]> 2023-09-01 17:43 ` Alvaro Herrera <[email protected]> 2023-09-01 17:50 ` Matthias van de Meent <[email protected]> 2024-03-20 19:20 [PATCH v6 1/2] pg_lfind32(): add "overlap" code for remaining elements Nathan Bossart <[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