From 2dfe8a94a8bea160e9cac512f0d3346fdc90cf44 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 2 Oct 2017 14:27:05 -0700 Subject: [PATCH 2/2] Replace binary search in fmgr_isbuiltin with a lookup array. Turns out we have enough functions that the binary search is quite noticeable in profiles. Thus have Gen_fmgrtab.pl build a new mapping from a builtin function's oid to an index in the existing fmgr_builtins array. That keeps the additional memory usage at a reasonable amount. Author: Andres Freund Discussion: https://postgr.es/m/20170914065128.a5sk7z4xde5uy3ei@alap3.anarazel.de --- src/backend/utils/Gen_fmgrtab.pl | 84 +++++++++++++++++++++++++++++++++------- src/backend/utils/fmgr/fmgr.c | 27 ++++++------- src/include/utils/fmgrtab.h | 8 ++++ 3 files changed, 90 insertions(+), 29 deletions(-) diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl index 17851fe2a4..b8821798e6 100644 --- a/src/backend/utils/Gen_fmgrtab.pl +++ b/src/backend/utils/Gen_fmgrtab.pl @@ -21,6 +21,8 @@ use warnings; # Collect arguments my $infile; # pg_proc.h my $output_path = ''; +my @include_path; + while (@ARGV) { my $arg = shift @ARGV; @@ -32,6 +34,10 @@ while (@ARGV) { $output_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV; } + elsif ($arg =~ /^-I/) + { + push @include_path, length($arg) > 2 ? substr($arg, 2) : shift @ARGV; + } else { usage(); @@ -44,6 +50,13 @@ if ($output_path ne '' && substr($output_path, -1) ne '/') $output_path .= '/'; } +# Sanity check arguments. +die "No input files.\n" if !$infile; +die "No include path; you must specify -I at least once.\n" if !@include_path; + +my $FirstBootstrapObjectId = + Catalog::FindDefinedSymbol('access/transam.h', \@include_path, 'FirstBootstrapObjectId'); + # Read all the data from the include/catalog files. my $catalogs = Catalog::Catalogs($infile); @@ -176,6 +189,7 @@ qq|/*------------------------------------------------------------------------- #include "postgres.h" +#include "access/transam.h" #include "utils/fmgrtab.h" #include "utils/fmgrprotos.h" @@ -191,32 +205,76 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr) print $pfh "extern Datum $s->{prosrc}(PG_FUNCTION_ARGS);\n"; } -# Create the fmgr_builtins table +# Create the fmgr_builtins table, collect data for fmgr_builtin_oid_index print $tfh "\nconst FmgrBuiltin fmgr_builtins[] = {\n"; my %bmap; $bmap{'t'} = 'true'; $bmap{'f'} = 'false'; +my $fmgr_builtin_max_oid = -1; +my @fmgr_builtin_oid_index; +my $fmgr_count = 0; foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr) { print $tfh -" { $s->{oid}, \"$s->{prosrc}\", $s->{nargs}, $bmap{$s->{strict}}, $bmap{$s->{retset}}, $s->{prosrc} },\n"; +" { $s->{oid}, \"$s->{prosrc}\", $s->{nargs}, $bmap{$s->{strict}}, $bmap{$s->{retset}}, $s->{prosrc} }"; + + if ($s->{oid} > $fmgr_builtin_max_oid) + { + $fmgr_builtin_max_oid = $s->{oid}; + } + + $fmgr_builtin_oid_index[$s->{oid}] = $fmgr_count++; + + if ($fmgr_count <= $#fmgr) + { + print $tfh ",\n"; + } + else + { + print $tfh "\n"; + } } +print $tfh "};"; + +print $tfh qq| +const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin)); + +const Oid fmgr_builtin_max_oid = $fmgr_builtin_max_oid; + +|; + +# Create fmgr_builtins_oid_index table. +# +# Note that the array has to be filled up to FirstBootstrapObjectId, +# as we can't rely on zero initialization as 0 is a valid mapping. +print $tfh "const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId] = {\n"; +for (my $i = 0; $i < $FirstBootstrapObjectId; $i++) +{ + my $oid = $fmgr_builtin_oid_index[$i]; + + # fmgr_builtin_oid_index is sparse, map nonexistant functions to + # InvalidOidBuiltinMapping + if (not defined $oid) + { + $oid = 'InvalidOidBuiltinMapping'; + } + + if ($i + 1 == $FirstBootstrapObjectId) + { + print $tfh " $oid\n"; + } + else + { + print $tfh " $oid,\n"; + } +} +print $tfh "};"; + # And add the file footers. print $ofh "\n#endif /* FMGROIDS_H */\n"; print $pfh "\n#endif /* FMGRPROTOS_H */\n"; -print $tfh -qq| /* dummy entry is easier than getting rid of comma after last real one */ - /* (not that there has ever been anything wrong with *having* a - comma after the last field in an array initializer) */ - { 0, NULL, 0, false, false, NULL } -}; - -/* Note fmgr_nbuiltins excludes the dummy entry */ -const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin)) - 1; -|; - close($ofh); close($pfh); close($tfh); diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index 919733517b..971ea40bee 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -70,26 +70,21 @@ static Datum fmgr_security_definer(PG_FUNCTION_ARGS); static const FmgrBuiltin * fmgr_isbuiltin(Oid id) { - int low = 0; - int high = fmgr_nbuiltins - 1; + int16 index; + + /* fast lookup only possible if original oid still assigned */ + if (id >= FirstBootstrapObjectId) + return NULL; /* - * Loop invariant: low is the first index that could contain target entry, - * and high is the last index that could contain it. + * Lookup function data. If there's a miss in that range it's likely a + * nonexistant function, returning NULL here will trigger an ERROR later. */ - while (low <= high) - { - int i = (high + low) / 2; - const FmgrBuiltin *ptr = &fmgr_builtins[i]; + index = fmgr_builtin_oid_index[id]; + if (index == InvalidOidBuiltinMapping) + return NULL; - if (id == ptr->foid) - return ptr; - else if (id > ptr->foid) - low = i + 1; - else - high = i - 1; - } - return NULL; + return &fmgr_builtins[index]; } /* diff --git a/src/include/utils/fmgrtab.h b/src/include/utils/fmgrtab.h index 6130ef8f9c..dbd2c0b8e1 100644 --- a/src/include/utils/fmgrtab.h +++ b/src/include/utils/fmgrtab.h @@ -13,6 +13,7 @@ #ifndef FMGRTAB_H #define FMGRTAB_H +#include "access/transam.h" #include "fmgr.h" @@ -36,4 +37,11 @@ extern const FmgrBuiltin fmgr_builtins[]; extern const int fmgr_nbuiltins; /* number of entries in table */ +/* + * Mapping from a builtin function's oid to the index in the fmgr_builtins + * array. + */ +#define InvalidOidBuiltinMapping -1 +extern const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId]; + #endif /* FMGRTAB_H */ -- 2.14.1.536.g6867272d5b.dirty