fix delete function #606

woodsaj · 2017-04-17T12:13:31Z

when deleting a branch node, we first walk up the tree to delete
children. As all siblings and the parents are already being deleted,
there is no need to walk back down the tree removing the children and
any empty branches.

woodsaj · 2017-04-17T12:33:33Z

just added a benchmark.

Old code

$ go test -v -run None -bench BenchmarkDeletes -benchtime 10s
BenchmarkDeletes-4   	   50000	   2384248 ns/op	 1313605 B/op	      27 allocs/op
PASS
ok  	github.com/raintank/metrictank/idx/memory	123.452s

New Code

$ go test -v -run None -bench BenchmarkDeletes -benchtime 10s
BenchmarkDeletes-4   	 5000000	      4145 ns/op	    1354 B/op	       8 allocs/op
PASS
ok  	github.com/raintank/metrictank/idx/memory	66.588s

replay

That makes a lot of sense. I'm still surprised by how big the performance difference is though, good find

- when deleting a branch node, we first walk up the tree to delete children. As all siblings and the parents are already being deleted, there is no need to walk back down the tree removing the children and any empty branches.

Dieterbe · 2017-04-17T18:09:11Z

idx/memory/memory_test.go

@@ -279,6 +279,30 @@ func TestDelete(t *testing.T) {
 	})
 }

+func TestDeleteWithLotsOfSeries(t *testing.T) {


I think we should just put 10k or 1M or whatever in the title instead of "lots"

also what's the point of doing this many in a unit test anyway? doing it with 100 instead of 100k invokes all the exact same code, but runs the test in 4ms instead of 800ms on my computer.

the complexity of the old code was exponential, O(n^2). But this new code is O(n)

With the old code, it would take many minutes to delete 100k items from the index.

fixed in #609

my point is that the purpose of unit tests is testing functionality. if you want to verify performance criteria (such as x must take no more than y time) then that is what benchmarks are for.

Dieterbe · 2017-04-17T18:09:30Z

idx/memory/memory_test.go

+		data.SetId()
+		ix.AddOrUpdate(data, 1)
+	}
+	Convey("when deleting 1 million series", t, func() {


it was 1million, but that broke circleci due to needing too much ram.

fixed in #609

woodsaj requested review from Dieterbe and replay April 17, 2017 12:13

replay approved these changes Apr 17, 2017

View reviewed changes

fix delete function

8fc67f1

- when deleting a branch node, we first walk up the tree to delete children. As all siblings and the parents are already being deleted, there is no need to walk back down the tree removing the children and any empty branches.

woodsaj force-pushed the fixDelete branch from e2a4a20 to 8fc67f1 Compare April 17, 2017 12:58

woodsaj merged commit 4646f2d into master Apr 17, 2017

woodsaj deleted the fixDelete branch April 17, 2017 17:58

Dieterbe reviewed Apr 17, 2017

View reviewed changes

Dieterbe mentioned this pull request Apr 17, 2017

some operations lock the index too long #608

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix delete function #606

fix delete function #606

woodsaj commented Apr 17, 2017

woodsaj commented Apr 17, 2017

replay left a comment

Dieterbe Apr 17, 2017

Dieterbe Apr 17, 2017

woodsaj Apr 18, 2017

woodsaj Apr 18, 2017

Dieterbe Apr 18, 2017

Dieterbe Apr 17, 2017

woodsaj Apr 18, 2017

woodsaj Apr 18, 2017

fix delete function #606

fix delete function #606

Conversation

woodsaj commented Apr 17, 2017

woodsaj commented Apr 17, 2017

replay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment