sizegen: do not ignore type aliases #17556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Vicent Marti <[email protected]>
vmg
requested review from
deepthi,
harshit-gangal,
mattlord,
frouioui,
systay and
shlomi-noach
as code owners
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
|
vitess-bot
bot
added
NeedsBackportReason
dbussink
added
Type: Bug
Component: Query Serving
Backport to: release-21.0
dbussink
reviewed
Jan 16, 2025
| @@ -163,6 +163,8 @@ func (sizegen *sizegen) generateTyp(tt types.Type) { | |||
| sizegen.generateKnownType(tt) | |||
| case *types.Alias: | |||
| sizegen.generateTyp(types.Unalias(tt)) | |||
| default: | |||
| panic(fmt.Sprintf("unhandled type: %v (%T)", tt, tt)) | |||
There was a problem hiding this comment.
We now make sure we panic here on unexpected values as well.
| default: | ||
| log.Printf("unhandled type: %T", node) | ||
| return nil, 0 | ||
| panic(fmt.Sprintf("unhandled type: %v (%T)", node, node)) |
There was a problem hiding this comment.
We should not just print a warning here, but actively panic. This is so that if we have other new types in the future, we don't ignore this again but actively investigate.
dbussink
approved these changes
Jan 16, 2025
vmg
commented
Jan 16, 2025
Comment on lines
+40
to
+47
| for _, elem := range cached.Rows { | ||
| { | ||
| size += hack.RuntimeAllocSize(int64(cap(elem)) * int64(32)) | ||
| for _, elem := range elem { | ||
| size += elem.CachedSize(false) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This was the big missing memory calculation for the size of in-memory query results.
systay
approved these changes
Jan 16, 2025
systay
approved these changes
Jan 16, 2025
vitess-bot
pushed a commit
that referenced
this pull request
Jan 16, 2025
Signed-off-by: Vicent Marti <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17556 +/- ##
==========================================
+ Coverage 67.68% 67.72% +0.03%
==========================================
Files 1584 1584
Lines 254718 254721 +3
==========================================
+ Hits 172414 172508 +94
+ Misses 82304 82213 -91 ☔ View full report in Codecov by Sentry. |
|
Isn't there a unit test that could have caught this? We should add one. |
That's why we changed this to |
dbussink
pushed a commit
that referenced
this pull request
Jan 16, 2025
Signed-off-by: Vicent Marti <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Important fix for memory calculations of in-memory data structures. Was causing OOMs in production.
In Go 1.23 a new type alias node was introduced for reflection, see golang/go#63223. This initially caused the sizegen generation to crash, which was fixed by @systay in #16650 but that fix was not complete. It still creates a ton of warnings since that change that were never fixed:
This might have looked innocent, but it certainly is not. The problem we found here specifically starts showing up in the
vttabletconsolidation logic. The problem is that we need these sizes to properly compute the cache. We have a type alias forRowto[]Valuefor the actual row storage which is now ignored.This means that the cache in use size for the consolidator did not actually count the row data! This is basically the main part of what it needs to measure. This in turns leads to excessive memory usage by the consolidator.
This also applies to the normal query consolidator, to the query cache, and any other piece of code that was depending on CachedSize to be accurate in order to limit vttablet memory usage. We're hoping this will cut down on the amount of OOMs we see in production.
cc @dbussink
Related Issue(s)
Checklist
Deployment Notes