From 7b24c7534e796ae09271917cd0d5d5ce75da4d87 Mon Sep 17 00:00:00 2001 From: Martin Martinez Rivera Date: Fri, 27 Sep 2019 15:07:16 -0700 Subject: [PATCH 1/3] Do not store non-pointer values in sync.Pool. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sync.Pool is used to avoid unnecessary allocations and reduce the amount of work the garbage collector has to do. When passing a value that is not a pointer to a function that accepts an interface, the value needs to be placed on the heap, which means an additional allocation. Slices are a common thing to put in sync.Pools, and they’re structs with 3 fields (length, capacity, and a pointer to an array). In order to avoid the extra allocation, one should store a pointer to the slice instead. --- query/shortest.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/query/shortest.go b/query/shortest.go index d7b23a95fb0..759e82e0e7f 100644 --- a/query/shortest.go +++ b/query/shortest.go @@ -51,7 +51,7 @@ type queueItem struct { var pathPool = sync.Pool{ New: func() interface{} { - return []pathInfo{} + return &[]pathInfo{} }, } @@ -357,17 +357,18 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { continue } - curPath := pathPool.Get().([]pathInfo) - if cap(curPath) < len(item.path.route)+1 { + curPath := pathPool.Get().(*[]pathInfo) + if cap(*curPath) < len(item.path.route)+1 { // We can't use it due to insufficient capacity. Put it back. pathPool.Put(curPath) - curPath = make([]pathInfo, len(item.path.route)+1) + newSlice := make([]pathInfo, len(item.path.route)+1) + curPath = &newSlice } else { // Use the curPath from pathPool. Set length appropriately. - curPath = curPath[:len(item.path.route)+1] + *curPath = (*curPath)[:len(item.path.route)+1] } - n := copy(curPath, item.path.route) - curPath[n] = pathInfo{ + n := copy(*curPath, item.path.route) + (*curPath)[n] = pathInfo{ uid: toUid, attr: info.attr, facet: info.facet, @@ -376,12 +377,12 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { uid: toUid, cost: item.cost + cost, hop: item.hop + 1, - path: route{route: curPath}, + path: route{route: *curPath}, } heap.Push(&pq, node) } // Return the popped nodes path to pool. - pathPool.Put(item.path.route) + pathPool.Put(&item.path.route) } next <- false From 62e3118af40e8e1c85838475de0b7ee87b71bc98 Mon Sep 17 00:00:00 2001 From: Martin Martinez Rivera Date: Fri, 27 Sep 2019 15:17:03 -0700 Subject: [PATCH 2/3] Store route as pointer to array. --- query/shortest.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/query/shortest.go b/query/shortest.go index 759e82e0e7f..68ec1f6feaf 100644 --- a/query/shortest.go +++ b/query/shortest.go @@ -37,7 +37,7 @@ type pathInfo struct { } type route struct { - route []pathInfo + route *[]pathInfo totalWeight float64 } @@ -284,7 +284,7 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { uid: sg.Params.From, cost: 0, hop: 0, - path: route{route: []pathInfo{{uid: sg.Params.From}}}, + path: route{route: &[]pathInfo{{uid: sg.Params.From}}}, } heap.Push(&pq, srcNode) @@ -358,16 +358,16 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { } curPath := pathPool.Get().(*[]pathInfo) - if cap(*curPath) < len(item.path.route)+1 { + if cap(*curPath) < len(*item.path.route)+1 { // We can't use it due to insufficient capacity. Put it back. pathPool.Put(curPath) - newSlice := make([]pathInfo, len(item.path.route)+1) + newSlice := make([]pathInfo, len(*item.path.route)+1) curPath = &newSlice } else { // Use the curPath from pathPool. Set length appropriately. - *curPath = (*curPath)[:len(item.path.route)+1] + *curPath = (*curPath)[:len(*item.path.route)+1] } - n := copy(*curPath, item.path.route) + n := copy(*curPath, *item.path.route) (*curPath)[n] = pathInfo{ uid: toUid, attr: info.attr, @@ -377,12 +377,12 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { uid: toUid, cost: item.cost + cost, hop: item.hop + 1, - path: route{route: *curPath}, + path: route{route: curPath}, } heap.Push(&pq, node) } // Return the popped nodes path to pool. - pathPool.Put(&item.path.route) + pathPool.Put(item.path.route) } next <- false @@ -392,7 +392,7 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { return nil, nil } var res []uint64 - for _, it := range kroutes[0].route { + for _, it := range *kroutes[0].route { res = append(res, it.uid) } sg.DestUIDs.Uids = res @@ -642,26 +642,26 @@ func createkroutesubgraph(ctx context.Context, kroutes []route) []*SubGraph { shortestSg.pathMeta = &pathMetadata{ weight: it.totalWeight, } - curUid := it.route[0].uid + curUid := (*it.route)[0].uid shortestSg.SrcUIDs = &pb.List{Uids: []uint64{curUid}} shortestSg.DestUIDs = &pb.List{Uids: []uint64{curUid}} shortestSg.uidMatrix = []*pb.List{{Uids: []uint64{curUid}}} curNode := shortestSg i := 0 - for ; i < len(it.route)-1; i++ { - curUid := it.route[i].uid - childUid := it.route[i+1].uid + for ; i < len(*it.route)-1; i++ { + curUid := (*it.route)[i].uid + childUid := (*it.route)[i+1].uid node := new(SubGraph) node.Params = params{ Shortest: true, } - if it.route[i+1].facet != nil { + if (*it.route)[i+1].facet != nil { // For consistent later processing. node.Params.Facet = &pb.FacetParams{} } - node.Attr = it.route[i+1].attr - node.facetsMatrix = []*pb.FacetsList{{FacetsList: []*pb.Facets{it.route[i+1].facet}}} + node.Attr = (*it.route)[i+1].attr + node.facetsMatrix = []*pb.FacetsList{{FacetsList: []*pb.Facets{(*it.route)[i+1].facet}}} node.SrcUIDs = &pb.List{Uids: []uint64{curUid}} node.DestUIDs = &pb.List{Uids: []uint64{childUid}} node.uidMatrix = []*pb.List{{Uids: []uint64{childUid}}} @@ -674,7 +674,7 @@ func createkroutesubgraph(ctx context.Context, kroutes []route) []*SubGraph { node.Params = params{ Shortest: true, } - uid := it.route[i].uid + uid := (*it.route)[i].uid node.SrcUIDs = &pb.List{Uids: []uint64{uid}} node.uidMatrix = []*pb.List{{Uids: []uint64{uid}}} curNode.Children = append(curNode.Children, node) From be85bf4ee1b374255ad2d8e1e838634a39363cb3 Mon Sep 17 00:00:00 2001 From: Martin Martinez Rivera Date: Thu, 3 Oct 2019 14:28:59 -0700 Subject: [PATCH 3/3] Added sanity check to pointer retrieved from sync pool. --- query/shortest.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/query/shortest.go b/query/shortest.go index 68ec1f6feaf..d8de9339c59 100644 --- a/query/shortest.go +++ b/query/shortest.go @@ -358,6 +358,9 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { } curPath := pathPool.Get().(*[]pathInfo) + if curPath == nil { + return nil, errors.Errorf("Sync pool returned a nil pointer") + } if cap(*curPath) < len(*item.path.route)+1 { // We can't use it due to insufficient capacity. Put it back. pathPool.Put(curPath)