Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/gopls: duplicated analysis work #61508

Closed
adonovan opened this issue Jul 21, 2023 · 3 comments
Closed

x/tools/gopls: duplicated analysis work #61508

adonovan opened this issue Jul 21, 2023 · 3 comments
Assignees
@findleyr
Labels
gopls/performance Issues related to gopls performance (CPU, memory, etc). gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.13.0

Comments

@adonovan
Copy link
Member

adonovan commented Jul 21, 2023 •
edited

The following log events were observed in a single run of "gopls check" (with staticcheck and nilness enabled) that was instrumented to report start/end calls to Analyze:

	t0 := time.Now()
	log.Println("Analyze start", analyzers, pkgs, t0)
	defer func() {
		log.Println("Analyze end", analyzers, pkgs, t0, time.Since(t0)) // repeat t0 so we can tie start/end together
	}()

Observe that two concurrent Analyze calls request that the same package be analyzed with two different sets of analyzers.

ec2$ (cd ~/w/xtools && go build -o x ./gopls ) && time ~/w/xtools/x --profile.cpu=prof check ./internal/customizations/unit_test.go

2023/07/21 11:38:11 Analyze start [structtag stdmethods lostcancel unusedresult unusedparams copylocks unusedwrite timeformat buildtag bools embed shift sortslice loopclosure simplifyslice useany cgocall errorsas simplifyrange printf fieldalignment atomicalign unsafeptr assign testinggoroutine nilness ifaceassert composites nilfunc unmarshal directive stringintconv atomic simplifycompositelit deepequalerrors httpresponse unreachable asmdecl shadow tests QF1012 SA9002 S1007 ST1019 ST1021 ST1003 SA9005 SA1015 QF1010 SA9004 SA1029 SA4006 ST1000 QF1005 SA1019 SA1021 ST1013 SA1026 QF1002 ST1005 SA4030 SA9007 S1034 SA4016 SA1014 SA5002 SA1007 SA4021 SA4005 SA5001 SA1025 S1035 S1010 SA5000 SA1024 S1011 SA4014 SA6005 SA6001 S1018 SA2001 SA4009 SA5008 ST1016 S1028 S1006 QF1007 SA9003 SA4010 ST1012 QF1003 SA1000 S1005 ST1022 SA5004 S1000 S1023 QF1011 S1029 SA4024 SA1020 S1009 SA2002 SA4011 SA1030 SA4022 QF1006 S1038 SA4000 SA5007 S1020 S1031 S1021 S1032 SA1010 QF1004 ST1001 SA4029 SA1001 SA1012 S1016 SA1013 SA4012 SA4025 ST1023 S1003 SA1008 SA4013 S1017 SA4019 SA1023 SA4026 QF1008 SA9008 S1037 SA6003 SA1016 SA4008 ST1011 ST1018 SA1003 SA4001 QF1001 ST1017 S1002 S1040 SA1028 SA4018 SA1027 SA1004 SA3001 SA5003 S1039 SA1017 S1036 S1024 QF1009 ST1020 S1008 SA4015 SA2003 S1001 SA4003 SA4028 SA1006 SA4023 SA1011 SA5005 SA5010 S1030 SA6000 S1004 SA4017 SA9006 SA4004 SA2000 SA1002 SA6002 SA5012 SA1005 SA9001 SA3000 ST1008 SA1018 S1033 ST1006 SA4031 SA4020 ST1015 S1019 S1012 SA4027 S1025 nonewvars noresultvalues undeclaredname unusedvariable fillreturns] map[github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations_test [github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations.test]:{}] 2023-07-21 11:38:11.38853 -0400 EDT m=+0.728676834

2023/07/21 11:38:12 Analyze start [bools embed shift sortslice loopclosure simplifyslice unsafeptr useany cgocall errorsas simplifyrange printf fieldalignment atomicalign assign testinggoroutine nilness ifaceassert composites nilfunc unmarshal directive stringintconv atomic simplifycompositelit deepequalerrors httpresponse unreachable asmdecl shadow tests structtag stdmethods lostcancel unusedresult unusedparams copylocks unusedwrite timeformat buildtag SA4000 SA5007 S1020 S1031 S1021 S1032 SA1010 QF1004 ST1001 SA4029 SA1001 SA1012 S1016 SA1013 SA4012 SA4025 ST1023 S1003 SA1008 SA4013 S1017 SA4019 SA1023 SA4026 QF1008 SA9008 S1037 SA6003 SA1016 SA4008 ST1011 ST1018 SA1003 SA4001 QF1001 ST1017 S1002 S1040 SA1028 SA4018 SA1027 SA1004 SA3001 SA5003 S1039 SA1017 S1036 S1024 QF1009 ST1020 S1008 SA4015 SA2003 S1001 SA4003 SA4028 SA1006 SA4023 SA1011 S1030 SA6000 S1004 SA4017 SA9006 SA4004 SA5005 SA5010 SA2000 SA1002 SA6002 SA5012 SA1005 SA9001 SA3000 S1033 ST1006 SA4031 SA4020 ST1015 S1019 ST1008 SA1018 S1012 SA4027 S1025 SA9002 S1007 ST1019 ST1021 ST1003 SA9005 QF1012 SA1015 QF1010 SA9004 SA1029 SA4006 ST1000 QF1005 SA1019 SA1021 ST1013 SA1026 QF1002 ST1005 S1034 SA4016 SA1014 SA5002 SA1007 SA4021 SA4030 SA9007 SA4005 SA5001 SA1025 SA5000 SA1024 S1011 SA4014 SA6005 SA6001 S1035 S1010 SA4009 SA5008 ST1016 S1028 S1006 QF1007 S1018 SA2001 ST1012 QF1003 SA1000 S1005 ST1022 SA5004 SA9003 SA4010 S1000 S1023 SA4024 SA1020 S1009 SA2002 SA4011 SA1030 QF1011 S1029 SA4022 QF1006 S1038 noresultvalues undeclaredname unusedvariable fillreturns nonewvars] map[github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations_test [github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations.test]:{}] 2023-07-21 11:38:12.39064 -0400 EDT m=+1.730787501


2023/07/21 11:38:14 Analyze end [] map[github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations_test [github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations.test]:{}] 2023-07-21 11:38:11.38853 -0400 EDT m=+0.728676834 3.175811292s

2023/07/21 11:38:14 Analyze end [] map[github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations_test [github.com/aws/aws-sdk-go-v2/service/ec2/internal/customizations.test]:{}] 2023-07-21 11:38:12.39064 -0400 EDT m=+1.730787501 2.341551917s

The different sets arise from the two calls to source.Analyze, from diagnosePkg and codeAction, providing different values of the "includeConvenience" boolean, which varies the set. However, the facty subset of both sets is the same, so in principle they should be able to work synergistically.

Even if the flag is always true, the concurrent calls both create a DAG of analysisNodes for a batch of work, and execute it in parallel, and both encounter the slow ec2 package (1s, see #61506), so both make cache misses, even though they want the same computation. The only sharing is of completed analysis summaries via the file cache; there is no de-duplication of inflight requests (i.e. the first thread doesn't "lick the cookie"). This is problematic for large packages like ec2 because they are expensive to recompute, and more likely to be recomputed because their inflight duration is longer, increasing the odds of a concurrent request.

One solution would be to dedup using singleflight or a promise cache, but the logic could be tricky. Another would to somehow stagger the requests (codeAction and diagnosePkg) during initial indexing to make concurrent requests for large packages less likely; once the cache is populated subsequent operation is fast.

It's worth noting that this causes at worst a factor of 2x slowdown, which is a lot, but still much less than the current difference between actual and ideal performance of buildssa (see #61506).
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jul 21, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jul 21, 2023
@adonovan adonovan added the gopls/performance Issues related to gopls performance (CPU, memory, etc). label Jul 21, 2023
@findleyr findleyr self-assigned this Jul 24, 2023
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.12.5 Jul 24, 2023
@findleyr
Copy link
Contributor

I'm eliminating the codeAction call to analysis.

@gopherbot
Copy link

Change https://go.dev/cl/512495 mentions this issue: gopls/internal/regtest/bench: add benchmarks for codeactions

@gopherbot
Copy link

Change https://go.dev/cl/511995 mentions this issue: gopls/internal/lsp: don't recompute diagnostics during code actions

gopherbot pushed a commit to golang/tools that referenced this issue Jul 25, 2023
@findleyr
gopls/internal/regtest/bench: add benchmarks for codeactions
fa093b2 
Add missing benchmarks for codeactions.

For golang/go#61508

Change-Id: I260fdae1c70c2e2291b7469cb7240d3d68cf1ded
Reviewed-on: https://go-review.googlesource.com/c/tools/+/512495
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@findleyr findleyr

Labels
gopls/performance Issues related to gopls performance (CPU, memory, etc). gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.13.0
Development

No branches or pull requests
3 participants
@adonovan @gopherbot @findleyr