public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/9] Optimize allocations in bringetbitmap
6+ messages / 4 participants
[nested] [flat]
* [PATCH 3/9] Optimize allocations in bringetbitmap
@ 2020-09-13 10:12 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Tomas Vondra @ 2020-09-13 10:12 UTC (permalink / raw)
The bringetbitmap function allocates memory for various purposes, which
may be quite expensive, depending on the number of scan keys. Instead of
allocating them separately, allocate one bit chunk of memory an carve it
into smaller pieces as needed - all the pieces have the same lifespan,
and it saves quite a bit of CPU and memory overhead.
Author: Tomas Vondra <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/brin/brin.c | 62 +++++++++++++++++++++++++++-------
1 file changed, 49 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 14da9ed17f..c7f7175a2a 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -373,6 +373,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
int *nkeys,
*nnullkeys;
int keyno;
+ char *ptr;
+ Size len;
+ char *tmp PG_USED_FOR_ASSERTS_ONLY;
opaque = (BrinOpaque *) scan->opaque;
bdesc = opaque->bo_bdesc;
@@ -398,11 +401,50 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
* Make room for per-attribute lists of scan keys that we'll pass to the
* consistent support procedure. We keep null and regular keys separate,
* so that we can easily pass regular keys to the consistent function.
+ *
+ * To reduce the allocation overhead, we allocate one big chunk and then
+ * carve it into smaller arrays ourselves. All the pieces have exactly
+ * the same lifetime, so that's OK.
*/
- keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
- nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
- nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
- nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
+ len =
+ /* regular keys */
+ MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) +
+ MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys) * bdesc->bd_tupdesc->natts +
+ MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts) +
+ /* NULL keys */
+ MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) +
+ MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys) * bdesc->bd_tupdesc->natts +
+ MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts);
+
+ ptr = palloc(len);
+ tmp = ptr;
+
+ keys = (ScanKey **) ptr;
+ ptr += MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
+
+ nullkeys = (ScanKey **) ptr;
+ ptr += MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
+
+ nkeys = (int *) ptr;
+ ptr += MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts);
+
+ nnullkeys = (int *) ptr;
+ ptr += MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts);
+
+ for (int i = 0; i < bdesc->bd_tupdesc->natts; i++)
+ {
+ keys[i] = (ScanKey *) ptr;
+ ptr += MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys);
+
+ nullkeys[i] = (ScanKey *) ptr;
+ ptr += MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys);
+ }
+
+ Assert(tmp + len == ptr);
+
+ /* zero the number of keys */
+ memset(nkeys, 0, sizeof(int) * bdesc->bd_tupdesc->natts);
+ memset(nnullkeys, 0, sizeof(int) * bdesc->bd_tupdesc->natts);
/*
* Preprocess the scan keys - split them into per-attribute arrays.
@@ -438,9 +480,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
{
FmgrInfo *tmp;
- /* No key/null arrays for this attribute. */
- Assert((keys[keyattno - 1] == NULL) && (nkeys[keyattno - 1] == 0));
- Assert((nullkeys[keyattno - 1] == NULL) && (nnullkeys[keyattno - 1] == 0));
+ /* First time we see this attribute, so no key/null keys. */
+ Assert(nkeys[keyattno - 1] == 0);
+ Assert(nnullkeys[keyattno - 1] == 0);
tmp = index_getprocinfo(idxRel, keyattno,
BRIN_PROCNUM_CONSISTENT);
@@ -451,17 +493,11 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
/* Add key to the proper per-attribute array. */
if (key->sk_flags & SK_ISNULL)
{
- if (!nullkeys[keyattno - 1])
- nullkeys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
-
nullkeys[keyattno - 1][nnullkeys[keyattno - 1]] = key;
nnullkeys[keyattno - 1]++;
}
else
{
- if (!keys[keyattno - 1])
- keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
-
keys[keyattno - 1][nkeys[keyattno - 1]] = key;
nkeys[keyattno - 1]++;
}
--
2.26.2
--------------0E72B707603BED22B4040825
Content-Type: text/x-patch; charset=UTF-8;
name="0002-Move-IS-NOT-NULL-handling-from-BRIN-support-20210211.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0002-Move-IS-NOT-NULL-handling-from-BRIN-support-20210211.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-*
@ 2023-11-23 13:32 Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Dunstan @ 2023-11-23 13:32 UTC (permalink / raw)
To: Andres Freund <[email protected]>; [email protected]
On 2023-11-20 Mo 20:53, Andres Freund wrote:
> meson: docs: Add {html,man} targets, rename install-doc-*
>
> We have toplevel html, man targets in the autoconf build as well. It'd be odd
> to have an 'html' target but have the install target be 'install-doc-html',
> thus rename the install targets to match.
This commit of one of its nearby friends appears to have broken crake's
docs build:
ERROR: Can't invoke target `html`: ambiguous name.Add target type and/or path:
- ./doc/src/sgml/html:custom
- ./doc/src/sgml/html:alias
See<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2023-11-23%2012%3A52%3A04;
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-*
@ 2023-11-29 02:28 Andres Freund <[email protected]>
parent: Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2023-11-29 02:28 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: [email protected]
Hi,
On 2023-11-23 08:32:21 -0500, Andrew Dunstan wrote:
> On 2023-11-20 Mo 20:53, Andres Freund wrote:
> > meson: docs: Add {html,man} targets, rename install-doc-*
> >
> > We have toplevel html, man targets in the autoconf build as well. It'd be odd
> > to have an 'html' target but have the install target be 'install-doc-html',
> > thus rename the install targets to match.
>
>
> This commit of one of its nearby friends appears to have broken crake's docs
> build:
>
> ERROR: Can't invoke target `html`: ambiguous name.Add target type and/or path:
> - ./doc/src/sgml/html:custom
> - ./doc/src/sgml/html:alias
>
> See<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2023-11-23%2012%3A52%3A04;
Ah, I realize now that this is from meson compile html, not 'ninja html'. That
explains why I couldn't reproduce this initially and why CI didn't complain.
I don't really understand why meson compile complains in this case. I assume
you don't want to disambiguate as suggested, by building html:alias instead?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-*
@ 2023-11-29 12:20 Andrew Dunstan <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Dunstan @ 2023-11-29 12:20 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: [email protected]
On 2023-11-28 Tu 21:28, Andres Freund wrote:
> Hi,
>
> On 2023-11-23 08:32:21 -0500, Andrew Dunstan wrote:
>> On 2023-11-20 Mo 20:53, Andres Freund wrote:
>>> meson: docs: Add {html,man} targets, rename install-doc-*
>>>
>>> We have toplevel html, man targets in the autoconf build as well. It'd be odd
>>> to have an 'html' target but have the install target be 'install-doc-html',
>>> thus rename the install targets to match.
>>
>> This commit of one of its nearby friends appears to have broken crake's docs
>> build:
>>
>> ERROR: Can't invoke target `html`: ambiguous name.Add target type and/or path:
>> - ./doc/src/sgml/html:custom
>> - ./doc/src/sgml/html:alias
>>
>> See<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2023-11-23%2012%3A52%3A04;
> Ah, I realize now that this is from meson compile html, not 'ninja html'. That
> explains why I couldn't reproduce this initially and why CI didn't complain.
> I don't really understand why meson compile complains in this case. I assume
> you don't want to disambiguate as suggested, by building html:alias instead?
>
I've done that as a temporary fix to get crake out of the hole, but it's
pretty ugly, and I don't want to do it in a release if at all possible.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-*
@ 2023-11-29 13:49 Tom Lane <[email protected]>
parent: Andrew Dunstan <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Tom Lane @ 2023-11-29 13:49 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected]
Andrew Dunstan <[email protected]> writes:
> On 2023-11-28 Tu 21:28, Andres Freund wrote:
>> I don't really understand why meson compile complains in this case. I assume
>> you don't want to disambiguate as suggested, by building html:alias instead?
> I've done that as a temporary fix to get crake out of the hole, but it's
> pretty ugly, and I don't want to do it in a release if at all possible.
Our documentation says specifically that "ninja html" will build the
HTML format. I would expect that to work by analogy with the "make"
target; having to spell it differently seems like clearly a bug.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-*
@ 2023-11-29 18:36 Andres Freund <[email protected]>
parent: Andrew Dunstan <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Andres Freund @ 2023-11-29 18:36 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; +Cc: [email protected]
Hi,
This started at https://www.postgresql.org/message-id/746ba786-85bb-d1f7-b613-57bec35c642a%40dunslane.net
but seems worth discussing on -hackers.
On 2023-11-29 07:20:59 -0500, Andrew Dunstan wrote:
> On 2023-11-28 Tu 21:28, Andres Freund wrote:
> > On 2023-11-23 08:32:21 -0500, Andrew Dunstan wrote:
> > > On 2023-11-20 Mo 20:53, Andres Freund wrote:
> > > > meson: docs: Add {html,man} targets, rename install-doc-*
> > > >
> > > > We have toplevel html, man targets in the autoconf build as well. It'd be odd
> > > > to have an 'html' target but have the install target be 'install-doc-html',
> > > > thus rename the install targets to match.
> > >
> > > This commit of one of its nearby friends appears to have broken crake's docs
> > > build:
> > >
> > > ERROR: Can't invoke target `html`: ambiguous name.Add target type and/or path:
> > > - ./doc/src/sgml/html:custom
> > > - ./doc/src/sgml/html:alias
> > >
> > > See<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2023-11-23%2012%3A52%3A04;
> > Ah, I realize now that this is from meson compile html, not 'ninja html'. That
> > explains why I couldn't reproduce this initially and why CI didn't complain.
> > I don't really understand why meson compile complains in this case. I assume
> > you don't want to disambiguate as suggested, by building html:alias instead?
> >
>
> I've done that as a temporary fix to get crake out of the hole, but it's
> pretty ugly, and I don't want to do it in a release if at all possible.
If we want to prevent these kind of conflicts, which doesn't seem
unreasonable, I think we need an automatic check that prevents reintroducing
them. I think most people will just use ninja and not see them. Meson stores
the relevant information in meson-info/intro-targets.json, so that's just a
bit of munging of that file.
I think the background for this issue existing is that meson supports a "flat"
build directory layout (which is deprecated), so the directory name can't be
used to deconflict with meson compile, which tries to work across all "build
execution" systems.
Prototype of such a check, as well as a commit deconflicting the target names,
attached.
Greetings,
Andres Freund
Attachments:
[text/x-diff] v1-0001-meson-Rename-target-names-that-conflict-with-glob.patch (2.0K, ../../[email protected]/2-v1-0001-meson-Rename-target-names-that-conflict-with-glob.patch)
download | inline diff:
From 7b44707b9caa4528f1a6e8dd218644fa114007b3 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Wed, 29 Nov 2023 10:33:47 -0800
Subject: [PATCH v1 1/2] meson: Rename target names that conflict with global
target names
Author:
Reviewed-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
src/common/unicode/meson.build | 2 +-
doc/src/sgml/meson.build | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/common/unicode/meson.build b/src/common/unicode/meson.build
index 6af46122c4e..c548710c293 100644
--- a/src/common/unicode/meson.build
+++ b/src/common/unicode/meson.build
@@ -139,7 +139,7 @@ endif
# Use a custom target, as run targets serialize the output, making this harder
# to debug, and don't deal well with targets with multiple outputs.
-update_unicode = custom_target('update-unicode',
+update_unicode = custom_target('tgt-update-unicode',
depends: update_unicode_dep,
output: ['dont-exist'],
input: update_unicode_targets,
diff --git a/doc/src/sgml/meson.build b/doc/src/sgml/meson.build
index e1a85dc607b..c604bdbd644 100644
--- a/doc/src/sgml/meson.build
+++ b/doc/src/sgml/meson.build
@@ -135,7 +135,7 @@ endif
# Full documentation as html, text
#
if docs_dep.found()
- html = custom_target('html',
+ html = custom_target('tgt-build-html',
input: ['stylesheet.xsl', postgres_full_xml],
output: 'html',
depfile: 'html.d',
@@ -144,7 +144,7 @@ if docs_dep.found()
)
alldocs += html
- install_doc_html = custom_target('install-html',
+ install_doc_html = custom_target('tgt-install-html',
output: 'install-html',
command: [
python, install_files, '--prefix', dir_prefix,
@@ -224,7 +224,7 @@ endif
#
if docs_dep.found()
# FIXME: implement / consider sqlmansectnum logic
- man = custom_target('man',
+ man = custom_target('tgt-build-man',
input: ['stylesheet-man.xsl', postgres_full_xml],
output: ['man1', 'man3', 'man7'],
depfile: 'man.d',
--
2.38.0
[text/x-diff] v1-0002-meson-Add-test-checking-if-there-are-conflicting-.patch (3.4K, ../../[email protected]/3-v1-0002-meson-Add-test-checking-if-there-are-conflicting-.patch)
download | inline diff:
From a15cc895abfc9b426af75051d069fd625391b618 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Wed, 29 Nov 2023 10:30:21 -0800
Subject: [PATCH v1 2/2] meson: Add test checking if there are conflicting
target names
Author:
Reviewed-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
meson.build | 8 ++++++
src/tools/check_meson | 67 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
create mode 100755 src/tools/check_meson
diff --git a/meson.build b/meson.build
index 0095fb183af..e0895f41f78 100644
--- a/meson.build
+++ b/meson.build
@@ -3320,6 +3320,14 @@ if meson.version().version_compare('>=0.57')
endif
+# Tests verifing that source code follows certain rules
+test('meson-check',
+ files('src/tools/check_meson'),
+ args: ['--srcdir', meson.source_root(), '--builddir', meson.build_root()],
+ suite: 'source',
+ priority: 10)
+
+
###############################################################
# Pseudo targets
diff --git a/src/tools/check_meson b/src/tools/check_meson
new file mode 100755
index 00000000000..060ccd269ff
--- /dev/null
+++ b/src/tools/check_meson
@@ -0,0 +1,67 @@
+#!/usr/bin/env python3
+
+# Script to perform some consistency checks on the meson build.
+
+from collections import defaultdict
+import argparse
+import json
+import os
+import sys
+
+
+def check_target_names(args):
+ """
+ Check that the target names for run_target() and alias_target() do
+ not conflict with other target types like
+ custom_target(). alias_target()/run_target() targets are intended to
+ be invoked at the top-level, but "meson compile $target" complains
+ about conflicts if another name of the same target exists.
+ """
+ targets_info_path = os.path.join(
+ args.builddir, 'meson-info/intro-targets.json')
+ targets_info = json.load(open(targets_info_path))
+
+ targets_info_byname = defaultdict(list)
+
+ for r in targets_info:
+ targets_info_byname[r['name']].append(r)
+
+ have_conflicts = False
+
+ for name, v in targets_info_byname.items():
+ if len(targets_info_byname[name]) > 1:
+ dirs = set()
+ types = set()
+ have_target_conflict = False
+ for t in v:
+ dirs.add(t['defined_in'])
+ types.add(t['type'])
+ if len(dirs) < len(v) and ('alias' in types or 'run' in types):
+ have_target_conflict = True
+
+ if have_target_conflict:
+ have_conflicts = True
+ print(f'Global target "{name}" has conflicting target names:',
+ file=sys.stderr)
+ for t in v:
+ fname = os.path.relpath(t['defined_in'], args.srcdir)
+ print(f'\t"{t["name"]}:{t["type"]}", defined in "{fname}"',
+ file=sys.stderr)
+ print(file=sys.stderr)
+
+ if have_conflicts:
+ print("Please rename conflicting targets",
+ file=sys.stderr)
+ return False
+ return True
+
+
+if __name__ == '__main__':
+ parser = argparse.ArgumentParser()
+ parser.add_argument('--builddir', type=str, required=True)
+ parser.add_argument('--srcdir', type=str, required=True)
+ args = parser.parse_args()
+
+ all_ok = True
+ all_ok &= check_target_names(args)
+ sys.exit(not all_ok)
--
2.38.0
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-11-29 18:36 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-13 10:12 [PATCH 3/9] Optimize allocations in bringetbitmap Tomas Vondra <[email protected]>
2023-11-23 13:32 Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-* Andrew Dunstan <[email protected]>
2023-11-29 02:28 ` Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-* Andres Freund <[email protected]>
2023-11-29 12:20 ` Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-* Andrew Dunstan <[email protected]>
2023-11-29 13:49 ` Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-* Tom Lane <[email protected]>
2023-11-29 18:36 ` Re: pgsql: meson: docs: Add {html,man} targets, rename install-doc-* 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