From e0ddded0774eb9e971d507a3d6e483df50cbdc6a Mon Sep 17 00:00:00 2001 From: Gyu-Ho Lee Date: Mon, 13 Feb 2017 09:00:55 -0800 Subject: [PATCH] auth: simplify merging range perm No need of separate function to filter duplicates. Just merge ranges in-place ``` go test -v -run=xxx -bench=BenchmarkMergeOld -benchmem BenchmarkMergeOld-8 100000 13524 ns/op 1104 B/op 8 allocs/op go test -v -run=xxx -bench=BenchmarkMergeNew -benchmem BenchmarkMergeNew-8 100000 13432 ns/op 936 B/op 3 allocs/op ``` Not much performance boost, but less memory allocation and simpler --- auth/range_perm_cache.go | 92 +++++++-------- auth/range_perm_cache_test.go | 204 ++++++++++++++++------------------ 2 files changed, 140 insertions(+), 156 deletions(-) diff --git a/auth/range_perm_cache.go b/auth/range_perm_cache.go index 3cd1ad2a4..d0bc286e8 100644 --- a/auth/range_perm_cache.go +++ b/auth/range_perm_cache.go @@ -47,59 +47,53 @@ func isRangeEqual(a, b *rangePerm) bool { return bytes.Equal(a.begin, b.begin) && bytes.Equal(a.end, b.end) } -// removeSubsetRangePerms removes any rangePerms that are subsets of other rangePerms. -// If there are equal ranges, removeSubsetRangePerms only keeps one of them. -// It returns a sorted rangePerm slice. -func removeSubsetRangePerms(perms []*rangePerm) (newp []*rangePerm) { - sort.Sort(RangePermSliceByBegin(perms)) - var prev *rangePerm - for i := range perms { - if i == 0 { - prev = perms[i] - newp = append(newp, perms[i]) - continue - } - if isRangeEqual(perms[i], prev) { - continue - } - if isSubset(perms[i], prev) { - continue - } - if isSubset(prev, perms[i]) { - prev = perms[i] - newp[len(newp)-1] = perms[i] - continue - } - prev = perms[i] - newp = append(newp, perms[i]) +// mergeRangePerms merges adjacent rangePerms. +func mergeRangePerms(perms []*rangePerm) (rs []*rangePerm) { + if len(perms) < 2 { + return perms } - return newp + sort.Sort(RangePermSliceByBegin(perms)) + + // initialize with first rangePerm + rs = append(rs, perms[0]) + + // merge in-place from 2nd + for i := 1; i < len(perms); i++ { + rp := rs[len(rs)-1] + + // skip duplicate range + if isRangeEqual(rp, perms[i]) { + continue + } + + // merge ["a", "") with ["a", "c") + if bytes.Equal(rp.begin, perms[i].begin) && len(rp.end) == 0 && len(perms[i].end) > 0 { + rp.end = perms[i].end + continue + } + + // do not merge ["a", "b") with ["b", "") + if bytes.Equal(rp.end, perms[i].begin) && len(perms[i].end) == 0 { + rs = append(rs, perms[i]) + continue + } + + // rp.end < perms[i].begin; found beginning of next range + if bytes.Compare(rp.end, perms[i].begin) < 0 { + rs = append(rs, perms[i]) + continue + } + + rp.end = getMax(rp.end, perms[i].end) + } + return } -// mergeRangePerms merges adjacent rangePerms. -func mergeRangePerms(perms []*rangePerm) []*rangePerm { - var merged []*rangePerm - perms = removeSubsetRangePerms(perms) - - i := 0 - for i < len(perms) { - begin, next := i, i - for next+1 < len(perms) && bytes.Compare(perms[next].end, perms[next+1].begin) >= 0 { - next++ - } - // don't merge ["a", "b") with ["b", ""), because perms[next+1].end is empty. - if next != begin && len(perms[next].end) > 0 { - merged = append(merged, &rangePerm{begin: perms[begin].begin, end: perms[next].end}) - } else { - merged = append(merged, perms[begin]) - if next != begin { - merged = append(merged, perms[next]) - } - } - i = next + 1 +func getMax(a, b []byte) []byte { + if bytes.Compare(a, b) > 0 { + return a } - - return merged + return b } func getMergedPerms(tx backend.BatchTx, userName string) *unifiedRangePermissions { diff --git a/auth/range_perm_cache_test.go b/auth/range_perm_cache_test.go index 22745158a..2af31e0d5 100644 --- a/auth/range_perm_cache_test.go +++ b/auth/range_perm_cache_test.go @@ -17,7 +17,6 @@ package auth import ( "bytes" "fmt" - "reflect" "testing" ) @@ -43,109 +42,14 @@ func TestGetMergedPerms(t *testing.T) { tests := []struct { params []*rangePerm want []*rangePerm - }{ - { - []*rangePerm{{[]byte("a"), []byte("b")}}, - []*rangePerm{{[]byte("a"), []byte("b")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("")}}, - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("c")}}, - []*rangePerm{{[]byte("a"), []byte("c")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("c")}, {[]byte("b"), []byte("d")}}, - []*rangePerm{{[]byte("a"), []byte("d")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("c")}, {[]byte("d"), []byte("e")}}, - []*rangePerm{{[]byte("a"), []byte("c")}, {[]byte("d"), []byte("e")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("c"), []byte("d")}, {[]byte("e"), []byte("f")}}, - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("c"), []byte("d")}, {[]byte("e"), []byte("f")}}, - }, - { - []*rangePerm{{[]byte("e"), []byte("f")}, {[]byte("c"), []byte("d")}, {[]byte("a"), []byte("b")}}, - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("c"), []byte("d")}, {[]byte("e"), []byte("f")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("c"), []byte("d")}, {[]byte("a"), []byte("z")}}, - []*rangePerm{{[]byte("a"), []byte("z")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("c"), []byte("d")}, {[]byte("a"), []byte("z")}, {[]byte("1"), []byte("9")}}, - []*rangePerm{{[]byte("1"), []byte("9")}, {[]byte("a"), []byte("z")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("c"), []byte("d")}, {[]byte("a"), []byte("z")}, {[]byte("1"), []byte("a")}}, - []*rangePerm{{[]byte("1"), []byte("z")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("a"), []byte("z")}, {[]byte("5"), []byte("6")}, {[]byte("1"), []byte("9")}}, - []*rangePerm{{[]byte("1"), []byte("9")}, {[]byte("a"), []byte("z")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("c")}, {[]byte("c"), []byte("d")}, {[]byte("d"), []byte("f")}, {[]byte("1"), []byte("9")}}, - []*rangePerm{{[]byte("1"), []byte("9")}, {[]byte("a"), []byte("f")}}, - }, - // overlapping - { - []*rangePerm{{[]byte("a"), []byte("f")}, {[]byte("b"), []byte("g")}}, - []*rangePerm{{[]byte("a"), []byte("g")}}, - }, - // keys - { - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("")}}, - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("a"), []byte("c")}}, - []*rangePerm{{[]byte("a"), []byte("c")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("a"), []byte("c")}, {[]byte("b"), []byte("")}}, - []*rangePerm{{[]byte("a"), []byte("c")}}, - }, - { - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("c")}, {[]byte("b"), []byte("")}, {[]byte("c"), []byte("")}, {[]byte("d"), []byte("")}}, - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("c")}, {[]byte("c"), []byte("")}, {[]byte("d"), []byte("")}}, - }, - // duplicate ranges - { - []*rangePerm{{[]byte("a"), []byte("f")}, {[]byte("a"), []byte("f")}}, - []*rangePerm{{[]byte("a"), []byte("f")}}, - }, - // duplicate keys - { - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("a"), []byte("")}, {[]byte("a"), []byte("")}}, - []*rangePerm{{[]byte("a"), []byte("")}}, - }, - } - - for i, tt := range tests { - result := mergeRangePerms(tt.params) - if !isPermsEqual(result, tt.want) { - t.Errorf("#%d: result=%q, want=%q", i, result, tt.want) - } - } -} - -func TestRemoveSubsetRangePerms(t *testing.T) { - tests := []struct { - perms []*rangePerm - expect []*rangePerm }{ { // subsets converge []*rangePerm{{[]byte{2}, []byte{3}}, {[]byte{2}, []byte{5}}, {[]byte{1}, []byte{4}}}, - []*rangePerm{{[]byte{1}, []byte{4}}, {[]byte{2}, []byte{5}}}, + []*rangePerm{{[]byte{1}, []byte{5}}}, }, { // subsets converge []*rangePerm{{[]byte{0}, []byte{3}}, {[]byte{0}, []byte{1}}, {[]byte{2}, []byte{4}}, {[]byte{0}, []byte{2}}}, - []*rangePerm{{[]byte{0}, []byte{3}}, {[]byte{2}, []byte{4}}}, + []*rangePerm{{[]byte{0}, []byte{4}}}, }, { // biggest range at the end []*rangePerm{{[]byte{2}, []byte{3}}, {[]byte{0}, []byte{2}}, {[]byte{1}, []byte{4}}, {[]byte{0}, []byte{5}}}, @@ -159,21 +63,107 @@ func TestRemoveSubsetRangePerms(t *testing.T) { []*rangePerm{{[]byte{2}, []byte{3}}, {[]byte{0}, []byte{1}}, {[]byte{4}, []byte{7}}, {[]byte{8}, []byte{15}}}, []*rangePerm{{[]byte{0}, []byte{1}}, {[]byte{2}, []byte{3}}, {[]byte{4}, []byte{7}}, {[]byte{8}, []byte{15}}}, }, + { + []*rangePerm{makePerm("00", "03"), makePerm("18", "19"), makePerm("02", "08"), makePerm("10", "12")}, + []*rangePerm{makePerm("00", "08"), makePerm("10", "12"), makePerm("18", "19")}, + }, + { + []*rangePerm{makePerm("a", "b")}, + []*rangePerm{makePerm("a", "b")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("b", "")}, + []*rangePerm{makePerm("a", "b"), makePerm("b", "")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("b", "c")}, + []*rangePerm{makePerm("a", "c")}, + }, + { + []*rangePerm{makePerm("a", "c"), makePerm("b", "d")}, + []*rangePerm{makePerm("a", "d")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("b", "c"), makePerm("d", "e")}, + []*rangePerm{makePerm("a", "c"), makePerm("d", "e")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("c", "d"), makePerm("e", "f")}, + []*rangePerm{makePerm("a", "b"), makePerm("c", "d"), makePerm("e", "f")}, + }, + { + []*rangePerm{makePerm("e", "f"), makePerm("c", "d"), makePerm("a", "b")}, + []*rangePerm{makePerm("a", "b"), makePerm("c", "d"), makePerm("e", "f")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("c", "d"), makePerm("a", "z")}, + []*rangePerm{makePerm("a", "z")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("c", "d"), makePerm("a", "z"), makePerm("1", "9")}, + []*rangePerm{makePerm("1", "9"), makePerm("a", "z")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("c", "d"), makePerm("a", "z"), makePerm("1", "a")}, + []*rangePerm{makePerm("1", "z")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("a", "z"), makePerm("5", "6"), makePerm("1", "9")}, + []*rangePerm{makePerm("1", "9"), makePerm("a", "z")}, + }, + { + []*rangePerm{makePerm("a", "b"), makePerm("b", "c"), makePerm("c", "d"), makePerm("b", "f"), makePerm("1", "9")}, + []*rangePerm{makePerm("1", "9"), makePerm("a", "f")}, + }, + // overlapping + { + []*rangePerm{makePerm("a", "f"), makePerm("b", "g")}, + []*rangePerm{makePerm("a", "g")}, + }, + // keys + { + []*rangePerm{makePerm("a", ""), makePerm("b", "")}, + []*rangePerm{makePerm("a", ""), makePerm("b", "")}, + }, + { + []*rangePerm{makePerm("a", ""), makePerm("a", "c")}, + []*rangePerm{makePerm("a", "c")}, + }, + { + []*rangePerm{makePerm("a", ""), makePerm("a", "c"), makePerm("b", "")}, + []*rangePerm{makePerm("a", "c")}, + }, + { + []*rangePerm{makePerm("a", ""), makePerm("b", "c"), makePerm("b", ""), makePerm("c", ""), makePerm("d", "")}, + []*rangePerm{makePerm("a", ""), makePerm("b", "c"), makePerm("c", ""), makePerm("d", "")}, + }, + // duplicate ranges + { + []*rangePerm{makePerm("a", "f"), makePerm("a", "f")}, + []*rangePerm{makePerm("a", "f")}, + }, + // duplicate keys + { + []*rangePerm{makePerm("a", ""), makePerm("a", ""), makePerm("a", "")}, + []*rangePerm{makePerm("a", "")}, + }, } + for i, tt := range tests { - rs := removeSubsetRangePerms(tt.perms) - if !reflect.DeepEqual(rs, tt.expect) { - t.Fatalf("#%d: unexpected rangePerms %q, got %q", i, printPerms(rs), printPerms(tt.expect)) + result := mergeRangePerms(tt.params) + if !isPermsEqual(result, tt.want) { + t.Errorf("#%d: result=%q, want=%q", i, sprintPerms(result), sprintPerms(tt.want)) } } } -func printPerms(rs []*rangePerm) (txt string) { - for i, p := range rs { - if i != 0 { - txt += "," - } - txt += fmt.Sprintf("%+v", *p) +func makePerm(a, b string) *rangePerm { + return &rangePerm{begin: []byte(a), end: []byte(b)} +} + +func sprintPerms(rs []*rangePerm) (s string) { + for _, rp := range rs { + s += fmt.Sprintf("[%s %s] ", rp.begin, rp.end) } return }