fabric@lists.hyperledger.org | Private: Re: [Hyperledger Fabric] [FAB-17598] Improve the efficiency of viperutil.EnhancedExactUnmarshal

Nicholas Basker

#8431

Matt:

Investigated the code and able to see that as you had mentioned getKeysRecursively() and the "viper.ReadInConfig()" have an over-lap. The viper.ReadInConfig() internally uses yaml (gopkg.in/yaml.v2/yaml.go) or json utilities respectively to read the config and give it to getKeysRecursively() which again does a recursive-depth-walk to construct "TopLevel struct".

Its possible to come up with a change to avoid viper, instead use yaml or json to read the file and directly pass to getKeysRecursively() to avoid on recursive-depth-walk. I can make the change.

The getKeysRecursively() function is only used by "EnhancedExactUnmarshal()" which in turn is used only in the following two places.

// EnhancedExactUnmarshal. Cache key is the path of the configuration file that was used.
err := viperutil.EnhancedExactUnmarshal(config, conf)
./internal/configtxgen/genesisconfig/config.go
// EnhancedExactUnmarshal. Cache key is the path of the configuration file that was used.
err := viperutil.EnhancedExactUnmarshal(config, &uconf)
./orderer/common/localconfig/config.go

Is the objective to clean the above to avoid two recursive-depth-walk?

There are other places where viper is referenced but not used along with getKeysRecursively(), so there is no repetition. Is there clean up needed in these as well. (there are a lot of places where viper is used, clearing that seems like a fairly lot of change)

Thanks,

Nicholas.

On Thu, May 21, 2020 at 11:58 PM Matthew Sykes <matthew.sykes@...> wrote:

The test I pointed to will exercise the code. If you're trying to observe what I mean when I say the bug is now the feature:

$ go get github.com/spf13/viper # upgrade to a current version of viper
$ go mod vendor
$ go test -race -count 1 -run TestLoadProfile github.com/hyperledger/fabric/internal/configtxgen/genesisconfig

That test will now fail with a panic because "SampleDevModeKafka" is not present in the "Profiles" map associated with the config. This is related to bug fixes that went into viper in back in October of 2016. [1] if you're asking about the multi-pass nature, you should look at the implementation of `getKeysRecursively` and the viper implementation. There's quite a bit of overlap between the two.

Having said that, the ultimate goal is to remove viper from Fabric. While viper is a fine library, it does a lot of things we don't need and its property based model gets in the way as often as it doesn't - especially given how many of our components are populating value structures for config. So, if you want to pull on that thread, I think it would be a welcome effort.

[1]: https://github.com/spf13/viper/commit/50515b700e02658272117a72bd641b6b7f1222e5

Nicholas Basker

#8493

Matt, All:

As discussed in the thread below, I have raised a draft PR https://github.com/hyperledger/fabric/pull/1392 to decouple "viper" from getKeysRecursive() function. Attached below the test results and improved times.

The PR as it stands is raised to show the changes and get review comments. The code in "execmode := os.Getenv("SKIP_VIPER")" is the changes. The yaml.Unmarshal data could be reused instead of going through viper.Get() which does further recursive calls to viper.searchMap(). Furthermore the entire viper library can be skipped.

Have tested internal/configtxgen/genesisconfig, orderer/common/localconfig and common/viperutil (which are unit tests for viperutil.go).

At this time, the viper references are not cleaned up as it enables me to compare and measure. Based on review, if this change is aligning with the thought of viper-removal, then shall update the PR with full cleanup by removing viper references from these files.

go test -race -count 1 -v -run Test github.com/hyperledger/fabric/internal/configtxgen/genesisconfig | tail -2

PASS

ok github.com/hyperledger/fabric/internal/configtxgen/genesisconfig 10.481s

SKIP_VIPER=yes go test -race -count 1 -v -run Test github.com/hyperledger/fabric/internal/configtxgen/genesisconfig | tail -2

PASS

ok github.com/hyperledger/fabric/internal/configtxgen/genesisconfig 5.922s

go test -race -count 1 -v -run Test github.com/hyperledger/fabric/orderer/common/localconfig | tail -2

PASS

ok github.com/hyperledger/fabric/orderer/common/localconfig 5.639s

SKIP_VIPER=orderer go test -race -count 1 -v -run Test github.com/hyperledger/fabric/orderer/common/localconfig | tail -2

PASS

ok github.com/hyperledger/fabric/orderer/common/localconfig 5.418s

go test -race -count 1 -v -run Test github.com/hyperledger/fabric/common/viperutil | tail -2

PASS

ok github.com/hyperledger/fabric/common/viperutil 5.544s

SKIP_VIPER=viperutil go test -race -count 1 -v -run Test github.com/hyperledger/fabric/common/viperutil | tail -2

PASS

ok github.com/hyperledger/fabric/common/viperutil 5.558s

Thanks.

Show quoted text

On Sun, May 31, 2020 at 10:28 PM Nicholas Basker <nbasker@...> wrote:

Matt:

Investigated the code and able to see that as you had mentioned getKeysRecursively() and the "viper.ReadInConfig()" have an over-lap. The viper.ReadInConfig() internally uses yaml (gopkg.in/yaml.v2/yaml.go) or json utilities respectively to read the config and give it to getKeysRecursively() which again does a recursive-depth-walk to construct "TopLevel struct".

Its possible to come up with a change to avoid viper, instead use yaml or json to read the file and directly pass to getKeysRecursively() to avoid on recursive-depth-walk. I can make the change.
The getKeysRecursively() function is only used by "EnhancedExactUnmarshal()" which in turn is used only in the following two places.
// EnhancedExactUnmarshal. Cache key is the path of the configuration file that was used.
err := viperutil.EnhancedExactUnmarshal(config, conf)
./internal/configtxgen/genesisconfig/config.go
// EnhancedExactUnmarshal. Cache key is the path of the configuration file that was used.
err := viperutil.EnhancedExactUnmarshal(config, &uconf)
./orderer/common/localconfig/config.go

Is the objective to clean the above to avoid two recursive-depth-walk?
There are other places where viper is referenced but not used along with getKeysRecursively(), so there is no repetition. Is there clean up needed in these as well. (there are a lot of places where viper is used, clearing that seems like a fairly lot of change)

Thanks,
Nicholas.

On Thu, May 21, 2020 at 11:58 PM Matthew Sykes <matthew.sykes@...> wrote:
The test I pointed to will exercise the code. If you're trying to observe what I mean when I say the bug is now the feature:

$ go get github.com/spf13/viper # upgrade to a current version of viper
$ go mod vendor
$ go test -race -count 1 -run TestLoadProfile github.com/hyperledger/fabric/internal/configtxgen/genesisconfig

That test will now fail with a panic because "SampleDevModeKafka" is not present in the "Profiles" map associated with the config. This is related to bug fixes that went into viper in back in October of 2016. [1] if you're asking about the multi-pass nature, you should look at the implementation of `getKeysRecursively` and the viper implementation. There's quite a bit of overlap between the two.

Having said that, the ultimate goal is to remove viper from Fabric. While viper is a fine library, it does a lot of things we don't need and its property based model gets in the way as often as it doesn't - especially given how many of our components are populating value structures for config. So, if you want to pull on that thread, I think it would be a welcome effort.

[1]: https://github.com/spf13/viper/commit/50515b700e02658272117a72bd641b6b7f1222e5