[tools] gopls: remove a use of panic for flow control
Peter Weinberger (Gerrit)
Peter Weinberger has uploaded this change for review.
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go
index c758d68..be1d9c8 100644
--- a/gopls/internal/lsp/source/hover.go
+++ b/gopls/internal/lsp/source/hover.go
@@ -895,15 +895,6 @@
//
// It returns (nil, nil) if no Field or Decl is found at pos.
func FindDeclAndField(files []*ast.File, pos token.Pos) (decl ast.Decl, field *ast.Field) {
- // panic(nil) breaks off the traversal and
- // causes the function to return normally.
- defer func() {
- if x := recover(); x != nil {
- panic(x)
- }
- }()
-
- // Visit the files in search of the node at pos.
var stack []ast.Node
for _, file := range files {
ast.Inspect(file, func(n ast.Node) bool {
@@ -913,7 +904,9 @@
stack = stack[:len(stack)-1] // pop
return false
}
-
+ if decl != nil || field != nil {
+ return false
+ }
// Skip subtrees (incl. files) that don't contain the search point.
if !(n.Pos() <= pos && pos < n.End()) {
return false
@@ -921,39 +914,45 @@
switch n := n.(type) {
case *ast.Field:
- checkField := func(f ast.Node) {
+ checkField := func(f ast.Node) bool {
if f.Pos() == pos {
field = n
for i := len(stack) - 1; i >= 0; i-- {
if d, ok := stack[i].(ast.Decl); ok {
decl = d // innermost enclosing decl
- break
+ return true
}
}
- panic(nil) // found
}
+ return false
}
// Check *ast.Field itself. This handles embedded
// fields which have no associated *ast.Ident name.
- checkField(n)
+ if checkField(n) {
+ return false
+ }
// Check each field name since you can have
// multiple names for the same type expression.
for _, name := range n.Names {
- checkField(name)
+ if checkField(name) {
+ return false
+ }
}
// Also check "X" in "...X". This makes it easy
// to format variadic signature params properly.
if ell, ok := n.Type.(*ast.Ellipsis); ok && ell.Elt != nil {
- checkField(ell.Elt)
+ if checkField(ell.Elt) {
+ return false
+ }
}
case *ast.FuncDecl:
if n.Name.Pos() == pos {
decl = n
- panic(nil) // found
+ return false
}
case *ast.GenDecl:
@@ -962,13 +961,13 @@
case *ast.TypeSpec:
if spec.Name.Pos() == pos {
decl = n
- panic(nil) // found
+ return false
}
case *ast.ValueSpec:
for _, id := range spec.Names {
if id.Pos() == pos {
decl = n
- panic(nil) // found
+ return false
}
}
}
@@ -977,6 +976,5 @@
return true
})
}
-
- return nil, nil
+ return
}
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/6066a265-5d76-492a-aaba-1dcf76ea9813
Patch set 1:gopls-CI +1
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #2 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Peter Weinberger, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 92 insertions(+), 84 deletions(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Peter Weinberger.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/3919be0c-b1d9-4c0a-a3cd-3ae9652e24b0
Patch set 2:gopls-CI +1
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Peter Weinberger, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 32 insertions(+), 23 deletions(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Peter Weinberger.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/63f2b32a-6920-4a75-b24d-010dc124b656
Patch set 3:gopls-CI +1
Robert Findley (Gerrit)
Attention is currently required from: Peter Weinberger.
Patch set 3:Code-Review +2
3 comments:
Patchset:
LGTM, with comments, but I think we should re-run whatever benchmark inspired these optimizations, to ensure that there is no performance implication of this change.
File gopls/internal/lsp/source/hover.go:
Patch Set #3, Line 898:
var stack []ast.NodeIIRC, this function is very hot, and the recent pre-allocation of stack had a meaningful impact on performance. In that case, we should preserve it but leave a comment explaining why the seemingly premature optimization is not, in fact, premature.
Patch Set #3, Line 922:
return trueThis changes the logic of checkField: we only return true if we find both field AND decl.
Probably it doesn't make a difference, but I think it is best to preserve the previous behavior exactly, breaking here and returning true after the for loop.
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Peter Weinberger.
1 comment:
Patchset:
+Alan who implemented the optimizations of this function, for context.
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #4 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Peter Weinberger, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 30 insertions(+), 21 deletions(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Peter Weinberger.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/e9a1591e-902f-4911-bc6e-ce3ae6bc3cb9
Patch set 4:gopls-CI +1
Peter Weinberger (Gerrit)
2 comments:
File gopls/internal/lsp/source/hover.go:
Patch Set #3, Line 898:
var stack []ast.Node
IIRC, this function is very hot, and the recent pre-allocation of stack had a meaningful impact on p […]
Done
Patch Set #3, Line 922:
return true
This changes the logic of checkField: we only return true if we find both field AND decl. […]
Done
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Alan Donovan (Gerrit)
Attention is currently required from: Peter Weinberger.
The change is no longer submittable: No-Unresolved-Comments is unsatisfied now.
2 comments:
File gopls/internal/lsp/source/hover.go:
Patch Set #3, Line 907:
return falseIn defense of the old code, this transformation (from panic to return error in AST traversals) is more subtle than it appears. The old code would break out of the files loop as soon as it found a match, whereas the new code will visit every file unconditionally. Also, returning false from f causes the visitor not to call f(nil), so it won't pop the stack, so the stack will be nonempty when Inspect returns. This may cause some calls of f to see a meaningless or impossible stack. Then, the files loop will cause stack to continue to grow by one element per remaining file, perhaps exceeding the size of the deepest actual AST, again with unexpected contents.
To be clear, none of this matters much in this specific instance, but it's easy to imagine slight variants of the problem in which this innocuous code transformation introduces three separate bugs.
Patch Set #3, Line 922:
return true
Done
Unfortunately checkField now always returns false, which causes all the if statements below to do the wrong thing. I think it wants a "return true" after the inner for-loop.
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #5 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Peter Weinberger, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 115 insertions(+), 1 deletion(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #6 to this change.
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion. The new
code is somewhat fiddlier.
When running all the gopls tests, the old code returned 32,339 non-nil values.
The new code returns exactly the same results in all these cases.
The new code is generally faster than the old code. As they use wall-clock
times, the timings are somewhat noisy, but the median and 99th percentiles
were 1520ns, 12070ns for the new code, and 7870ns, 26500ns for the old.
For 270 of the 32,339 calls, the old code was faster.
The stack is preallocated to capacity 20. The 99th percentile of stack
size is 80, which is only 2 reallocations.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 62 insertions(+), 37 deletions(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Peter Weinberger.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/1c3917d2-c587-47ec-a6fe-259220a29725
Patch set 6:gopls-CI +1
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #7 to this change.
The following approvals got outdated and were removed: gopls-CI+1 by kokoro
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion. The new
code is somewhat fiddlier.
When running all the gopls tests, the old code returned 32,339 non-nil values.
The new code returns exactly the same results in all these cases.
The new code is generally faster than the old code. As they use wall-clock
times, the timings are somewhat noisy, but the median and 99th percentiles
were 1520ns, 12070ns for the new code, and 7870ns, 26500ns for the old.
For 270 of the 32,339 calls, the old code was faster.
The stack is preallocated to capacity 20. The 99th percentile of stack
size on the tests is 80, which is only 2 reallocations.
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 62 insertions(+), 37 deletions(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Peter Weinberger (Gerrit)
Attention is currently required from: Alan Donovan.
Patch set 7:Run-TryBot +1
2 comments:
File gopls/internal/lsp/source/hover.go:
Patch Set #3, Line 907:
return false
In defense of the old code, this transformation (from panic to return error in AST traversals) is mo […]
the CL description now addresses these points.
Patch Set #3, Line 922:
return true
Unfortunately checkField now always returns false, which causes all the if statements below to do th […]
It's a little worrying that no test picks up any of the failings mentioned by code reviewers.
But, in running all the gopls tests, the old code gives about 32,000 non-nil results, and the new code gives the identical results.
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Alan Donovan.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/2ceb15d6-aac1-4694-a5f0-130d8b58729a
Patch set 7:gopls-CI +1
Alan Donovan (Gerrit)
Attention is currently required from: Peter Weinberger.
Patch set 7:Code-Review +2
The change is no longer submittable: No-Unresolved-Comments is unsatisfied now.
5 comments:
Commit Message:
Patch Set #7, Line 18:
7870ns, 26500nsBig difference!
80's a lot. Given that we mostly follow the 80-ish column rule, I wonder what leads to such deep stacks. It's not because of long array literals, since the go/ast representation is a slice, not a linked list.
Patchset:
LGTM, two minor comments.
File gopls/internal/lsp/source/hover.go:
f :=
Let's keep the conventional capitalization: checkField.
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
Peter Weinberger (Gerrit)
Attention is currently required from: Peter Weinberger.
Peter Weinberger uploaded patch set #8 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Peter Weinberger, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
gopls: remove a use of panic for flow control
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion. The new
code is somewhat fiddlier.
When running all the gopls tests, the old code returned 32,339 non-nil values.
The new code returns exactly the same results in all these cases.
The new code is generally faster than the old code. As they use wall-clock
times, the timings are somewhat noisy, but the median and 99th percentiles
were 1520ns, 12070ns for the new code, and 7870ns, 26500ns for the old.
For 270 of the 32,339 calls, the old code was faster.
The stack is preallocated to capacity 20. The 99th percentile of stack
size is 80, which is only 2 reallocations. The large stacks appear to
happend looking in the go source tree while processing deep completions
Change-Id: I9dbffec5bfb9741bc1ffa1f3fa4c8bf3210567bd
---
M gopls/internal/lsp/source/hover.go
1 file changed, 61 insertions(+), 36 deletions(-)
To view, visit change 445337. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Peter Weinberger.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/7a4d8e02-b243-4fae-af85-7c8993147231
Patch set 8:gopls-CI +1
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +0 |
| Commit-Queue | +1 |
1 comment
While clearing some old tabs, I brought this CL up to date as https://go.dev/cl/563935.
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
(Ignore the commit queue, I hit the button out of habit. I think this CL can be abandoned.)
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |