thunder icon indicating copy to clipboard operation
thunder copied to clipboard

go federation: add fix for infinite recursion

Open steveshi7 opened this issue 3 years ago • 2 comments

This PR adds a fix for an infinite recursion bug in flattening code.

The bug is rooted in the mergeSameAlias function, where we combine selections with the same alias's subselections. However, because we do not deepcopy the selections before modifying them, and because our fragments have pointers to the same selections, this results in modifying the underlying subselections.

Performance: Since this approach uses deepcopying, we can reasonably expect this to impact performance. Here are the results from benchmarking:

Current
BenchmarkNormalize2-8   	    5182	    193595 ns/op	  101487 B/op	    2131 allocs/op

DeepCopyInMerge:
BenchmarkNormalize2-8   	     613	   1665200 ns/op	  810135 B/op	   22725 allocs/op

DeepCopyInFlatten:
BenchmarkNormalize2-8   	     619	   1845102 ns/op	  880146 B/op	   24415 allocs/op

As a result of this change, we see a roughly 760% runtime increase and 1000% memory utilization increase.

The DeepCopy in Merge refers to an alternative solution where we add the deepcopy to the mergeSameAlias function, here:

copy := *selection
selection = &copy
newSelections = append(newSelections, selection)
last = selection

steveshi7 avatar Jul 28 '21 09:07 steveshi7

I changed the deep copy to only copy the first layer of selections and fragments, the benchmark is as below:

BenchmarkNormalize2-16    	    5040	    248245 ns/op	  105241 B/op	    2232 allocs/op

KevinXing avatar Jul 30 '21 01:07 KevinXing

The root cause is last.SelectionSet.Selections = append(last.SelectionSet.Selections, s) sometimes create a new slice, sometimes not, when it does not create the new slice, we will run into the bug.

KevinXing avatar Jul 30 '21 01:07 KevinXing