Latest Viper doubles Hugo build time

[bep]

Candidate commit:
ec4eb2f

We are in the process of removing viper.Get* invocations in Hugo hot paths, but I consider this to be a regression bug that should be fixed.

We should also add some benchmarks for the most common functionality so we can spot this earlier.

See gohugoio/hugo#2536

[bep]

Here is the current benchmark effect of that commit:

benchmark                     old ns/op     new ns/op     delta
BenchmarkGetBool-4            469           1001          +113.43%

benchmark                     old allocs     new allocs     delta
BenchmarkGetBool-4            3              6              +100.00%

benchmark                     old bytes     new bytes     delta
BenchmarkGetBool-4            33            113           +242.42%

That benchmark is as simple as it gets:

https://github.com/spf13/viper/blob/master/viper_test.go#L957

1001 ns is too much for a map lookup.

[benoitmasson]

Sorry about that... Though I must admit I didn't try to measure it before, I'm not surprised that the new code for find() slows down the Get*() process, due to the intrinsic complexity of “merging” deeply nested configuration layers.

On my computer, the performance loss is not that huge, it is around +25%:

# old
> go test -bench . -benchtime 10s -cpu 1,4 github.com/spf13/viper
BenchmarkGetBool                 3000000          4243 ns/op
BenchmarkGetBool-4               3000000          5292 ns/op

# new
> go test -bench . -benchtime 10s -cpu 1,4 github.com/spf13/viper
BenchmarkGetBool                 3000000          5279 ns/op
BenchmarkGetBool-4               2000000          6767 ns/op

However, depth-1 keys (without ".") should be dealt with as efficiently as before, since there are no recursive calls for them (reason why I don't think a specific “fast path” would be necessary). I'll investigate why this is not the case. As for more “complex” keys, complexity should be linear in terms of key depth.

On the other hand, the AllKeys() and AllSettings() functions (and thus, Unmarshal() too) should be much slower than they used to be (again, I didn't measure how much slower). I don't think that's a big issue, because they are typically run only once at program startup.

[bep]

With those numbers (5k ns/op), your computer isn't representative for the PCs in the wild. And it doesn't really matter.

The All* funcs doesn't matter. The common case of .Get("SimpleKey") should not need the deep map tests and should be as fast as before.

I believe other people than me have been "bitten" by this, so this should be fixed sooner rather than later.

@benoitmasson Could you create a patch?

[awfm9]

I agree that this is undesirable, but I also think that proper semantics of nested maps takes precedence over performance. Viper is rarely the bottleneck for applications. Let's see if @benoitmasson can come up with some optimizations.

[bep]

I agree that this is undesirable, but I also think that proper semantics of nested maps takes precedence over performance.

Let us have a discussion about that next time. We do vendor the dependencies in Hugo, but we only commit the vendor file -- so people not using the release and just doing something similar to go get will now get a version that is 100% slower -- and that is really bad when we just two days ago went high and shiny on Reddit and Hackernews and bragged about the opposite.

If I don't see a proper fix for this today I'm going to revert the commit in question, and reopen the PR so we can do a proper code review.

[benoitmasson]

If I don't see a proper fix for this today I'm going to revert the commit in question, and reopen the PR so we can do a proper code review.

Today I won't have time for that, but I can have a look tomorrow evening if that's still OK.

[bep]

I'll see if I can fix it myself.

[awfm9]

Thank you @bep - however in regards to your comment, if you vendor dependencies in Hugo, the easier course of action would have been to stay on a previous version of Viper until the issue is fixed, no?

[bep]

if you vendor dependencies in Hugo, the easier course of action would have been to stay on a previous version of Viper until the issue is fixed, no?

No. I think I have explained above.

[bep]

... in general I wouldn't care too much about this, but the timing of this regression was incredibly bad for Hugo.

[awfm9]

I understand your frustration, but the reason to vendor is exactly to avoid this kind of regression, isn't it?
Wouldn't it make sense to at least include instructions for the use of govendor (or the like) in your README?
I agree that I should run benchmarks in the future before merging big PRs like this.

[bep]

Hugo has taken a litle half-assed approach to vendoring just to test it out. This would not have been a problem if we didn't put the vendor folder in .gitignore. If you build Hugo with make install you get vendoring, but not all do that -- Homebrew for one, I suspect.

[spf13]

The big problem was that the current approach to dependencies isn't really
vendoring but locking versions based on git references. In this case
someone did a git push --force and the referenced hash no longer existed.

True vendoring would resolve this but it would also bring a lot of
disadvantages along with it.
On Tue, Oct 11, 2016 at 2:30 AM Bjørn Erik Pedersen <
notifications@github.com> wrote:

Hugo has taken a litle half-assed approach to vendoring just to test it
out. This would not have been a problem if we didn't put the vendor
folder in .gitignore.If you build Hugo with make you get vendoring, but
not all do that -- Homebrew for one, I suspect.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#249 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAKlZJo6dza69paiTJIAUpQfkKOGoSzHks5qyy0MgaJpZM4KRxdR
.