Skip to content
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

Increase performance of nested keys search #256

Merged
merged 2 commits into from
Oct 14, 2016
Merged

Conversation

benoitmasson
Copy link
Contributor

This follows PR #253, which closed Issue #249 induced by PR #195.

It generalizes to nested keys the improvement from PR #253 which consists in assuming that all keys are lower-cased, thus allowing to search for values by writing

val = m[key]

instead of

for k, v := range m {
  if strings.ToLower(k) == strings.ToLower(key) {
    val = v
  }
}

This induces a huge performance gain.

Function insensitiviseMap() was also fixed (it did not recurse correctly through the inner maps).

@bep my performance tests show no performance loss (and even very little gain) from your version, can I let you benchmark on your hardware to make sure I didn't remove anything essential (it should be fine, but let's double-check this time...)?

If that's OK for you, I think this would be a more generic solution than your patch, less likely to be bypassed in the future.

All keys (even nested ones) are now lower-cased recursively.

On the way, map[interface{}]interface{} are cast to map[string]interface{}
Removed searchMapForKey(), fast path directly integrated into searchMap() and
searchMapWithPathPrefixes()
=> more generic (searchMapForKey() wasn't called everywhere it should have)

At the same time, significantly speed up searchMap() and searchMapWithPathPrefixes(),
which are still used for nested keys: the assumption that map keys are all
lower-cased allows to perform
    val = m[key]
instead of
    for k, v := range m {
      if strings.ToLower(k) == strings.ToLower(key) {
        val = v
      }
    }
=> i.e., directly access the map instead of enumerate the keys
@bep
Copy link
Collaborator

bep commented Oct 12, 2016

This looks very good in general, but it is slower and more memory hungry for the common case...

▶ benchcmp /tmp/1.bench /tmp/2.bench
benchmark                     old ns/op     new ns/op     delta
BenchmarkGetBool-4            553           562           +1.63%
BenchmarkGet-4                452           477           +5.53%
BenchmarkGetBoolFromMap-4     7.00          6.51          -7.00%

benchmark                     old allocs     new allocs     delta
BenchmarkGetBool-4            4              5              +25.00%
BenchmarkGet-4                4              5              +25.00%
BenchmarkGetBoolFromMap-4     0              0              +0.00%

benchmark                     old bytes     new bytes     delta
BenchmarkGetBool-4            49            65            +32.65%
BenchmarkGet-4                48            64            +33.33%
BenchmarkGetBoolFromMap-4     0             0             +0.00%

So, if you reintroduce my searchMapForKey method the above be fine.

@bep bep merged commit 50515b7 into spf13:master Oct 14, 2016
@bep bep mentioned this pull request Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants