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

perf(query): use quickselect instead of sorting while pagination #8995

Merged
merged 13 commits into from
Sep 14, 2023
Merged

perf(query): use quickselect instead of sorting while pagination #8995

merged 13 commits into from
Sep 14, 2023

Conversation

harshil-goel
Copy link
Contributor

@harshil-goel harshil-goel commented Sep 7, 2023 •
edited
Loading

When we get pagination request, we still sort the entire structure and then return relevant results. This diff introduces mini select algorithm that will only sort till we get first k elements out of n.

Both LDBC Benchmarks and MicroBenchmarks show that this is only useful until 50%

LDBC Queries Benchmarks
Total elements : 300000
Current main:
BenchmarkQueries/IC09/First_:20-8                      1        36621508916 ns/op
BenchmarkQueries/IC09/First_:304368-8                  1        50905281926 ns/op
BenchmarkQueries/IC09/First_:608716-8                  1        63878833326 ns/op
BenchmarkQueries/IC09/First_:913064-8                  1        77455114015 ns/op
BenchmarkQueries/IC09/First_:1217412-8                 1        94143638593 ns/op
BenchmarkQueries/IC09/First_:1521760-8                 1        111483390712 ns/op
BenchmarkQueries/IC09/First_:1826108-8                 1        123910498449 ns/op


QuickSelect:
BenchmarkQueries/IC09/First_:20-8                      1        32248068234 ns/op
BenchmarkQueries/IC09/First_:304368-8                  1        45877353021 ns/op
BenchmarkQueries/IC09/First_:608716-8                  1        60671818631 ns/op
BenchmarkQueries/IC09/First_:913064-8                  1        73447308743 ns/op
BenchmarkQueries/IC09/First_:1217412-8                 1        90371662972 ns/op
BenchmarkQueries/IC09/First_:1521760-8                 1        112880718231 ns/op
BenchmarkQueries/IC09/First_:1826108-8                 1        125487157700 ns/op

In microbenchmarks, first row is the current sort. Further rows are various different k values for the new algo

MicroBenchmarks
Normal Sort vs QuickSelect
BenchmarkSortQuickSort/Normal_Sort_Ratio_1000000_-8                    1        1652489894 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_1-8                1          77946322 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_166667-8                   1         327467804 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_333333-8                   1         613629295 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_499999-8                   1         899239726 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_666665-8                   1        1231191998 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_833331-8                   1        1565101591 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_999997-8                   1        1746166978 ns/op

@dgraph-bot dgraph-bot added area/core internal mechanisms go Pull requests that update Go code labels Sep 7, 2023
mangalaman93
mangalaman93 requested changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

  1. we don't need to compare start == 0
  2. what if end is close to len(ul.Uids)

mangalaman93
mangalaman93 requested changes Sep 10, 2023
View reviewed changes
types/sort.go Outdated Show resolved Hide resolved
types/sort.go Show resolved Hide resolved
types/sort.go Outdated Show resolved Hide resolved
worker/sort.go Show resolved Hide resolved
worker/sort.go Outdated Show resolved Hide resolved
mangalaman93
mangalaman93 requested changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

  1. move the quick select code to another files in the types package
  2. Fix the benchmarks
  3. Add a TODO in line 458 of worker/sort.go
  4. Update the description with the new benchmarks
  5. Add a reference to the place where we copied the quicksort code

@mangalaman93 mangalaman93 changed the title Update sorting function to improve performance perf(query): use quickselect instead of sorting while pagination Sep 14, 2023
mangalaman93
mangalaman93 requested changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

LG, minor comments

types/select.go Outdated Show resolved Hide resolved
types/select.go Show resolved Hide resolved
types/select.go Outdated Show resolved Hide resolved
types/sort.go Outdated Show resolved Hide resolved
types/sort.go Outdated Show resolved Hide resolved
types/sort_test.go Outdated Show resolved Hide resolved
types/sort_test.go Outdated Show resolved Hide resolved
types/sort_test.go Outdated Show resolved Hide resolved
types/sort_test.go Outdated Show resolved Hide resolved
worker/sort.go Outdated Show resolved Hide resolved
jairad26
jairad26 previously approved these changes Sep 14, 2023
View reviewed changes
Copy link

@jairad26 jairad26 left a comment

Choose a reason for hiding this comment

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

lgtm

jairad26
jairad26 approved these changes Sep 14, 2023
View reviewed changes
Copy link

@jairad26 jairad26 left a comment

Choose a reason for hiding this comment

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

lgtm

@mangalaman93 mangalaman93 merged commit 8d744e6 into main Sep 14, 2023
11 checks passed
@mangalaman93 mangalaman93 deleted the harshil-goel/sort-fix branch September 14, 2023 20:58
shivaji-dgraph pushed a commit that referenced this pull request Mar 12, 2024
@harshil-goel @mangalaman93
perf(query): use quickselect instead of sorting while pagination (#8995)
6357cb4 
When we get pagination request, we still sort the entire structure and
then return relevant results. This diff introduces mini select algorithm
that will only sort till we get first k elements out of n.

Both LDBC Benchmarks and MicroBenchmarks show that this is only useful
until 50%

```
LDBC Queries Benchmarks
Total elements : 300000
Current main:
BenchmarkQueries/IC09/First_:20-8                      1        36621508916 ns/op
BenchmarkQueries/IC09/First_:304368-8                  1        50905281926 ns/op
BenchmarkQueries/IC09/First_:608716-8                  1        63878833326 ns/op
BenchmarkQueries/IC09/First_:913064-8                  1        77455114015 ns/op
BenchmarkQueries/IC09/First_:1217412-8                 1        94143638593 ns/op
BenchmarkQueries/IC09/First_:1521760-8                 1        111483390712 ns/op
BenchmarkQueries/IC09/First_:1826108-8                 1        123910498449 ns/op
```

```
QuickSelect:
BenchmarkQueries/IC09/First_:20-8                      1        32248068234 ns/op
BenchmarkQueries/IC09/First_:304368-8                  1        45877353021 ns/op
BenchmarkQueries/IC09/First_:608716-8                  1        60671818631 ns/op
BenchmarkQueries/IC09/First_:913064-8                  1        73447308743 ns/op
BenchmarkQueries/IC09/First_:1217412-8                 1        90371662972 ns/op
BenchmarkQueries/IC09/First_:1521760-8                 1        112880718231 ns/op
BenchmarkQueries/IC09/First_:1826108-8                 1        125487157700 ns/op
```

In microbenchmarks, first row is the current sort. Further rows are
various different k values for the new algo

```
MicroBenchmarks
Normal Sort vs QuickSelect
BenchmarkSortQuickSort/Normal_Sort_Ratio_1000000_-8                    1        1652489894 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_1-8                1          77946322 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_166667-8                   1         327467804 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_333333-8                   1         613629295 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_499999-8                   1         899239726 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_666665-8                   1        1231191998 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_833331-8                   1        1565101591 ns/op
BenchmarkSortQuickSort/QuickSort_Sort_Ratio_1000000_999997-8                   1        1746166978 ns/op
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mangalaman93 mangalaman93 mangalaman93 approved these changes

@jairad26 jairad26 jairad26 approved these changes

@meghalims meghalims Awaiting requested review from meghalims

@sanjayk-github-dev sanjayk-github-dev Awaiting requested review from sanjayk-github-dev

@all-seeing-code all-seeing-code Awaiting requested review from all-seeing-code

@billprovince billprovince Awaiting requested review from billprovince

@joshua-goldstein joshua-goldstein Awaiting requested review from joshua-goldstein

Assignees
No one assigned
Labels
area/core internal mechanisms go Pull requests that update Go code
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harshil-goel @mangalaman93 @jairad26 @dgraph-bot