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.

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

[stats] costed index scan perf #2421

Merged
merged 5 commits into from
Apr 2, 2024
Merged

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Mar 29, 2024

Histogram copying is expensive. Instead pass and mutate references. We have to use a different struct type to load stats from JSON in order to support histogram interface generalization.

related Dolt-side: dolthub/dolt#7666

@max-hoffman max-hoffman requested a review from zachmu April 1, 2024 18:01
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

func Union(s1, s2 sql.Statistic) sql.Statistic {
return s1
func Union(b1, b2 []sql.HistogramBucket, types []sql.Type) ([]sql.HistogramBucket, error) {
var ret []sql.HistogramBucket
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to allocate the full potential capacity here to avoid a bunch of mallocs during the loop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk

func Intersect(s1, s2 sql.Statistic) sql.Statistic {
return s1
func Intersect(b1, b2 []sql.HistogramBucket, types []sql.Type) ([]sql.HistogramBucket, error) {
var ret []sql.HistogramBucket
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about allocations. Could also consider a pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it starts to show up on profiles again i can add a pool, so far sysbench and tpc-c don't seem to execute this a lot

sql/stats/filter.go Outdated Show resolved Hide resolved
sql/stats/filter.go Outdated Show resolved Hide resolved
@max-hoffman max-hoffman merged commit a7c3dd0 into main Apr 2, 2024
7 checks passed
@max-hoffman max-hoffman deleted the max/cheaper-costed-index-scans branch April 2, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants