From 41a118f7c204abd4437c2ec47f6b24caafb67044 Mon Sep 17 00:00:00 2001 From: Steffen Siering Date: Fri, 13 Apr 2018 15:15:40 +0200 Subject: [PATCH] MapStr accessor optimisations (#6837) * MapStr accessor optimisations Replace walkMap + interface callbacks with mapFind function, providing all the information required by the different accessor operations. The mapFind function uses a while loop, searching for the next sub-key in the key-path. This is a tail-call optimisation on walkMapRecursive. By searching for the next sub-string and using slices, allocations on key-paths with dots are greatly reduced. Benchmark results: benchmark old ns/op new ns/op delta BenchmarkWalkMap/Get-4 1125 30.8 -97.26% BenchmarkWalkMap/Put-4 890 458 -48.54% BenchmarkWalkMap/PutMissing-4 813 401 -50.68% BenchmarkWalkMap/HasKey-4 565 157 -72.21% BenchmarkWalkMap/HasKeyFirst-4 23.9 12.4 -48.12% BenchmarkWalkMap/Delete-4 860 457 -46.86% benchmark old allocs new allocs delta BenchmarkWalkMap/Get-4 7 0 -100.00% BenchmarkWalkMap/Put-4 10 4 -60.00% BenchmarkWalkMap/PutMissing-4 10 4 -60.00% BenchmarkWalkMap/HasKey-4 5 0 -100.00% BenchmarkWalkMap/HasKeyFirst-4 0 0 +0.00% BenchmarkWalkMap/Delete-4 10 4 -60.00% benchmark old bytes new bytes delta BenchmarkWalkMap/Get-4 432 0 -100.00% BenchmarkWalkMap/Put-4 1120 672 -40.00% BenchmarkWalkMap/PutMissing-4 1120 672 -40.00% BenchmarkWalkMap/HasKey-4 160 0 -100.00% BenchmarkWalkMap/HasKeyFirst-4 0 0 +0.00% BenchmarkWalkMap/Delete-4 1120 672 -40.00% --- libbeat/common/mapstr.go | 181 +++++++++--------------- libbeat/common/safemapstr/safemapstr.go | 81 +++++++---- 2 files changed, 123 insertions(+), 139 deletions(-) diff --git a/libbeat/common/mapstr.go b/libbeat/common/mapstr.go index 93404cdfe98..508bd46c06b 100644 --- a/libbeat/common/mapstr.go +++ b/libbeat/common/mapstr.go @@ -80,20 +80,28 @@ func deepUpdateValue(old interface{}, val MapStr) interface{} { // Delete deletes the given key from the map. func (m MapStr) Delete(key string) error { - _, err := walkMap(key, m, opDelete) - return err + k, d, _, found, err := mapFind(key, m, false) + if err != nil { + return err + } + if !found { + return ErrKeyNotFound + } + + delete(d, k) + return nil } // CopyFieldsTo copies the field specified by key to the given map. It will // overwrite the key if it exists. An error is returned if the key does not // exist in the source map. func (m MapStr) CopyFieldsTo(to MapStr, key string) error { - v, err := walkMap(key, m, opGet) + v, err := m.GetValue(key) if err != nil { return err } - _, err = walkMap(key, to, mapStrOperation{putOperation{v}, true}) + _, err = to.Put(key, v) return err } @@ -115,18 +123,21 @@ func (m MapStr) Clone() MapStr { // HasKey returns true if the key exist. If an error occurs then false is // returned with a non-nil error. func (m MapStr) HasKey(key string) (bool, error) { - hasKey, err := walkMap(key, m, opHasKey) - if err != nil { - return false, err - } - - return hasKey.(bool), nil + _, _, _, hasKey, err := mapFind(key, m, false) + return hasKey, err } // GetValue gets a value from the map. If the key does not exist then an error // is returned. func (m MapStr) GetValue(key string) (interface{}, error) { - return walkMap(key, m, opGet) + _, _, v, found, err := mapFind(key, m, false) + if err != nil { + return nil, err + } + if !found { + return nil, ErrKeyNotFound + } + return v, nil } // Put associates the specified value with the specified key. If the map @@ -138,7 +149,13 @@ func (m MapStr) GetValue(key string) (interface{}, error) { // to insert values (e.g. m[key] = value). func (m MapStr) Put(key string, value interface{}) (interface{}, error) { // XXX `safemapstr.Put` mimics this implementation, both should be updated to have similar behavior - return walkMap(key, m, mapStrOperation{putOperation{value}, true}) + k, d, old, _, err := mapFind(key, m, true) + if err != nil { + return nil, err + } + + d[k] = value + return old, nil } // StringToPrint returns the MapStr as pretty JSON. @@ -316,110 +333,50 @@ func tryToMapStr(v interface{}) (MapStr, bool) { } } -// walkMapRecursive walks the data MapStr to arrive at the value specified by the key. -// The key is expressed in dot-notation (eg. x.y.z). When the key is found then -// the given mapStrOperation is invoked. -func walkMapRecursive(key string, data MapStr, op mapStrOperation) (interface{}, error) { - - // Splits up the key in two parts: full key and first part before the dot - _, exists := data[key] - // If leave node or key exists directly - if exists { - // Execute the mapStrOperator on the leaf object. - return op.Do(key, data) - } - - keyParts := strings.SplitN(key, ".", 2) - // If leave node or key exists directly - if len(keyParts) == 1 { - // Execute the mapStrOperator on the leaf object. - return op.Do(key, data) - } - - // Checks if first part of the key exists - k := keyParts[0] - d, keyExist := data[k] - if !keyExist { - if op.CreateMissingKeys { - d = MapStr{} - data[k] = d - } else { - return nil, ErrKeyNotFound +// mapFind iterates a MapStr based on a the given dotted key, finding the final +// subMap and subKey to operate on. +// An error is returned if some intermediate is no map or the key doesn't exist. +// If createMissing is set to true, intermediate maps are created. +// The final map and un-dotted key to run further operations on are returned in +// subKey and subMap. The subMap already contains a value for subKey, the +// present flag is set to true and the oldValue return will hold +// the original value. +func mapFind( + key string, + data MapStr, + createMissing bool, +) (subKey string, subMap MapStr, oldValue interface{}, present bool, err error) { + // XXX `safemapstr.mapFind` mimics this implementation, both should be updated to have similar behavior + + for { + // Fast path, key is present as is. + if v, exists := data[key]; exists { + return key, data, v, true, nil } - } - - v, err := toMapStr(d) - if err != nil { - return nil, err - } - return walkMapRecursive(keyParts[1], v, op) -} - -func walkMap(key string, data MapStr, op mapStrOperation) (interface{}, error) { - v, err := walkMapRecursive(key, data, op) - if err != nil { - // Add key to error - err = errors.Wrapf(err, "key=%v", key) - } - return v, err -} - -// mapStrOperation types - -// These are static mapStrOperation types that store no state and are reusable. -var ( - opDelete = mapStrOperation{deleteOperation{}, false} - opGet = mapStrOperation{getOperation{}, false} - opHasKey = mapStrOperation{hasKeyOperation{}, false} -) - -// mapStrOperation represents an operation that can be applied to map. -type mapStrOperation struct { - mapStrOperator - CreateMissingKeys bool -} - -// mapStrOperator is an interface with a single function that performs an -// operation on a MapStr. -type mapStrOperator interface { - Do(key string, data MapStr) (value interface{}, err error) -} - -type deleteOperation struct{} + idx := strings.IndexRune(key, '.') + if idx < 0 { + return key, data, nil, false, nil + } -func (op deleteOperation) Do(key string, data MapStr) (interface{}, error) { - value, found := data[key] - if !found { - return nil, ErrKeyNotFound - } - delete(data, key) - return value, nil -} + k := key[:idx] + d, exists := data[k] + if !exists { + if createMissing { + d = MapStr{} + data[k] = d + } else { + return "", nil, nil, false, ErrKeyNotFound + } + } -type getOperation struct{} + v, err := toMapStr(d) + if err != nil { + return "", nil, nil, false, err + } -func (op getOperation) Do(key string, data MapStr) (interface{}, error) { - value, found := data[key] - if !found { - return nil, ErrKeyNotFound + // advance to sub-map + key = key[idx+1:] + data = v } - return value, nil -} - -type hasKeyOperation struct{} - -func (op hasKeyOperation) Do(key string, data MapStr) (interface{}, error) { - _, found := data[key] - return found, nil -} - -type putOperation struct { - Value interface{} -} - -func (op putOperation) Do(key string, data MapStr) (interface{}, error) { - existingValue, _ := data[key] - data[key] = op.Value - return existingValue, nil } diff --git a/libbeat/common/safemapstr/safemapstr.go b/libbeat/common/safemapstr/safemapstr.go index d81aa7b16c1..80b4268bfd3 100644 --- a/libbeat/common/safemapstr/safemapstr.go +++ b/libbeat/common/safemapstr/safemapstr.go @@ -22,39 +22,66 @@ const alternativeKey = "value" // `.value` func Put(data common.MapStr, key string, value interface{}) error { // XXX This implementation mimics `common.MapStr.Put`, both should be updated to have similar behavior - keyParts := strings.SplitN(key, ".", 2) - // If leaf node or key exists directly - if len(keyParts) == 1 { - oldValue, exists := data[key] - if exists { - oldMap, ok := tryToMapStr(oldValue) - if ok { - // This would replace a whole object, change its key to avoid that: - oldMap[alternativeKey] = value - return nil + d, k := mapFind(data, key, alternativeKey) + d[k] = value + return nil +} + +// mapFind walk the map based on the given dotted key and returns the final map +// and key to operate on. This function adds intermediate maps, if the key is +// missing from the original map. + +// mapFind iterates a MapStr based on the given dotted key, finding the final +// subMap and subKey to operate on. +// If a key is already used, but the used value is no map, an intermediate map will be inserted and +// the old value will be stored using the 'alternativeKey' in a new map. +// If the old value found under key is already an dictionary, subMap will be +// the old value and subKey will be set to alternativeKey. +func mapFind(data common.MapStr, key, alternativeKey string) (subMap common.MapStr, subKey string) { + // XXX This implementation mimics `common.mapFind`, both should be updated to have similar behavior + + for { + if oldValue, exists := data[key]; exists { + if oldMap, ok := tryToMapStr(oldValue); ok { + return oldMap, alternativeKey } + return data, key } - data[key] = value - return nil - } - // Checks if first part of the key exists - k := keyParts[0] - d, exists := data[k] - if !exists { - d = common.MapStr{} - data[k] = d - } + idx := strings.IndexRune(key, '.') + if idx < 0 { + // if old value exists and is a dictionary, return the old dictionary and + // make sure we store the new value using the 'alternativeKey' + if oldValue, exists := data[key]; exists { + if oldMap, ok := tryToMapStr(oldValue); ok { + return oldMap, alternativeKey + } + } - v, ok := tryToMapStr(d) - if !ok { - // This would replace a leaf with an object, change its key to avoid that: - v = common.MapStr{alternativeKey: d} - data[k] = v - } + return data, key + } + + // Check if first sub-key exists. Create an intermediate map if not. + k := key[:idx] + d, exists := data[k] + if !exists { + d = common.MapStr{} + data[k] = d + } - return Put(v, keyParts[1], value) + // store old value under 'alternativeKey' if the old value is no map. + // Do not overwrite old value. + v, ok := tryToMapStr(d) + if !ok { + v = common.MapStr{alternativeKey: d} + data[k] = v + } + + // advance into sub-map + key = key[idx+1:] + data = v + } } func tryToMapStr(v interface{}) (common.MapStr, bool) {