Indexing: clean up ctags parser wrapper #708
Conversation
|
|
||
| parser := lp.parsers[typ] | ||
| recv := make(chan parseResult, 1) | ||
| go func() { |
There was a problem hiding this comment.
I simplified here by using a new goroutine and channel per parse request. It looked totally fine in benchmarks, but maybe this is bad practice. Any thoughts?
| return resp.entries, resp.err | ||
| case <-deadline.C: | ||
| // Error out since ctags hanging is a sign something bad is happening. | ||
| return nil, fmt.Errorf("ctags timedout after %s parsing %s", parseTimeout, name) |
There was a problem hiding this comment.
I decided to make this a hard error, in contrast to before where if require_ctags was not set, we attempted to restart the ctags process and move to the next document. It felt better to keep this simple, especially since we always enable require_ctags by default.
ctags/json.go
Outdated
| func (lp *CTagsParser) Close() { | ||
| for _, parser := range lp.parsers { | ||
| parser.Close() |
There was a problem hiding this comment.
the old implementation protected from calling lp.p.Close twice. Not sure if that is needed given Close likely can be called twice. Just checking :)
There was a problem hiding this comment.
My understanding: because of how parser.Close() is implemented, it is okay to call twice. I just tested this by adding an extra call to parser.Close() during indexing and it was fine. Also, we do try to structure things so that parser.Close() should ever only be called once.
ctags/json.go
Outdated
| go func() { | ||
| entry, err := parser.Parse(name, content) | ||
| recv <- parseResult{entries: entry, err: err} | ||
| }() |
There was a problem hiding this comment.
I think the overhead of a new goroutine is likely fine given we know ctags is already the bottleneck. I'd be interested to know if it pops up in profiles now.
There was a problem hiding this comment.
👍 I didn't see any difference in job latency or in the CPU profiles
|
Thanks for the review! |
This change cleans up the Go ctags parser wrapper as a follow-up to #702. Specific changes:
lockedParserand rename it toCTagsParserRelates to https://github.com/sourcegraph/sourcegraph/issues/58112