-
Notifications
You must be signed in to change notification settings - Fork 2.3k
golangci-lint: add prealloc linter + manually fix problems
#18901
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.
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
base: main
Are you sure you want to change the base?
golangci-lint: add prealloc linter + manually fix problems
#18901
Conversation
Signed-off-by: Tim Vaillancourt <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Tim Vaillancourt <[email protected]>
| exportedStructFields = make([]reflect.StructField, 0, len(visibleFields)) | ||
| fields = make([]*querypb.Field, 0, len(visibleFields)) | ||
| rows = make([]Row, 0, val.Len()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude pointed out here that the len(visibleFields) will most likely over-allocate, because not all fields might be exported. I don't think that's an issue, just potentially wasteful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is expected because we sometimes continue. But an upper limit should be more efficient than a capacity of zero
|
This PR is also causing this unit test failure because of a change to 2025-11-17T16:59:18.2318158Z === RUN TestPlanTestSuite/TestPlan/misc_cases.json/execute_no_param_statement
2025-11-17T16:59:18.2318412Z plan_test.go:704: misc_cases.json
2025-11-17T16:59:18.2318509Z Diff:
2025-11-17T16:59:18.2318593Z {
2025-11-17T16:59:18.2318748Z "Instructions": {
2025-11-17T16:59:18.2318906Z "Inputs": [
2025-11-17T16:59:18.2319063Z {
2025-11-17T16:59:18.2319517Z "FieldQuery": "select 1 from `user` where 1 != 1",
2025-11-17T16:59:18.2319746Z "Keyspace": {
2025-11-17T16:59:18.2320009Z "Name": "user",
2025-11-17T16:59:18.2320276Z "Sharded": true
2025-11-17T16:59:18.2320437Z },
2025-11-17T16:59:18.2320725Z "OperatorType": "Route",
2025-11-17T16:59:18.2321061Z "Query": "select 1 from `user`",
2025-11-17T16:59:18.2321316Z "Variant": "Scatter"
2025-11-17T16:59:18.2321444Z }
2025-11-17T16:59:18.2321560Z ],
2025-11-17T16:59:18.2321775Z "OperatorType": "EXECUTE",
2025-11-17T16:59:18.2322220Z "Parameters": �[0;33mnull => []�[0m
2025-11-17T16:59:18.2322315Z },
2025-11-17T16:59:18.2322531Z "Original": "execute prep_no_param",
2025-11-17T16:59:18.2322703Z "QueryType": "EXECUTE",
2025-11-17T16:59:18.2322841Z "TablesUsed": [
2025-11-17T16:59:18.2322988Z "user.user"
2025-11-17T16:59:18.2323087Z ],
2025-11-17T16:59:18.2323231Z "Type": "Complex"
2025-11-17T16:59:18.2323309Z }
2025-11-17T16:59:18.2323391Z [{
2025-11-17T16:59:18.2323552Z "Type": "Complex",
2025-11-17T16:59:18.2323742Z "QueryType": "EXECUTE",
2025-11-17T16:59:18.2323974Z "Original": "execute prep_no_param",
2025-11-17T16:59:18.2324132Z "Instructions": {
2025-11-17T16:59:18.2324422Z "OperatorType": "EXECUTE",
2025-11-17T16:59:18.2324611Z "Parameters": null,
2025-11-17T16:59:18.2324755Z "Inputs": [
2025-11-17T16:59:18.2324872Z {
2025-11-17T16:59:18.2325116Z "OperatorType": "Route",
2025-11-17T16:59:18.2325336Z "Variant": "Scatter",
2025-11-17T16:59:18.2325523Z "Keyspace": {
2025-11-17T16:59:18.2325734Z "Name": "user",
2025-11-17T16:59:18.2325950Z "Sharded": true
2025-11-17T16:59:18.2326139Z },
2025-11-17T16:59:18.2326528Z "FieldQuery": "select 1 from `user` where 1 != 1",
2025-11-17T16:59:18.2326808Z "Query": "select 1 from `user`"
2025-11-17T16:59:18.2326918Z }
2025-11-17T16:59:18.2327019Z ]
2025-11-17T16:59:18.2327124Z },
2025-11-17T16:59:18.2327271Z "TablesUsed": [
2025-11-17T16:59:18.2327416Z "user.user"
2025-11-17T16:59:18.2327522Z ]
2025-11-17T16:59:18.2327615Z }]
2025-11-17T16:59:18.2327731Z [{
2025-11-17T16:59:18.2327974Z "Type": "Complex",
2025-11-17T16:59:18.2328120Z "QueryType": "EXECUTE",
2025-11-17T16:59:18.2328318Z "Original": "execute prep_no_param",
2025-11-17T16:59:18.2328447Z "Instructions": {
2025-11-17T16:59:18.2328622Z "OperatorType": "EXECUTE",
2025-11-17T16:59:18.2328839Z "Parameters": [],
2025-11-17T16:59:18.2328967Z "Inputs": [
2025-11-17T16:59:18.2329062Z {
2025-11-17T16:59:18.2329275Z "OperatorType": "Route",
2025-11-17T16:59:18.2329459Z "Variant": "Scatter",
2025-11-17T16:59:18.2329617Z "Keyspace": {
2025-11-17T16:59:18.2329791Z "Name": "user",
2025-11-17T16:59:18.2329963Z "Sharded": true
2025-11-17T16:59:18.2330075Z },
2025-11-17T16:59:18.2330405Z "FieldQuery": "select 1 from `user` where 1 != 1",
2025-11-17T16:59:18.2330721Z "Query": "select 1 from `user`"
2025-11-17T16:59:18.2330823Z }
2025-11-17T16:59:18.2330911Z ]
2025-11-17T16:59:18.2331003Z },
2025-11-17T16:59:18.2331129Z "TablesUsed": [
2025-11-17T16:59:18.2331250Z "user.user"
2025-11-17T16:59:18.2331339Z ]
2025-11-17T16:59:18.2331415Z }
2025-11-17T16:59:18.2331494Z ] |
Signed-off-by: Tim Vaillancourt <[email protected]>
|
|
||
| func generateOwnedVindexQuery(del *sqlparser.Delete, table TargetTable, ksidCols []sqlparser.IdentifierCI) *sqlparser.Select { | ||
| var selExprs []sqlparser.SelectExpr | ||
| selExprs := make([]sqlparser.SelectExpr, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right? No len(ksidCols)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbussink I kept capacity zero here because of this sub-loop that I don't really know the capacity length of up-front:
vitess/go/vt/vtgate/planbuilder/operators/delete.go
Lines 272 to 275 in 9e5aeb6
| for _, col := range cv.Columns { | |
| colName := makeColName(col, table, sqlparser.MultiTable(del.TableExprs)) | |
| selExprs = append(selExprs, aeWrap(colName)) | |
| } |
| } | ||
|
|
||
| var newOption []*VindexOption | ||
| newOption := make([]*VindexOption, 0, len(v.Options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about this specific case, but in general, is it really correct to allocate it all everywhere? Do we not have cases where it's more the exception an element is added and not the common case? And we very much over-allocate?
That's a general issue with this linter, it's not very smart and complains about every case, while not every case makes really sense to convert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about this specific case, but in general, is it really correct to allocate it all everywhere?
@dbussink I haven't validated it myself, but I've consistently read (and just confirmed with ChatGPG) that allocating a slice with a fixed capacity is "usually" more efficient, because a zero-capacity slice has to be grown every append, causing new allocations for a new backing array each append
To my knowledge the risk to using a capacity that may/may not be 100% used is memory usage that may just be filled with empty values. But there must be cases where this isn't always true, perhaps really huge slices(?). I wondered if you had any scenarios in mind?
Signed-off-by: Tim Vaillancourt <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Description
This PR introduces the
prealloclinter to ourgolangci-lintconfig, in order to reduce allocations in the runtimeIncluded are manual fixes to 116 files 😱 that should be reviewed carefully. Here is the method I followed:
intrange-loop, I set the cap == theintlen(whatWeRangedOn)appends, I set the cap ==len(whatWeRangedOn) + #, where#is the number of individualappends0at some minor costappends a slice needs, I just used a cap of0While I was cleaning up, I removed the empty
.gitmodulesfile in the repo root, and mentions ofGodepsandphpin.dockerfile+.gitignore(both are long gone) 🧹Related Issue(s)
Checklist
Deployment Notes
AI Disclosure