Change Set internals and make (extreme) performance improvements

Changing the Set internals makes a lot of sense as it saves doing
conversions in multiple places and gives a central place to alter
the key when a item is computed.

This will have no side effects other then that the ordering is now
based on strings instead on integers, so the order will be different.
This will however have no effect on existing configs as these will
use the individual codes/keys and not the ordering to determine if
there is a diff or not.

Lastly (but I think also most importantly) there is a fix in this PR
that makes diffing sets extremely more performand. Before a full diff
required reading the complete Set for every single parameter/attribute
you wanted to diff, while now it only gets that specific parameter.

We have a use case where we have a Set that has 18 parameters and the
set consist of about 600 items (don't ask 😉). So when doing a diff
it would take 100% CPU of all cores and stay that way for almost an
hour before being able to complete the diff.

Debugging this we learned that for retrieving every single parameter
it made over 52.000 calls to `func (c *ResourceConfig) get(..)`. In
this function a slice is created and used only for the duration of the
call, so the time needed to create all needed slices and on the other
hand the time the garbage collector needed to clean them up again caused
the system to cripple itself. Next to that there are also some expensive
reflect calls in this function which also claimed a fair amount of CPU
time.

After this fix the number of calls needed to get a single parameter
dropped from 52.000+ to only 2! 😃

helper/schema/field_reader_config.go

@@ -18,10 +18,12 @@ type ConfigFieldReader struct {
 	Config *terraform.ResourceConfig
 	Schema map[string]*Schema
 
-	lock sync.Mutex
+	indexMaps map[string]map[string]int
+	once      sync.Once
 }
 
 func (r *ConfigFieldReader) ReadField(address []string) (FieldReadResult, error) {
+	r.once.Do(func() { r.indexMaps = make(map[string]map[string]int) })
 	return r.readField(address, false)
 }
 
@@ -55,20 +57,18 @@ func (r *ConfigFieldReader) readField(
 				continue
 			}
 
-			// Get the code
-			code, err := strconv.ParseInt(address[i+1], 0, 0)
-			if err != nil {
-				return FieldReadResult{}, err
-			}
-
+			indexMap, ok := r.indexMaps[strings.Join(address[:i+1], ".")]
+			if !ok {
 				// Get the set so we can get the index map that tells us the
 				// mapping of the hash code to the list index
-			_, indexMap, err := r.readSet(address[:i+1], v)
+				_, err := r.readSet(address[:i+1], v)
 				if err != nil {
 					return FieldReadResult{}, err
 				}
+				indexMap = r.indexMaps[strings.Join(address[:i+1], ".")]
+			}
 
-			index, ok := indexMap[int(code)]
+			index, ok := indexMap[address[i+1]]
 			if !ok {
 				return FieldReadResult{}, nil
 			}
@@ -87,8 +87,7 @@ func (r *ConfigFieldReader) readField(
 	case TypeMap:
 		return r.readMap(k)
 	case TypeSet:
-		result, _, err := r.readSet(address, schema)
-		return result, err
+		return r.readSet(address, schema)
 	case typeObject:
 		return readObjectField(
 			&nestedConfigFieldReader{r},
@@ -112,7 +111,7 @@ func (r *ConfigFieldReader) readMap(k string) (FieldReadResult, error) {
 	switch m := mraw.(type) {
 	case []interface{}:
 		for i, innerRaw := range m {
-			for ik, _ := range innerRaw.(map[string]interface{}) {
+			for ik := range innerRaw.(map[string]interface{}) {
 				key := fmt.Sprintf("%s.%d.%s", k, i, ik)
 				if r.Config.IsComputed(key) {
 					computed = true
@@ -125,7 +124,7 @@ func (r *ConfigFieldReader) readMap(k string) (FieldReadResult, error) {
 		}
 	case []map[string]interface{}:
 		for i, innerRaw := range m {
-			for ik, _ := range innerRaw {
+			for ik := range innerRaw {
 				key := fmt.Sprintf("%s.%d.%s", k, i, ik)
 				if r.Config.IsComputed(key) {
 					computed = true
@@ -137,7 +136,7 @@ func (r *ConfigFieldReader) readMap(k string) (FieldReadResult, error) {
 			}
 		}
 	case map[string]interface{}:
-		for ik, _ := range m {
+		for ik := range m {
 			key := fmt.Sprintf("%s.%s", k, ik)
 			if r.Config.IsComputed(key) {
 				computed = true
@@ -198,17 +197,17 @@ func (r *ConfigFieldReader) readPrimitive(
 }
 
 func (r *ConfigFieldReader) readSet(
-	address []string, schema *Schema) (FieldReadResult, map[int]int, error) {
-	indexMap := make(map[int]int)
+	address []string, schema *Schema) (FieldReadResult, error) {
+	indexMap := make(map[string]int)
 	// Create the set that will be our result
 	set := schema.ZeroValue().(*Set)
 
 	raw, err := readListField(&nestedConfigFieldReader{r}, address, schema)
 	if err != nil {
-		return FieldReadResult{}, indexMap, err
+		return FieldReadResult{}, err
 	}
 	if !raw.Exists {
-		return FieldReadResult{Value: set}, indexMap, nil
+		return FieldReadResult{Value: set}, nil
 	}
 
 	// If the list is computed, the set is necessarilly computed
@@ -217,7 +216,7 @@ func (r *ConfigFieldReader) readSet(
 			Value:    set,
 			Exists:   true,
 			Computed: raw.Computed,
-		}, indexMap, nil
+		}, nil
 	}
 
 	// Build up the set from the list elements
@@ -226,19 +225,16 @@ func (r *ConfigFieldReader) readSet(
 		computed := r.hasComputedSubKeys(
 			fmt.Sprintf("%s.%d", strings.Join(address, "."), i), schema)
 
-		code := set.add(v)
+		code := set.add(v, computed)
 		indexMap[code] = i
-		if computed {
-			set.m[-code] = set.m[code]
-			delete(set.m, code)
-			code = -code
-		}
 	}
 
+	r.indexMaps[strings.Join(address, ".")] = indexMap
+
 	return FieldReadResult{
 		Value:  set,
 		Exists: true,
-	}, indexMap, nil
+	}, nil
 }
 
 // hasComputedSubKeys walks through a schema and returns whether or not the

helper/schema/field_reader_config_test.go

@@ -228,8 +228,8 @@ func TestConfigFieldReader_ComputedSet(t *testing.T) {
 		"set, normal": {
 			[]string{"strSet"},
 			FieldReadResult{
-				Value: map[int]interface{}{
-					2356372769: "foo",
+				Value: map[string]interface{}{
+					"2356372769": "foo",
 				},
 				Exists:   true,
 				Computed: false,

helper/schema/field_writer_map.go

@@ -298,8 +298,7 @@ func (w *MapFieldWriter) setSet(
 	}
 
 	for code, elem := range value.(*Set).m {
-		codeStr := strconv.FormatInt(int64(code), 10)
-		if err := w.set(append(addrCopy, codeStr), elem); err != nil {
+		if err := w.set(append(addrCopy, code), elem); err != nil {
 			return err
 		}
 	}

helper/schema/resource_data.go

@@ -228,7 +228,7 @@ func (d *ResourceData) State() *terraform.InstanceState {
 	// attribute set as a map[string]interface{}, write it to a MapFieldWriter,
 	// and then use that map.
 	rawMap := make(map[string]interface{})
-	for k, _ := range d.schema {
+	for k := range d.schema {
 		source := getSourceSet
 		if d.partial {
 			source = getSourceState
@@ -343,13 +343,13 @@ func (d *ResourceData) diffChange(
 }
 
 func (d *ResourceData) getChange(
-	key string,
+	k string,
 	oldLevel getSource,
 	newLevel getSource) (getResult, getResult) {
 	var parts, parts2 []string
-	if key != "" {
-		parts = strings.Split(key, ".")
-		parts2 = strings.Split(key, ".")
+	if k != "" {
+		parts = strings.Split(k, ".")
+		parts2 = strings.Split(k, ".")
 	}
 
 	o := d.get(parts, oldLevel)
@@ -374,13 +374,6 @@ func (d *ResourceData) get(addr []string, source getSource) getResult {
 		level = "state"
 	}
 
-	// Build the address of the key we're looking for and ask the FieldReader
-	for i, v := range addr {
-		if v[0] == '~' {
-			addr[i] = v[1:]
-		}
-	}
-
 	var result FieldReadResult
 	var err error
 	if exact {

helper/schema/resource_data_test.go

@@ -1509,9 +1509,9 @@ func TestResourceDataSet(t *testing.T) {
 
 			Key: "ports",
 			Value: &Set{
-				m: map[int]interface{}{
-					1: 1,
-					2: 2,
+				m: map[string]interface{}{
+					"1": 1,
+					"2": 2,
 				},
 			},
 
@@ -1546,7 +1546,7 @@ func TestResourceDataSet(t *testing.T) {
 			Err:   true,
 
 			GetKey:   "ports",
-			GetValue: []interface{}{80, 100},
+			GetValue: []interface{}{100, 80},
 		},
 
 		// #11: Set with nested set

helper/schema/schema.go

@@ -866,23 +866,16 @@ func (m schemaMap) diffSet(
 
 	// Build the list of codes that will make up our set. This is the
 	// removed codes as well as all the codes in the new codes.
-	codes := make([][]int, 2)
+	codes := make([][]string, 2)
 	codes[0] = os.Difference(ns).listCode()
 	codes[1] = ns.listCode()
 	for _, list := range codes {
 		for _, code := range list {
-			// If the code is negative (first character is -) then
-			// replace it with "~" for our computed set stuff.
-			codeStr := strconv.Itoa(code)
-			if codeStr[0] == '-' {
-				codeStr = string('~') + codeStr[1:]
-			}
-
 			switch t := schema.Elem.(type) {
 			case *Resource:
 				// This is a complex resource
 				for k2, schema := range t.Schema {
-					subK := fmt.Sprintf("%s.%s.%s", k, codeStr, k2)
+					subK := fmt.Sprintf("%s.%s.%s", k, code, k2)
 					err := m.diff(subK, schema, diff, d, true)
 					if err != nil {
 						return err
@@ -896,7 +889,7 @@ func (m schemaMap) diffSet(
 
 				// This is just a primitive element, so go through each and
 				// just diff each.
-				subK := fmt.Sprintf("%s.%s", k, codeStr)
+				subK := fmt.Sprintf("%s.%s", k, code)
 				err := m.diff(subK, &t2, diff, d, true)
 				if err != nil {
 					return err

helper/schema/set.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"reflect"
 	"sort"
+	"strconv"
 	"sync"
 
 	"github.com/hashicorp/terraform/helper/hashcode"
@@ -43,7 +44,7 @@ func HashSchema(schema *Schema) SchemaSetFunc {
 type Set struct {
 	F SchemaSetFunc
 
-	m    map[int]interface{}
+	m    map[string]interface{}
 	once sync.Once
 }
 
@@ -65,7 +66,7 @@ func CopySet(otherSet *Set) *Set {
 
 // Add adds an item to the set if it isn't already in the set.
 func (s *Set) Add(item interface{}) {
-	s.add(item)
+	s.add(item, false)
 }
 
 // Remove removes an item if it's already in the set. Idempotent.
@@ -157,13 +158,17 @@ func (s *Set) GoString() string {
 }
 
 func (s *Set) init() {
-	s.m = make(map[int]interface{})
+	s.m = make(map[string]interface{})
 }
 
-func (s *Set) add(item interface{}) int {
+func (s *Set) add(item interface{}, computed bool) string {
 	s.once.Do(s.init)
 
 	code := s.hash(item)
+	if computed {
+		code = "~" + code
+	}
+
 	if _, ok := s.m[code]; !ok {
 		s.m[code] = item
 	}
@@ -171,34 +176,34 @@ func (s *Set) add(item interface{}) int {
 	return code
 }
 
-func (s *Set) hash(item interface{}) int {
+func (s *Set) hash(item interface{}) string {
 	code := s.F(item)
 	// Always return a nonnegative hashcode.
 	if code < 0 {
-		return -code
+		code = -code
 	}
-	return code
+	return strconv.Itoa(code)
 }
 
-func (s *Set) remove(item interface{}) int {
+func (s *Set) remove(item interface{}) string {
 	s.once.Do(s.init)
 
-	code := s.F(item)
+	code := s.hash(item)
 	delete(s.m, code)
 
 	return code
 }
 
 func (s *Set) index(item interface{}) int {
-	return sort.SearchInts(s.listCode(), s.hash(item))
+	return sort.SearchStrings(s.listCode(), s.hash(item))
 }
 
-func (s *Set) listCode() []int {
+func (s *Set) listCode() []string {
 	// Sort the hash codes so the order of the list is deterministic
-	keys := make([]int, 0, len(s.m))
-	for k, _ := range s.m {
+	keys := make([]string, 0, len(s.m))
+	for k := range s.m {
 		keys = append(keys, k)
 	}
-	sort.Sort(sort.IntSlice(keys))
+	sort.Sort(sort.StringSlice(keys))
 	return keys
 }

helper/schema/set_test.go

@@ -11,7 +11,7 @@ func TestSetAdd(t *testing.T) {
 	s.Add(5)
 	s.Add(25)
 
-	expected := []interface{}{1, 5, 25}
+	expected := []interface{}{1, 25, 5}
 	actual := s.List()
 	if !reflect.DeepEqual(actual, expected) {
 		t.Fatalf("bad: %#v", actual)
@@ -101,7 +101,7 @@ func TestSetUnion(t *testing.T) {
 	union := s1.Union(s2)
 	union.Add(2)
 
-	expected := []interface{}{1, 2, 5, 25}
+	expected := []interface{}{1, 2, 25, 5}
 	actual := union.List()
 	if !reflect.DeepEqual(actual, expected) {
 		t.Fatalf("bad: %#v", actual)