From: ilmari@ilmari.org
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andreas Karlsson <andreas@proxel.se>
Cc: pgsql-hackers@postgreSQL.org, Andres Freund <andres@anarazel.de>
Subject: Re: Cutting initdb's runtime (Perl question embedded)
Date: Mon, 17 Apr 2017 17:49:54 +0100
Message-ID: <d8jpogbq8r1.fsf@dalvik.ping.uio.no> (raw)
In-Reply-To: <6312.1492100010@sss.pgh.pa.us>
References: <30896.1492006367@sss.pgh.pa.us>
<a15e94ac-c346-5816-69ae-ec6811e32af1@proxel.se>
<972d541d-7285-53ff-ea71-dcbfd886570a@proxel.se>
<6312.1492100010@sss.pgh.pa.us>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Tom Lane <tgl@sss.pgh.pa.us> writes:
> There's certainly lots more that could be done in the genbki code,
> but I think all we can justify at this stage of the development
> cycle is to get the low-hanging fruit for testing speedups.
I threw Devel::NYTProf at it and picked some more low-hanging fruit.
Attached are separate patches for each change, and here are the runtimes
of genbki.pl and Gen_fmgrtab.pl, respectively, after each patch
(averages of 5 runs, in millseconds):
master (b6dd1271): 355, 182
1: Avoid unnecessary regex captures: 349, 183
2: Avoid repeated calls to SplitDataLine: 316, 158
3: Inline SplitDataLine: 291, 141
4: Inline check_natts: 287, 141
Together they shave 68ms or 19.2% off the runtime of genbki.pl and 41ms
or 22.5% off the runtime of Gen_fmgrtab.pl
Finally, one non-performance patch, which just removes the use of
Exporter in Catalog.pm, since none of the users actually import anything
from it.
- ilmari
--
"I use RMS as a guide in the same way that a boat captain would use
a lighthouse. It's good to know where it is, but you generally
don't want to find yourself in the same spot." - Tollef Fog Heen
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[text/x-diff] 0001-Avoid-unnecessary-regex-captures-in-Catalog.pm.patch (1.2K, ../d8jpogbq8r1.fsf@dalvik.ping.uio.no/2-0001-Avoid-unnecessary-regex-captures-in-Catalog.pm.patch)
download | inline diff:
From 5d7f4b1e78243daa6939501b88b2644bfcf9bd77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 17 Apr 2017 13:26:35 +0100
Subject: [PATCH 1/8] Avoid unnecessary regex captures in Catalog.pm
---
src/backend/catalog/Catalog.pm | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pmindex fa8de04..e7b647a 100644--- a/src/backend/catalog/Catalog.pm+++ b/src/backend/catalog/Catalog.pm@@ -54,7 +54,7 @@ sub Catalogs
{
# Strip C-style comments.
- s;/\*(.|\n)*\*/;;g;+ s;/\*(?:.|\n)*\*/;;g;
if (m;/\*;)
{
@@ -80,12 +80,12 @@ sub Catalogs
{
$catalog{natts} = $1;
}
- elsif (/^DATA\(insert(\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)+ elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
{
- check_natts($filename, $catalog{natts}, $3,+ check_natts($filename, $catalog{natts}, $2,
$input_file, $input_line_number);
- push @{ $catalog{data} }, { oid => $2, bki_values => $3 };+ push @{ $catalog{data} }, { oid => $1, bki_values => $2 };
}
elsif (/^DESCR\(\"(.*)\"\)$/)
{
--
2.7.4
[text/x-diff] 0002-Avoid-repeated-calls-to-Catalog-SplitDataLine.patch (3.2K, ../d8jpogbq8r1.fsf@dalvik.ping.uio.no/3-0002-Avoid-repeated-calls-to-Catalog-SplitDataLine.patch)
download | inline diff:
From 92402e0306eda209b1a2811acc41325d75add0cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 17 Apr 2017 14:04:20 +0100
Subject: [PATCH 2/8] Avoid repeated calls to Catalog::SplitDataLine
Both check_natts and the callers of Catalog::Catalogs were calling
Catalog::SplitDataLines, the former discarding the result.
Instead, have Catalog::Catalogs store the split fields directly and pass
the count to check_natts
---
src/backend/catalog/Catalog.pm | 14 +++++++-------
src/backend/catalog/genbki.pl | 3 +--
src/backend/utils/Gen_fmgrtab.pl | 3 +--
3 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pmindex e7b647a..0c508b0 100644--- a/src/backend/catalog/Catalog.pm+++ b/src/backend/catalog/Catalog.pm@@ -82,10 +82,12 @@ sub Catalogs
}
elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
{
- check_natts($filename, $catalog{natts}, $2,+ my @bki_values = SplitDataLine($2);++ check_natts($filename, $catalog{natts}, scalar(@bki_values),
$input_file, $input_line_number);
- push @{ $catalog{data} }, { oid => $1, bki_values => $2 };+ push @{ $catalog{data} }, { oid => $1, bki_values => \@bki_values };
}
elsif (/^DESCR\(\"(.*)\"\)$/)
{
@@ -254,17 +256,15 @@ sub RenameTempFile
# verify the number of fields in the passed-in DATA line
sub check_natts
{
- my ($catname, $natts, $bki_val, $file, $line) = @_;+ my ($catname, $natts, $bki_vals, $file, $line) = @_;
die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"
unless defined $natts;
- my $nfields = scalar(SplitDataLine($bki_val));-
die sprintf
"Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",
- $file, $line, $natts, $nfields- unless $natts == $nfields;+ $file, $line, $natts, $bki_vals+ unless $natts == $bki_vals;
}
1;
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.plindex 198e072..8875f6c 100644--- a/src/backend/catalog/genbki.pl+++ b/src/backend/catalog/genbki.pl@@ -161,9 +161,8 @@ foreach my $catname (@{ $catalogs->{names} })
foreach my $row (@{ $catalog->{data} })
{
- # Split line into tokens without interpreting their meaning.
my %bki_values;
- @bki_values{@attnames} = Catalog::SplitDataLine($row->{bki_values});+ @bki_values{@attnames} = @{$row->{bki_values}};
# Perform required substitutions on fields
foreach my $att (keys %bki_values)
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.plindex 76bdf5c..d2c4617 100644--- a/src/backend/utils/Gen_fmgrtab.pl+++ b/src/backend/utils/Gen_fmgrtab.pl@@ -58,9 +58,8 @@ foreach my $column (@{ $catalogs->{pg_proc}->{columns} })
my $data = $catalogs->{pg_proc}->{data};
foreach my $row (@$data)
{
- # Split line into tokens without interpreting their meaning.
my %bki_values;
- @bki_values{@attnames} = Catalog::SplitDataLine($row->{bki_values});+ @bki_values{@attnames} = @{$row->{bki_values}};
# Select out just the rows for internal-language procedures.
# Note assumption here that INTERNALlanguageId is 12.
--
2.7.4
[text/x-diff] 0003-Inline-SplitDataLines-into-its-only-caller.patch (2.8K, ../d8jpogbq8r1.fsf@dalvik.ping.uio.no/4-0003-Inline-SplitDataLines-into-its-only-caller.patch)
download | inline diff:
From c8e55bd9ff36029c3f4fe7053f54f9f862c79d7e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 17 Apr 2017 14:47:06 +0100
Subject: [PATCH 3/8] Inline SplitDataLines into its only caller
---
src/backend/catalog/Catalog.pm | 38 ++++++++++++++------------------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pmindex 0c508b0..9bd6263 100644--- a/src/backend/catalog/Catalog.pm+++ b/src/backend/catalog/Catalog.pm@@ -82,12 +82,24 @@ sub Catalogs
}
elsif (/^DATA\(insert(?:\s+OID\s+=\s+(\d+))?\s+\(\s*(.*)\s*\)\s*\)$/)
{
- my @bki_values = SplitDataLine($2);+ # The field-splitting regex will clobber $1, save it+ my $oid = $1;++ # This handling of quoted strings might look too+ # simplistic, but it matches what bootscanner.l does:+ # that has no provision for quote marks inside quoted+ # strings, either. If we don't have a quoted string,+ # just snarf everything till next whitespace. That will+ # accept some things that bootscanner.l will see as+ # erroneous tokens; but it seems wiser to do that and+ # let bootscanner.l complain than to silently drop+ # non-whitespace characters.+ my @bki_values = $2 =~ /"[^"]*"|\S+/g;
check_natts($filename, $catalog{natts}, scalar(@bki_values),
$input_file, $input_line_number);
- push @{ $catalog{data} }, { oid => $1, bki_values => \@bki_values };+ push @{ $catalog{data} }, { oid => $oid, bki_values => \@bki_values };
}
elsif (/^DESCR\(\"(.*)\"\)$/)
{
@@ -218,28 +230,6 @@ sub Catalogs
return \%catalogs;
}
-# Split a DATA line into fields.-# Call this on the bki_values element of a DATA item returned by Catalogs();-# it returns a list of field values. We don't strip quoting from the fields.-# Note: it should be safe to assign the result to a list of length equal to-# the nominal number of catalog fields, because check_natts already checked-# the number of fields.-sub SplitDataLine-{- my $bki_values = shift;-- # This handling of quoted strings might look too simplistic, but it- # matches what bootscanner.l does: that has no provision for quote marks- # inside quoted strings, either. If we don't have a quoted string, just- # snarf everything till next whitespace. That will accept some things- # that bootscanner.l will see as erroneous tokens; but it seems wiser- # to do that and let bootscanner.l complain than to silently drop- # non-whitespace characters.- my @result = $bki_values =~ /"[^"]*"|\S+/g;-- return @result;-}-
# Rename temporary files to final names.
# Call this function with the final file name and the .tmp extension
# Note: recommended extension is ".tmp$$", so that parallel make steps
--
2.7.4
[text/x-diff] 0004-Inline-check_nattrs-into-its-only-caller.patch (1.8K, ../d8jpogbq8r1.fsf@dalvik.ping.uio.no/5-0004-Inline-check_nattrs-into-its-only-caller.patch)
download | inline diff:
From fc6c69a692002277378308c2f6ca019185ef9fbb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 17 Apr 2017 14:38:22 +0100
Subject: [PATCH 4/8] Inline check_nattrs into its only caller
---
src/backend/catalog/Catalog.pm | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pmindex 9bd6263..0aa3e0c 100644--- a/src/backend/catalog/Catalog.pm+++ b/src/backend/catalog/Catalog.pm@@ -96,8 +96,14 @@ sub Catalogs
# non-whitespace characters.
my @bki_values = $2 =~ /"[^"]*"|\S+/g;
- check_natts($filename, $catalog{natts}, scalar(@bki_values),- $input_file, $input_line_number);+ # Check that the DATA line matches the declared number of attributes+ die "Could not find definition for Natts_${catname} before start of DATA() in $filename\n"+ unless defined $catalog{natts};++ die sprintf+ "Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",+ $filename, $input_line_number, $catalog{natts}, scalar(@bki_values)+ unless $catalog{natts} == @bki_values;
push @{ $catalog{data} }, { oid => $oid, bki_values => \@bki_values };
}
@@ -243,18 +249,4 @@ sub RenameTempFile
rename($temp_name, $final_name) || die "rename: $temp_name: $!";
}
-# verify the number of fields in the passed-in DATA line-sub check_natts-{- my ($catname, $natts, $bki_vals, $file, $line) = @_;-- die "Could not find definition for Natts_${catname} before start of DATA() in $file\n"- unless defined $natts;-- die sprintf- "Wrong number of attributes in DATA() entry at %s:%d (expected %d but got %d)\n",- $file, $line, $natts, $bki_vals- unless $natts == $bki_vals;-}-
1;
--
2.7.4
[text/x-diff] 0005-Remove-pointless-Exporter-usage-in-Catalog.pm.patch (986B, ../d8jpogbq8r1.fsf@dalvik.ping.uio.no/6-0005-Remove-pointless-Exporter-usage-in-Catalog.pm.patch)
download | inline diff:
From 592dbc64d21066f5da43d535af2cb3780b95d006 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 17 Apr 2017 15:57:18 +0100
Subject: [PATCH 6/8] Remove pointless Exporter usage in Catalog.pm
None of the users actually import any of the subroutines, they just call
them by their fully-qualified names.
---
src/backend/catalog/Catalog.pm | 5 -----
1 file changed, 5 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pmindex b326427..d4b4170 100644--- a/src/backend/catalog/Catalog.pm+++ b/src/backend/catalog/Catalog.pm@@ -16,11 +16,6 @@ package Catalog;
use strict;
use warnings;
-require Exporter;-our @ISA = qw(Exporter);-our @EXPORT = ();-our @EXPORT_OK = qw(Catalogs SplitDataLine RenameTempFile);-
# Call this function with an array of names of header files to parse.
# Returns a nested data structure describing the data in the headers.
sub Catalogs
--
2.7.4
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilmari@ilmari.org, tgl@sss.pgh.pa.us, andreas@proxel.se, andres@anarazel.de
Subject: Re: Cutting initdb's runtime (Perl question embedded)
In-Reply-To: <d8jpogbq8r1.fsf@dalvik.ping.uio.no>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox