Improvements to ListGoroutines #1440

aarzilli · 2018-12-06T09:43:22Z

service: make ListGoroutines return all running goroutines when
Start==0

Change ListGoroutines API call so that it tries to fit all running
goroutines in the ListGoroutines(Start=0) results.

Users are more likely to be interested in running goroutines, with this
change a frontend that just calls ListGoroutines(Start=0, Count=n) is
guaranteed to show all running goroutines to its users (as long as
there aren't more than n running goroutines).

proc: improve performance of FindGoroutine in normal circumstances

FindGoroutine can be slow when there are many goroutines running. This
can not be fixed in the general case, however:

1. Instead of getting the entire list of goroutines at once just get a
few at a time and return as soon as we find the one we want.

2. Since FindGoroutine is mostly called by ConvertEvalScope and users
are more likely to request informations about a goroutine running on a
thread, look within the threads first.

proc: fix GoroutinesInfo cache

The allg cache was corrupted when the count parameter was actually
reached.

Fix the bug and add a test for this.

aarzilli · 2018-12-06T09:58:08Z

This is a series of improvements to GoroutinesInfo/ListGoroutines connected to microsoft/vscode-go#2133:

Fix a bug in GoroutinesInfo where the wrong list would be cached in allgCache
Improve performance of FindGoroutine, especially when it's called with the gid of a running goroutine
Change API call ListGoroutines to always return running goroutines in the first batch (if Count != 0 is specified), This is a quality of life improvement, it means that a front end can just call ListGoroutines(0, n) and its guaranteed to get all running goroutines (unless n is very small or there is a pathologically large number of threads).

derekparker

I like the improvement to FindGoroutine and the cache fix, however I think we can create a smarter cache in the future.

What I'm not convinced of is the semantics of using start to signal a sorting of goroutines by trying to return only running first. I think maybe it's better to be explicit and add a parameter runningOnly bool to control this behavior, that way the logic becomes much simpler and it's more declarative.

Sorting and prioritizing running goroutines can be difficult because of how we're essentially trying to paginate the list of goroutines.

derekparker · 2018-12-12T17:06:29Z

service/debugger/debugger.go

@@ -916,7 +917,44 @@ func (d *Debugger) Goroutines(start, count int) ([]*api.Goroutine, int, error) {
 	if err != nil {
 		return nil, 0, err
 	}
+	if start > 0 || nextg >= 0 {
+		// Ensure that the first batch of goroutines always contains all running


nit: I think we should reword this comment, as we don't make any explicit guarentee that we are returning all running goroutines in the first batch, as that's limited by the count the user specifies. It would suffice, I think, to simply say that we sort and prioritize running goroutines first.

derekparker · 2018-12-12T17:13:15Z

service/debugger/debugger.go

@@ -916,7 +917,44 @@ func (d *Debugger) Goroutines(start, count int) ([]*api.Goroutine, int, error) {
 	if err != nil {
 		return nil, 0, err
 	}
+	if start > 0 || nextg >= 0 {


I'm confused, here we're checking if start > 0 and then trying to return all running goroutines, however in the commit and documentation it's specified that we only want to prioritize and return only running goroutines when start == 0. This leads us to having a bunch of conditionals below, I think we can clean this up by having this check first if start ==0, no?

There are two paths here: start == 0 && nextg >= 0 where we add running goroutines and start > 0 where we remove them (because we returned them before when we had start == 0). It is a bit confusing as it is, I rewrote it by factoring out the common code and moving the conditions outside, hopefully it's clearer.

derekparker · 2018-12-13T18:58:19Z

service/debugger/debugger.go

@@ -906,6 +907,25 @@ func (d *Debugger) SetVariableInScope(scope api.EvalScope, symbol, value string)
 	return s.SetVariable(symbol, value)
 }

+func (d *Debugger) getRunningGoroutines(count int) map[int]*proc.G {
+	threads := d.target.ThreadList()
+	sort.Slice(threads, func(i, j int) bool { return threads[i].ThreadID() < threads[j].ThreadID() })


What does sorting buy us here?

The output of ThreadList is not guaranteed to always be in the same order (because the native backend is converting from a map).

Right, but we just iterate them and them stuff them into another map and return it, I don't see why we should sort them first, order doesn't really matter here unless I'm missing something obvious.

Is this to make the call idempotent when called with a same count value?

derekparker · 2019-01-05T00:06:01Z

service/debugger/debugger.go

@@ -916,7 +936,31 @@ func (d *Debugger) Goroutines(start, count int) ([]*api.Goroutine, int, error) {
 	if err != nil {
 		return nil, 0, err
 	}
+
+	// Prioritize running goroutines in the first batch we return (adds up to
+	// 'count' running goroutines to the first batch and insures that they


s/insures/ensures/

derekparker · 2019-01-05T00:19:09Z

service/debugger/debugger.go

+	// Prioritize running goroutines in the first batch we return (adds up to
+	// 'count' running goroutines to the first batch and insures that they
+	// aren't returned a second time in the batches after the first one).
+	if start > 0 {


I still don't like the semantics here, and I think it would be better to have a parameter like onlyRunning if a caller is only concerned with running Goroutines.

Consider a caller specifies start == 0, count == 5, then (and I don't know why they would, but just consider) they call it again with start == 1, count ==5. All running goroutines would be removed from the list, which just kind of seems like odd behavior. Lastly, I don't think we should ever return more than count items, that's just very unexpected behavior.

It doesn't make sense to call proc.GoroutinesInfo with arbitrary values of start, it will skip an arbitrary number n (with 0 <= n < start).

You've convinced me that this feature (returning running goroutines first) isn't worth it. I don't think we can implement it without the weird behavior and since running goroutines are returned as part of GetState it isn't that useful anyway.

I've removed this code and added an exetended explanation of Start and Count.

The allg cache was corrupted when the count parameter was actually reached. Fix the bug and add a test for this.

FindGoroutine can be slow when there are many goroutines running. This can not be fixed in the general case, however: 1. Instead of getting the entire list of goroutines at once just get a few at a time and return as soon as we find the one we want. 2. Since FindGoroutine is mostly called by ConvertEvalScope and users are more likely to request informations about a goroutine running on a thread, look within the threads first.

Describe how the Start and Count parameters of ListGoroutines are used.

derekparker

LGTM

derekparker requested changes Dec 12, 2018

View reviewed changes

aarzilli force-pushed the findgperf branch from 00123db to 1a45a89 Compare December 13, 2018 07:38

derekparker reviewed Dec 13, 2018

View reviewed changes

derekparker reviewed Jan 5, 2019

View reviewed changes

aarzilli added 3 commits January 5, 2019 08:55

proc: fix GoroutinesInfo cache

db9b865

The allg cache was corrupted when the count parameter was actually reached. Fix the bug and add a test for this.

service: improve documentation of ListGoroutines

6063e4d

Describe how the Start and Count parameters of ListGoroutines are used.

aarzilli force-pushed the findgperf branch from 1a45a89 to 6063e4d Compare January 5, 2019 08:34

derekparker approved these changes Jan 7, 2019

View reviewed changes

derekparker merged commit 86c4b72 into go-delve:master Jan 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to ListGoroutines #1440

Improvements to ListGoroutines #1440

aarzilli commented Dec 6, 2018

aarzilli commented Dec 6, 2018

derekparker left a comment

derekparker Dec 12, 2018

derekparker Dec 12, 2018

aarzilli Dec 13, 2018

derekparker Dec 13, 2018

aarzilli Dec 13, 2018

derekparker Jan 5, 2019

derekparker Jan 5, 2019

aarzilli Jan 5, 2019

derekparker Jan 5, 2019

derekparker Jan 5, 2019 •

edited

Loading

aarzilli Jan 5, 2019

derekparker left a comment

Improvements to ListGoroutines #1440

Improvements to ListGoroutines #1440

Conversation

aarzilli commented Dec 6, 2018

aarzilli commented Dec 6, 2018

derekparker 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

Choose a reason for hiding this comment

derekparker Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekparker left a comment

Choose a reason for hiding this comment

derekparker Jan 5, 2019 •

edited

Loading