New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use viper.Get* in tight loops #2495

Closed
bep opened this Issue Sep 25, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@bep
Member

bep commented Sep 25, 2016

Profiling indicate that using viper.GetBool inside the url funcs (MakePath etc.) have a surprising cost.

Just removing one viper.Get temporary from MakePathSanitized:

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     20347856514     17916615991     -11.95%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     66611825       60243953       -9.56%

benchmark           old bytes       new bytes       delta
BenchmarkHugo-4     10282345656     10122546104     -1.55%
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 26, 2016

Member

I have "suggested" this for the 0.17 release -- but it is probably a more natural fit for the next ... My PR isn't entirely finished, but I will wrap it up during the day.

I'm not sure why Viper allocates memory on a simple GetBool (I will look into that), but we should anyhow avoid doing lookups in tight loops (I didn't consider the URL funcs to be in "tight loops").

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     32771543767     20956368681     -36.05%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     66611886       51747681       -22.31%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     10272277160     9906356584     -3.56%

/cc @spf13

Member

bep commented Sep 26, 2016

I have "suggested" this for the 0.17 release -- but it is probably a more natural fit for the next ... My PR isn't entirely finished, but I will wrap it up during the day.

I'm not sure why Viper allocates memory on a simple GetBool (I will look into that), but we should anyhow avoid doing lookups in tight loops (I didn't consider the URL funcs to be in "tight loops").

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     32771543767     20956368681     -36.05%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     66611886       51747681       -22.31%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     10272277160     9906356584     -3.56%

/cc @spf13

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 26, 2016

Member

Also see spf13/viper#242

Member

bep commented Sep 26, 2016

Also see spf13/viper#242

bep added a commit that referenced this issue Sep 26, 2016

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 26, 2016

Member

With the fix in cast, the win is less, but still significant:

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     24116936011     19791183884     -17.94%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     60902955       50003607       -17.90%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     10121519632     9857226960     -2.61%
Member

bep commented Sep 26, 2016

With the fix in cast, the win is less, but still significant:

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     24116936011     19791183884     -17.94%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     60902955       50003607       -17.90%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     10121519632     9857226960     -2.61%
@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Sep 26, 2016

Contributor

I like your PathSpec approach. I was thinking along the same lines when I first saw this issue, but with your fix in cast, I'd say we hold this change for 0.18.

Contributor

moorereason commented Sep 26, 2016

I like your PathSpec approach. I was thinking along the same lines when I first saw this issue, but with your fix in cast, I'd say we hold this change for 0.18.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 26, 2016

Member

I have another simple fix for Viper coming that will improve this even further.

Member

bep commented Sep 26, 2016

I have another simple fix for Viper coming that will improve this even further.

@bep bep modified the milestones: v0.18, v0.17 Sep 26, 2016

bep added a commit that referenced this issue Sep 26, 2016

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 26, 2016

Member

This is the delta after the latest Viper update:

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     14465408023     13147573109     -9.11%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     55147163       48210421       -12.58%

benchmark           old bytes      new bytes      delta
BenchmarkHugo-4     9998583512     9815322448     -1.83%

Still worth doing, but not super-important anymore.

Member

bep commented Sep 26, 2016

This is the delta after the latest Viper update:

benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     14465408023     13147573109     -9.11%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     55147163       48210421       -12.58%

benchmark           old bytes      new bytes      delta
BenchmarkHugo-4     9998583512     9815322448     -1.83%

Still worth doing, but not super-important anymore.

bep added a commit to bep/hugo that referenced this issue Oct 24, 2016

Avoid reading from Viper for path and URL funcs
The gain, given the "real sites benchmark" below, is obvious:

```
benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     14497594101     13084156335     -9.75%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     57404335       48282002       -15.89%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     9933505624      9721984424     -2.13%
```

Fixes #2495

@bep bep closed this in #2496 Oct 24, 2016

bep added a commit that referenced this issue Oct 24, 2016

Avoid reading from Viper for path and URL funcs
The gain, given the "real sites benchmark" below, is obvious:

```
benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     14497594101     13084156335     -9.75%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     57404335       48282002       -15.89%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     9933505624      9721984424     -2.13%
```

Fixes #2495

tychoish added a commit to tychoish/hugo that referenced this issue Aug 13, 2017

tychoish added a commit to tychoish/hugo that referenced this issue Aug 13, 2017

tychoish added a commit to tychoish/hugo that referenced this issue Aug 13, 2017

Avoid reading from Viper for path and URL funcs
The gain, given the "real sites benchmark" below, is obvious:

```
benchmark           old ns/op       new ns/op       delta
BenchmarkHugo-4     14497594101     13084156335     -9.75%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     57404335       48282002       -15.89%

benchmark           old bytes       new bytes      delta
BenchmarkHugo-4     9933505624      9721984424     -2.13%
```

Fixes #2495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment