From 7d8068f27b6b695f9616fd985517692236dda47d Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 3 Sep 2019 12:30:45 -0400 Subject: [PATCH] Don't decode manifest data when it is generated on a remote. Decoding a manifest from the JSON provided by C to the hash required by Perl is an expensive process. If manifest() was called on a remote it was being decoded into a hash and then immediately re-encoded into JSON for transmission over the protocol layer. Instead, provide a function for the remote to get the raw JSON which can be transmitted as is and decoded in the calling process instead. This makes remote manifest calls as fast as they were before 2.16, but local calls must still pay the decoding penalty and are therefore slower. This will continue to be true until the Perl storage interface is retired at the end of the C migration. Note that for reasonable numbers of tables there is no detectable difference. The case in question involved 250K tables with a 10 minute decode time (which was being doubled) on a fast workstation. --- lib/pgBackRest/Protocol/Remote/Minion.pm | 2 +- lib/pgBackRest/Protocol/Storage/Remote.pm | 7 ++++- lib/pgBackRest/Storage/Storage.pm | 30 ++++++++++++++++++- src/perl/embed.auto.c | 36 +++++++++++++++++++++-- 4 files changed, 69 insertions(+), 6 deletions(-) diff --git a/lib/pgBackRest/Protocol/Remote/Minion.pm b/lib/pgBackRest/Protocol/Remote/Minion.pm index db9106bad..8b042f056 100644 --- a/lib/pgBackRest/Protocol/Remote/Minion.pm +++ b/lib/pgBackRest/Protocol/Remote/Minion.pm @@ -112,7 +112,7 @@ sub init &OP_STORAGE_CIPHER_PASS_USER => sub {$oStorage->cipherPassUser()}, &OP_STORAGE_EXISTS => sub {$oStorage->exists(@{shift()})}, &OP_STORAGE_LIST => sub {$oStorage->list(@{shift()})}, - &OP_STORAGE_MANIFEST => sub {$oStorage->manifest(@{shift()})}, + &OP_STORAGE_MANIFEST => sub {$oStorage->manifestJson(@{shift()})}, &OP_STORAGE_MOVE => sub {$oStorage->move(@{shift()})}, &OP_STORAGE_PATH_GET => sub {$oStorage->pathGet(@{shift()})}, &OP_STORAGE_HASH_SIZE => sub {$oStorage->hashSize(@{shift()})}, diff --git a/lib/pgBackRest/Protocol/Storage/Remote.pm b/lib/pgBackRest/Protocol/Storage/Remote.pm index 0eb626001..d6e378e02 100644 --- a/lib/pgBackRest/Protocol/Storage/Remote.pm +++ b/lib/pgBackRest/Protocol/Storage/Remote.pm @@ -9,6 +9,8 @@ use warnings FATAL => qw(all); use Carp qw(confess); use English '-no_match_vars'; +use JSON::PP; + use pgBackRest::Common::Exception; use pgBackRest::Common::Log; use pgBackRest::Config::Config; @@ -42,6 +44,9 @@ sub new # Set variables $self->{oProtocol} = $oProtocol; + # Create JSON object + $self->{oJSON} = JSON::PP->new()->allow_nonref(); + # Return from function and log return values if any return logDebugReturn ( @@ -163,7 +168,7 @@ sub manifest {name => 'rhParam', required => false}, ); - my $hManifest = $self->{oProtocol}->cmdExecute(OP_STORAGE_MANIFEST, [$strPathExp, $rhParam]); + my $hManifest = $self->{oJSON}->decode($self->{oProtocol}->cmdExecute(OP_STORAGE_MANIFEST, [$strPathExp, $rhParam])); # Return from function and log return values if any return logDebugReturn diff --git a/lib/pgBackRest/Storage/Storage.pm b/lib/pgBackRest/Storage/Storage.pm index 3fe0328ba..4f560fd62 100644 --- a/lib/pgBackRest/Storage/Storage.pm +++ b/lib/pgBackRest/Storage/Storage.pm @@ -400,7 +400,7 @@ sub manifest {name => 'strFilter', optional => true, trace => true}, ); - my $hManifest = $self->{oJSON}->decode($self->{oStorageC}->manifest($strPathExp, $strFilter)); + my $hManifest = $self->{oJSON}->decode($self->manifestJson($strPathExp, {strFilter => $strFilter})); # Return from function and log return values if any return logDebugReturn @@ -410,6 +410,34 @@ sub manifest ); } +sub manifestJson +{ + my $self = shift; + + # Assign function parameters, defaults, and log debug info + my + ( + $strOperation, + $strPathExp, + $strFilter, + ) = + logDebugParam + ( + __PACKAGE__ . '->manifestJson', \@_, + {name => 'strPathExp'}, + {name => 'strFilter', optional => true, trace => true}, + ); + + my $strManifestJson = $self->{oStorageC}->manifest($strPathExp, $strFilter); + + # Return from function and log return values if any + return logDebugReturn + ( + $strOperation, + {name => 'strManifestJson', value => $strManifestJson, trace => true} + ); +} + #################################################################################################################################### # move - move path/file #################################################################################################################################### diff --git a/src/perl/embed.auto.c b/src/perl/embed.auto.c index 6eb327dde..31ac4f865 100644 --- a/src/perl/embed.auto.c +++ b/src/perl/embed.auto.c @@ -11512,7 +11512,7 @@ static const EmbeddedModule embeddedModule[] = "&OP_STORAGE_CIPHER_PASS_USER => sub {$oStorage->cipherPassUser()},\n" "&OP_STORAGE_EXISTS => sub {$oStorage->exists(@{shift()})},\n" "&OP_STORAGE_LIST => sub {$oStorage->list(@{shift()})},\n" - "&OP_STORAGE_MANIFEST => sub {$oStorage->manifest(@{shift()})},\n" + "&OP_STORAGE_MANIFEST => sub {$oStorage->manifestJson(@{shift()})},\n" "&OP_STORAGE_MOVE => sub {$oStorage->move(@{shift()})},\n" "&OP_STORAGE_PATH_GET => sub {$oStorage->pathGet(@{shift()})},\n" "&OP_STORAGE_HASH_SIZE => sub {$oStorage->hashSize(@{shift()})},\n" @@ -11811,6 +11811,8 @@ static const EmbeddedModule embeddedModule[] = "use Carp qw(confess);\n" "use English '-no_match_vars';\n" "\n" + "use JSON::PP;\n" + "\n" "use pgBackRest::Common::Exception;\n" "use pgBackRest::Common::Log;\n" "use pgBackRest::Config::Config;\n" @@ -11838,6 +11840,8 @@ static const EmbeddedModule embeddedModule[] = "\n\n" "$self->{oProtocol} = $oProtocol;\n" "\n\n" + "$self->{oJSON} = JSON::PP->new()->allow_nonref();\n" + "\n\n" "return logDebugReturn\n" "(\n" "$strOperation,\n" @@ -11939,7 +11943,7 @@ static const EmbeddedModule embeddedModule[] = "{name => 'rhParam', required => false},\n" ");\n" "\n" - "my $hManifest = $self->{oProtocol}->cmdExecute(OP_STORAGE_MANIFEST, [$strPathExp, $rhParam]);\n" + "my $hManifest = $self->{oJSON}->decode($self->{oProtocol}->cmdExecute(OP_STORAGE_MANIFEST, [$strPathExp, $rhParam]));\n" "\n\n" "return logDebugReturn\n" "(\n" @@ -14045,7 +14049,7 @@ static const EmbeddedModule embeddedModule[] = "{name => 'strFilter', optional => true, trace => true},\n" ");\n" "\n" - "my $hManifest = $self->{oJSON}->decode($self->{oStorageC}->manifest($strPathExp, $strFilter));\n" + "my $hManifest = $self->{oJSON}->decode($self->manifestJson($strPathExp, {strFilter => $strFilter}));\n" "\n\n" "return logDebugReturn\n" "(\n" @@ -14053,6 +14057,32 @@ static const EmbeddedModule embeddedModule[] = "{name => 'hManifest', value => $hManifest, trace => true}\n" ");\n" "}\n" + "\n" + "sub manifestJson\n" + "{\n" + "my $self = shift;\n" + "\n\n" + "my\n" + "(\n" + "$strOperation,\n" + "$strPathExp,\n" + "$strFilter,\n" + ") =\n" + "logDebugParam\n" + "(\n" + "__PACKAGE__ . '->manifestJson', \\@_,\n" + "{name => 'strPathExp'},\n" + "{name => 'strFilter', optional => true, trace => true},\n" + ");\n" + "\n" + "my $strManifestJson = $self->{oStorageC}->manifest($strPathExp, $strFilter);\n" + "\n\n" + "return logDebugReturn\n" + "(\n" + "$strOperation,\n" + "{name => 'strManifestJson', value => $strManifestJson, trace => true}\n" + ");\n" + "}\n" "\n\n\n\n" "sub move\n" "{\n"