Repository: allegro/marathon-consul
Pull request #250 (2017-07-22 14:49:21)

Calculate common tags only once.
This PR extracts commonTags outside of loop.
They are not changed inside the loop so there is no
need to recalculate them every time.
Ta reduce allocations tags slice i create with
initial capacity equal to the lenght of application labels.

```
benchmark                              old ns/op     new ns/op     delta
BenchmarkApp_RegistrationIntents-8     438           415           -5.25%
BenchmarkApp_RegistrationIntents-8     436           419           -3.90%
BenchmarkApp_RegistrationIntents-8     432           415           -3.94%

benchmark                              old allocs     new allocs     delta
BenchmarkApp_RegistrationIntents-8     4              3              -25.00%
BenchmarkApp_RegistrationIntents-8     4              3              -25.00%
BenchmarkApp_RegistrationIntents-8     4              3              -25.00%

benchmark                              old bytes     new bytes     delta
BenchmarkApp_RegistrationIntents-8     128           208           +62.50%
BenchmarkApp_RegistrationIntents-8     128           208           +62.50%
BenchmarkApp_RegistrationIntents-8     128           208           +62.50%
```

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12506595/badge)](https://coveralls.io/builds/12506595)

Coverage increased (+0.006%) to 89.374% when pulling **1f7dc8ee7d353bac06c2dfccb626a57b4e76a9f8 on improve_label_generation_perf** into **1cda5744ffbaa7c1b67e44ed547fd5bd58af6eeb on master**.


Patch:
diff --git a/apps/app.go b/apps/app.go
index c839809..5ea1fd1 100644
--- a/apps/app.go
+++ b/apps/app.go
@@ -105,12 +105,13 @@ func (app App) RegistrationIntentsNumber() int {
 func (app App) RegistrationIntents(task *Task, nameSeparator string) []RegistrationIntent {
 	taskPortsCount := len(task.Ports)
 	definitions := app.findConsulPortDefinitions()
+	commonTags := labelsToTags(app.Labels)
 	if len(definitions) == 0 && taskPortsCount != 0 {
 		return []RegistrationIntent{
 			{
 				Name: app.labelsToName(app.Labels, nameSeparator),
 				Port: task.Ports[0],
-				Tags: labelsToTags(app.Labels),
+				Tags: commonTags,
 			},
 		}
 	}
@@ -124,7 +125,7 @@ func (app App) RegistrationIntents(task *Task, nameSeparator string) []Registrat
 		intents = append(intents, RegistrationIntent{
 			Name: app.labelsToName(d.Labels, nameSeparator),
 			Port: task.Ports[d.Index],
-			Tags: append(labelsToTags(app.Labels), labelsToTags(d.Labels)...),
+			Tags: append(labelsToTags(d.Labels), commonTags...),
 		})
 	}
 	return intents
@@ -135,7 +136,7 @@ func marathonAppNameToServiceName(name string, nameSeparator string) string {
 }
 
 func labelsToTags(labels map[string]string) []string {
-	tags := []string{}
+	tags := make([]string, 0, len(labels))
 	for key, value := range labels {
 		if value == "tag" {
 			tags = append(tags, key)


Test patch:
diff --git a/apps/app_bench_test.go b/apps/app_bench_test.go
new file mode 100644
index 0000000..79f4505
--- /dev/null
+++ b/apps/app_bench_test.go
@@ -0,0 +1,33 @@
+package apps
+
+import "testing"
+
+func BenchmarkApp_RegistrationIntents(b *testing.B) {
+
+	// given an average app.
+	// Over 80% of our apps has only one port
+	// registered and more then five (5) labels but only 2 of them
+	// are tags. Over 90% of our application does not have
+	// special tags on ports.
+	app := &App{
+		ID: "app-name",
+		Labels: map[string]string{
+			"#1":     "label",
+			"#2":     "label",
+			"consul": "true",
+			"#3":     "label",
+			"#4":     "tag",
+			"#5":     "label",
+			"#6":     "tag",
+			"#7":     "label",
+		},
+		PortDefinitions: []PortDefinition{{Labels: map[string]string{"#8": "tag"}}},
+	}
+	task := &Task{
+		Ports: []int{0},
+	}
+
+	for i := 0; i < b.N; i++ {
+		app.RegistrationIntents(task, "-")
+	}
+}
diff --git a/apps/app_test.go b/apps/app_test.go
index 13dd754..3420ee3 100644
--- a/apps/app_test.go
+++ b/apps/app_test.go
@@ -58,12 +58,12 @@ func TestAppInt(t *testing.T) {
 	assert.Equal(t, 2, app.RegistrationIntentsNumber())
 
 	task := Task{Ports: []int{0, 1, 2, 3}}
-	intents := app.RegistrationIntents(&task,".")
+	intents := app.RegistrationIntents(&task, ".")
 
-	assert.Contains(t,  intents[0].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
-	assert.Contains(t,  intents[1].Tags, "secureConnection:true")
-	assert.NotContains(t,  intents[0].Tags, "secureConnection:true")
-	assert.NotContains(t,  intents[1].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
+	assert.Contains(t, intents[0].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
+	assert.Contains(t, intents[1].Tags, "secureConnection:true")
+	assert.NotContains(t, intents[0].Tags, "secureConnection:true")
+	assert.NotContains(t, intents[1].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
 }
 
 func TestParseApp(t *testing.T) {
@@ -312,7 +312,7 @@ func TestRegistrationIntent_OverrideNameAndAddTagsViaPortDefinitions(t *testing.
 	assert.Len(t, intents, 1)
 	assert.Equal(t, "other-name", intents[0].Name)
 	assert.Equal(t, 1234, intents[0].Port)
-	assert.Equal(t, []string{"private", "other"}, intents[0].Tags)
+	assert.Equal(t, []string{"other", "private"}, intents[0].Tags)
 }
 
 func TestRegistrationIntent_PickDifferentPortViaPortDefinitions(t *testing.T) {
@@ -367,10 +367,10 @@ func TestRegistrationIntent_MultipleIntentsViaPortDefinitionIfMultipleContainCon
 	assert.Len(t, intents, 2)
 	assert.Equal(t, "first-name", intents[0].Name)
 	assert.Equal(t, 1234, intents[0].Port)
-	assert.Equal(t, []string{"common-tag", "first-tag"}, intents[0].Tags)
+	assert.Equal(t, []string{"first-tag", "common-tag"}, intents[0].Tags)
 	assert.Equal(t, "second-name", intents[1].Name)
 	assert.Equal(t, 5678, intents[1].Port)
-	assert.Equal(t, []string{"common-tag", "second-tag"}, intents[1].Tags)
+	assert.Equal(t, []string{"second-tag", "common-tag"}, intents[1].Tags)
 }
 
 func TestRegistrationIntent_TaskHasLessPortsThanApp(t *testing.T) {
@@ -400,7 +400,7 @@ func TestRegistrationIntent_TaskHasLessPortsThanApp(t *testing.T) {
 	assert.Len(t, intents, 1)
 	assert.Equal(t, "first-name", intents[0].Name)
 	assert.Equal(t, 1234, intents[0].Port)
-	assert.Equal(t, []string{"common-tag", "first-tag"}, intents[0].Tags)
+	assert.Equal(t, []string{"first-tag", "common-tag"}, intents[0].Tags)
 }
 
 func TestRegistrationIntentsNumber(t *testing.T) {
diff --git a/consul/consul_test.go b/consul/consul_test.go
index 9393872..a78aa38 100644
--- a/consul/consul_test.go
+++ b/consul/consul_test.go
@@ -452,9 +452,9 @@ func TestRegisterServices_MultipleRegistrations(t *testing.T) {
 	assert.True(t, found, "second-name not found in services")
 
 	assert.Equal(t, "first-name", first.Name)
-	assert.Equal(t, []string{"marathon", "common-tag", "first-tag", "marathon-task:serviceA.0"}, first.Tags)
+	assert.Equal(t, []string{"marathon", "first-tag", "common-tag", "marathon-task:serviceA.0"}, first.Tags)
 	assert.Equal(t, "second-name", second.Name)
-	assert.Equal(t, []string{"marathon", "common-tag", "second-tag", "marathon-task:serviceA.0"}, second.Tags)
+	assert.Equal(t, []string{"marathon", "second-tag", "common-tag", "marathon-task:serviceA.0"}, second.Tags)
 }
 
 func findServiceByName(name string, services []*service.Service) (*service.Service, bool) {