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

[statspro] Avoid copying histograms, perf improvement #7666

Merged
merged 10 commits into from
Apr 2, 2024

Conversation

max-hoffman
Copy link
Contributor

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

@max-hoffman
Copy link
Contributor Author

#benchmark

Copy link

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

test_name from_latency_median to_latency_median is_faster
tpcc-scale-factor-1 123.28 123.28 0
test_name server_name server_version tps test_name server_name server_version tps is_faster
tpcc-scale-factor-1 dolt 078e2a1 16.8 tpcc-scale-factor-1 dolt 5db54bf 17.23 0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

comparing_percentages
100.000000 to 100.000000
version result total
5db54bf ok 5937457
version total_tests
5db54bf 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 3.02 3.07 0
groupby_scan 17.63 17.32 0
index_join 5.18 5.18 0
index_join_scan 2.26 2.26 0
index_scan 63.32 63.32 0
oltp_point_select 0.49 0.49 0
oltp_read_only 8.28 8.28 0
select_random_points 0.78 0.78 0
select_random_ranges 0.95 0.95 0
table_scan 64.47 64.47 0
types_table_scan 179.94 179.94 0
write_tests from_latency_median to_latency_median is_faster
oltp_delete_insert 7.04 7.04 0
oltp_insert 3.49 3.49 0
oltp_read_write 16.12 16.12 0
oltp_update_index 3.62 3.62 0
oltp_update_non_index 3.55 3.55 0
oltp_write_only 8.13 8.13 0
types_delete_insert 7.84 7.84 0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

comparing_percentages
100.000000 to 100.000000
version result total
3915b05 ok 5937457
version total_tests
3915b05 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

comparing_percentages
100.000000 to 100.000000
version result total
a5a6362 ok 5937457
version total_tests
a5a6362 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
3113ae1 ok 5937457
version total_tests
3113ae1 5937457
correctness_percentage
100.0

@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, just a minor comment about one interface method


var _ sql.Statistic = (*DoltStats)(nil)

func (s *DoltStats) WithColSet(set sql.ColSet) sql.Statistic {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not crazy about all this boilerplate code to avoid an embedding but I guess it's slightly preferable to this breaking silently when you add a new With method to the interface type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the embedded version i started with did have confusing looking errors for missed Withs, so i switched to this one

return &DoltStats{mu: &sync.Mutex{}, Active: make(map[hash.Hash]int), Statistic: &stats.Statistic{}}
}

func (s *DoltStats) ToInterface() interface{} {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be called JsonMap or similar and potentially have the return type map[string]any

Copy link
Contributor Author

@max-hoffman max-hoffman Apr 2, 2024

Choose a reason for hiding this comment

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

i think it can return a list of interfaces also, []interface{}. not for this type specifically, but the json interface this implements

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

comparing_percentages
100.000000 to 100.000000
version result total
27ca944 ok 5937457
version total_tests
27ca944 5937457
correctness_percentage
100.0

@max-hoffman max-hoffman merged commit d6aa1e6 into main Apr 2, 2024
20 checks passed
@max-hoffman max-hoffman deleted the max/stats-struct-perf branch April 2, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants