From 89c8da65b69c8bde2f567991ccab8319f4f9846a Mon Sep 17 00:00:00 2001 From: aarzilli Date: Mon, 29 Oct 2018 12:22:03 +0100 Subject: [PATCH] proc: Improve performance of loadMap on very large sparse maps Users can create sparse maps in two ways, either by: a) adding lots of entries to a map and then deleting most of them, or b) using the make(mapType, N) expression with a very large N When this happens reading the resulting map will be very slow because loadMap needs to scan many buckets for each entry it finds. Technically this is not a bug, the user just created a map that's very sparse and therefore very slow to read. However it's very annoying to have the debugger hang for several seconds when trying to read the local variables just because one of them (which you might not even be interested into) happens to be a very sparse map. There is an easy mitigation to this problem: not reading any additional buckets once we know that we have already read all entries of the map, or as many entries as we need to fulfill the MaxArrayValues parameter. Unfortunately this is mostly useless, a VLSM (Very Large Sparse Map) with a single entry will still be slow to access, because the single entry in the map could easily end up in the last bucket. The obvious solution to this problem is to set a limit to the number of buckets we read when loading a map. However there is no good way to set this limit. If we hardcode it there will be no way to print maps that are beyond whatever limit we pick. We could let users (or clients) specify it but the meaning of such knob would be arcane and they would have no way of picking a good value (because there is no objectively good value for it). The solution used in this commit is to set an arbirtray limit on the number of buckets we read but only when loadMap is invoked through API calls ListLocalVars and ListFunctionArgs. In this way `ListLocalVars` and `ListFunctionArgs` (which are often invoked automatically by GUI clients) remain fast even in presence of a VLSM, but the contents of the VLSM can still be inspected using `EvalVariable`. --- pkg/proc/core/core_test.go | 4 +-- pkg/proc/fncall.go | 2 +- pkg/proc/moduledata.go | 2 +- pkg/proc/proc_test.go | 4 +-- pkg/proc/stack.go | 2 +- pkg/proc/types.go | 6 ++-- pkg/proc/variables.go | 58 ++++++++++++++++++++++++++++++---- service/api/conversions.go | 1 + service/debugger/locations.go | 2 +- service/rpc1/server.go | 2 +- service/test/variables_test.go | 4 +-- 11 files changed, 66 insertions(+), 21 deletions(-) diff --git a/pkg/proc/core/core_test.go b/pkg/proc/core/core_test.go index 12c90867..d1a4dce4 100644 --- a/pkg/proc/core/core_test.go +++ b/pkg/proc/core/core_test.go @@ -358,11 +358,11 @@ mainSearch: } scope := proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, *mainFrame) - v1, err := scope.EvalVariable("t", proc.LoadConfig{true, 1, 64, 64, -1}) + v1, err := scope.EvalVariable("t", proc.LoadConfig{true, 1, 64, 64, -1, 0}) assertNoError(err, t, "EvalVariable(t)") assertNoError(v1.Unreadable, t, "unreadable variable 't'") t.Logf("t = %#v\n", v1) - v2, err := scope.EvalVariable("s", proc.LoadConfig{true, 1, 64, 64, -1}) + v2, err := scope.EvalVariable("s", proc.LoadConfig{true, 1, 64, 64, -1, 0}) assertNoError(err, t, "EvalVariable(s)") assertNoError(v2.Unreadable, t, "unreadable variable 's'") t.Logf("s = %#v\n", v2) diff --git a/pkg/proc/fncall.go b/pkg/proc/fncall.go index 271b3ce2..76775004 100644 --- a/pkg/proc/fncall.go +++ b/pkg/proc/fncall.go @@ -218,7 +218,7 @@ func funcCallEvalExpr(p Process, expr string) (fn *Function, closureAddr uint64, if fnvar.Kind != reflect.Func { return nil, 0, nil, fmt.Errorf("expression %q is not a function", exprToString(callexpr.Fun)) } - fnvar.loadValue(LoadConfig{false, 0, 0, 0, 0}) + fnvar.loadValue(LoadConfig{false, 0, 0, 0, 0, 0}) if fnvar.Unreadable != nil { return nil, 0, nil, fnvar.Unreadable } diff --git a/pkg/proc/moduledata.go b/pkg/proc/moduledata.go index 0508ac77..a24714e0 100644 --- a/pkg/proc/moduledata.go +++ b/pkg/proc/moduledata.go @@ -88,7 +88,7 @@ func resolveTypeOff(bi *BinaryInfo, typeAddr uintptr, off uintptr, mem MemoryRea if err != nil { return nil, err } - v.loadValue(LoadConfig{false, 1, 0, 0, -1}) + v.loadValue(LoadConfig{false, 1, 0, 0, -1, 0}) addr, _ := constant.Int64Val(v.Value) return v.newVariable(v.Name, uintptr(addr), rtyp, mem), nil } diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index 2cccf463..3177bebf 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -28,7 +28,7 @@ import ( protest "github.com/derekparker/delve/pkg/proc/test" ) -var normalLoadConfig = proc.LoadConfig{true, 1, 64, 64, -1} +var normalLoadConfig = proc.LoadConfig{true, 1, 64, 64, -1, 0} var testBackend, buildMode string func init() { @@ -2656,7 +2656,7 @@ func BenchmarkTrace(b *testing.B) { assertNoError(proc.Continue(p), b, "Continue()") s, err := proc.GoroutineScope(p.CurrentThread()) assertNoError(err, b, "Scope()") - _, err = s.FunctionArguments(proc.LoadConfig{false, 0, 64, 0, 3}) + _, err = s.FunctionArguments(proc.LoadConfig{false, 0, 64, 0, 3, 0}) assertNoError(err, b, "FunctionArguments()") } b.StopTimer() diff --git a/pkg/proc/stack.go b/pkg/proc/stack.go index 4abcca4b..30965c89 100644 --- a/pkg/proc/stack.go +++ b/pkg/proc/stack.go @@ -645,7 +645,7 @@ func (g *G) readDefers(frames []Stackframe) { } func (d *Defer) load() { - d.variable.loadValue(LoadConfig{false, 1, 0, 0, -1}) + d.variable.loadValue(LoadConfig{false, 1, 0, 0, -1, 0}) if d.variable.Unreadable != nil { d.Unreadable = d.variable.Unreadable return diff --git a/pkg/proc/types.go b/pkg/proc/types.go index 7f185bbd..e3057115 100644 --- a/pkg/proc/types.go +++ b/pkg/proc/types.go @@ -578,7 +578,7 @@ func runtimeTypeToDIE(_type *Variable, dataAddr uintptr) (typ godwarf.Type, kind if typestring == nil || typestring.Addr == 0 || typestring.Kind != reflect.String { return nil, 0, fmt.Errorf("invalid interface type") } - typestring.loadValue(LoadConfig{false, 0, 512, 0, 0}) + typestring.loadValue(LoadConfig{false, 0, 512, 0, 0, 0}) if typestring.Unreadable != nil { return nil, 0, fmt.Errorf("invalid interface type: %v", typestring.Unreadable) } @@ -880,7 +880,7 @@ func nameOfInterfaceRuntimeType(_type *Variable, kind, tflag int64) (string, err buf.WriteString("interface {") methods, _ := _type.structMember(interfacetypeFieldMhdr) - methods.loadArrayValues(0, LoadConfig{false, 1, 0, 4096, -1}) + methods.loadArrayValues(0, LoadConfig{false, 1, 0, 4096, -1, 0}) if methods.Unreadable != nil { return "", nil } @@ -941,7 +941,7 @@ func nameOfStructRuntimeType(_type *Variable, kind, tflag int64) (string, error) buf.WriteString("struct {") fields, _ := _type.structMember("fields") - fields.loadArrayValues(0, LoadConfig{false, 2, 0, 4096, -1}) + fields.loadArrayValues(0, LoadConfig{false, 2, 0, 4096, -1, 0}) if fields.Unreadable != nil { return "", fields.Unreadable } diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index f2b9845b..91e1f2f0 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -32,6 +32,8 @@ const ( hashMinTopHash = 4 // used by map reading code, indicates minimum value of tophash that isn't empty or evacuated maxFramePrefetchSize = 1 * 1024 * 1024 // Maximum prefetch size for a stack frame + + maxMapBucketsFactor = 100 // Maximum numbers of map buckets to read for every requested map entry when loading variables through (*EvalScope).LocalVariables and (*EvalScope).FunctionArguments. ) type floatSpecial uint8 @@ -122,10 +124,39 @@ type LoadConfig struct { MaxArrayValues int // MaxStructFields is the maximum number of fields read from a struct, -1 will read all fields. MaxStructFields int + + // MaxMapBuckets is the maximum number of map buckets to read before giving up. + // A value of 0 will read as many buckets as necessary until the entire map + // is read or MaxArrayValues is reached. + // + // Loading a map is an operation that issues O(num_buckets) operations. + // Normally the number of buckets is proportional to the number of elements + // in the map, since the runtime tries to keep the load factor of maps + // between 40% and 80%. + // + // It is possible, however, to create very sparse maps either by: + // a) adding lots of entries to a map and then deleting most of them, or + // b) using the make(mapType, N) expression with a very large N + // + // When this happens delve will have to scan many empty buckets to find the + // few entries in the map. + // MaxMapBuckets can be set to avoid annoying slowdowns␣while reading + // very sparse maps. + // + // Since there is no good way for a user of delve to specify the value of + // MaxMapBuckets, this field is not actually exposed through the API. + // Instead (*EvalScope).LocalVariables and (*EvalScope).FunctionArguments + // set this field automatically to MaxArrayValues * maxMapBucketsFactor. + // Every other invocation uses the default value of 0, obtaining the old behavior. + // In practice this means that debuggers using the ListLocalVars or + // ListFunctionArgs API will not experience a massive slowdown when a very + // sparse map is in scope, but evaluating a single variable will still work + // correctly, even if the variable in question is a very sparse map. + MaxMapBuckets int } -var loadSingleValue = LoadConfig{false, 0, 64, 0, 0} -var loadFullValue = LoadConfig{true, 1, 64, 64, -1} +var loadSingleValue = LoadConfig{false, 0, 64, 0, 0, 0} +var loadFullValue = LoadConfig{true, 1, 64, 64, -1, 0} // G status, from: src/runtime/runtime2.go const ( @@ -441,7 +472,7 @@ func (v *Variable) parseG() (*G, error) { } v = v.maybeDereference() } - v.loadValue(LoadConfig{false, 2, 64, 0, -1}) + v.loadValue(LoadConfig{false, 2, 64, 0, -1, 0}) if v.Unreadable != nil { return nil, v.Unreadable } @@ -591,7 +622,7 @@ func (g *G) stkbar() ([]savedLR, error) { if g.stkbarVar == nil { // stack barriers were removed in Go 1.9 return nil, nil } - g.stkbarVar.loadValue(LoadConfig{false, 1, 0, int(g.stkbarVar.Len), 3}) + g.stkbarVar.loadValue(LoadConfig{false, 1, 0, int(g.stkbarVar.Len), 3, 0}) if g.stkbarVar.Unreadable != nil { return nil, fmt.Errorf("unreadable stkbar: %v", g.stkbarVar.Unreadable) } @@ -658,6 +689,7 @@ func (scope *EvalScope) LocalVariables(cfg LoadConfig) ([]*Variable, error) { vars = filterVariables(vars, func(v *Variable) bool { return (v.Flags & (VariableArgument | VariableReturnArgument)) == 0 }) + cfg.MaxMapBuckets = maxMapBucketsFactor * cfg.MaxArrayValues loadValues(vars, cfg) return vars, nil } @@ -671,6 +703,7 @@ func (scope *EvalScope) FunctionArguments(cfg LoadConfig) ([]*Variable, error) { vars = filterVariables(vars, func(v *Variable) bool { return (v.Flags & (VariableArgument | VariableReturnArgument)) != 0 }) + cfg.MaxMapBuckets = maxMapBucketsFactor * cfg.MaxArrayValues loadValues(vars, cfg) return vars, nil } @@ -1569,6 +1602,11 @@ func (v *Variable) loadMap(recurseLevel int, cfg LoadConfig) { if it == nil { return } + it.maxNumBuckets = uint64(cfg.MaxMapBuckets) + + if v.Len == 0 || int64(v.mapSkip) >= v.Len || cfg.MaxArrayValues == 0 { + return + } for skip := 0; skip < v.mapSkip; skip++ { if ok := it.next(); !ok { @@ -1580,9 +1618,6 @@ func (v *Variable) loadMap(recurseLevel int, cfg LoadConfig) { count := 0 errcount := 0 for it.next() { - if count >= cfg.MaxArrayValues { - break - } key := it.key() var val *Variable if it.values.fieldType.Size() > 0 { @@ -1601,6 +1636,9 @@ func (v *Variable) loadMap(recurseLevel int, cfg LoadConfig) { if errcount > maxErrCount { break } + if count >= cfg.MaxArrayValues || int64(count) >= v.Len { + break + } } } @@ -1618,6 +1656,8 @@ type mapIterator struct { values *Variable overflow *Variable + maxNumBuckets uint64 // maximum number of buckets to scan + idx int64 } @@ -1683,6 +1723,10 @@ func (it *mapIterator) nextBucket() bool { } else { it.b = nil + if it.maxNumBuckets > 0 && it.bidx >= it.maxNumBuckets { + return false + } + for it.bidx < it.numbuckets { it.b = it.buckets.clone() it.b.Addr += uintptr(uint64(it.buckets.DwarfType.Size()) * it.bidx) diff --git a/service/api/conversions.go b/service/api/conversions.go index 9943b504..3daf42cb 100644 --- a/service/api/conversions.go +++ b/service/api/conversions.go @@ -283,6 +283,7 @@ func LoadConfigToProc(cfg *LoadConfig) *proc.LoadConfig { cfg.MaxStringLen, cfg.MaxArrayValues, cfg.MaxStructFields, + 0, // MaxMapBuckets is set internally by pkg/proc, read its documentation for an explanation. } } diff --git a/service/debugger/locations.go b/service/debugger/locations.go index 32289d4e..5595a446 100644 --- a/service/debugger/locations.go +++ b/service/debugger/locations.go @@ -265,7 +265,7 @@ func (loc *AddrLocationSpec) Find(d *Debugger, scope *proc.EvalScope, locStr str } return []api.Location{{PC: uint64(addr)}}, nil } else { - v, err := scope.EvalExpression(loc.AddrExpr, proc.LoadConfig{true, 0, 0, 0, 0}) + v, err := scope.EvalExpression(loc.AddrExpr, proc.LoadConfig{true, 0, 0, 0, 0, 0}) if err != nil { return nil, err } diff --git a/service/rpc1/server.go b/service/rpc1/server.go index 10a63456..eae6d87a 100644 --- a/service/rpc1/server.go +++ b/service/rpc1/server.go @@ -10,7 +10,7 @@ import ( "github.com/derekparker/delve/service/debugger" ) -var defaultLoadConfig = proc.LoadConfig{true, 1, 64, 64, -1} +var defaultLoadConfig = proc.LoadConfig{true, 1, 64, 64, -1, 0} type RPCServer struct { // config is all the information necessary to start the debugger and server. diff --git a/service/test/variables_test.go b/service/test/variables_test.go index 579c6b07..8e9e8837 100644 --- a/service/test/variables_test.go +++ b/service/test/variables_test.go @@ -17,8 +17,8 @@ import ( protest "github.com/derekparker/delve/pkg/proc/test" ) -var pnormalLoadConfig = proc.LoadConfig{true, 1, 64, 64, -1} -var pshortLoadConfig = proc.LoadConfig{false, 0, 64, 0, 3} +var pnormalLoadConfig = proc.LoadConfig{true, 1, 64, 64, -1, 0} +var pshortLoadConfig = proc.LoadConfig{false, 0, 64, 0, 3, 0} type varTest struct { name string