Skip to content

Use top-k sorted list builder instead of full-list sorts in search indexes. #8814

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use top-k sorted list builder instead of full-list sorts in search indexes. #8814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 27 additions & 21 deletions app/lib/search/mem_index.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import 'package:meta/meta.dart';
import 'package:pub_dev/service/topics/models.dart';
import 'package:pub_dev/third_party/bit_array/bit_array.dart';

import '../shared/utils.dart' show TopKSortedListBuilder;
import 'models.dart';
import 'search_service.dart';
import 'text_utils.dart';
Expand Down Expand Up @@ -263,6 +264,13 @@ class InMemoryPackageIndex {
// extra item, that will be addressed after the ranking score is determined.
var totalCount = packageScores?.positiveCount() ?? predicateFilterCount;

// Checking if it is worth to calculate the sorted order, estimating the
// total count by overcounting the best name matches.
final maximumTotalCount = totalCount + (bestNameIndex != null ? 1 : 0);
if (maximumTotalCount < query.offset) {
return PackageSearchResult.empty();
}

Iterable<IndexedPackageHit> indexedHits;
switch (query.effectiveOrder) {
case SearchOrder.top:
Expand All @@ -285,8 +293,8 @@ class InMemoryPackageIndex {
}
indexedHits = _rankWithValues(
packageScores,
requiredLengthThreshold: query.offset,
bestNameIndex: bestNameIndex ?? -1,
topK: query.offset + query.limit,
);
break;
case SearchOrder.created:
Expand Down Expand Up @@ -512,33 +520,31 @@ class InMemoryPackageIndex {
return _TextResults(topApiPages);
}

List<IndexedPackageHit> _rankWithValues(
Iterable<IndexedPackageHit> _rankWithValues(
IndexedScore<String> score, {
// if the item count is fewer than this threshold, an empty list will be returned
required int requiredLengthThreshold,
// When no best name match is applied, this parameter will be `-1`
/// When no best name match is applied, this parameter will be `-1`
required int bestNameIndex,

/// Return (and sort) only the top-k results.
required int topK,
}) {
final list = <IndexedPackageHit>[];
final builder = TopKSortedListBuilder<int>(topK, (aIndex, bIndex) {
if (aIndex == bestNameIndex) return -1;
if (bIndex == bestNameIndex) return 1;
final aScore = score.getValue(aIndex);
final bScore = score.getValue(bIndex);
final scoreCompare = -aScore.compareTo(bScore);
if (scoreCompare != 0) return scoreCompare;
// if two packages got the same score, order by last updated
return _compareUpdated(_documents[aIndex], _documents[bIndex]);
});
for (var i = 0; i < score.length; i++) {
final value = score.getValue(i);
if (value <= 0.0 && i != bestNameIndex) continue;
list.add(IndexedPackageHit(
i, PackageHit(package: score.keys[i], score: value)));
}
if (requiredLengthThreshold > list.length) {
// There is no point to sort or even keep the results, as the search query offset ignores these anyway.
return [];
builder.add(i);
}
list.sort((a, b) {
if (a.index == bestNameIndex) return -1;
if (b.index == bestNameIndex) return 1;
final scoreCompare = -a.hit.score!.compareTo(b.hit.score!);
if (scoreCompare != 0) return scoreCompare;
// if two packages got the same score, order by last updated
return _compareUpdated(_documents[a.index], _documents[b.index]);
});
return list;
return builder.getTopK().map((i) => IndexedPackageHit(
i, PackageHit(package: score.keys[i], score: score.getValue(i))));
}

List<IndexedPackageHit> _rankWithComparator(
Expand Down
20 changes: 8 additions & 12 deletions app/lib/search/token_index.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'dart:math' as math;

import 'package:meta/meta.dart';
import 'package:pub_dev/shared/utils.dart';
import 'package:pub_dev/third_party/bit_array/bit_array.dart';

import 'text_utils.dart';
Expand Down Expand Up @@ -313,21 +314,16 @@ class IndexedScore<K> {
}

Map<K, double> top(int count, {double? minValue}) {
final list = <int>[];
double? lastValue;
minValue ??= 0.0;
final builder = TopKSortedListBuilder<int>(
count, (a, b) => -_values[a].compareTo(_values[b]));
for (var i = 0; i < length; i++) {
final v = _values[i];
if (minValue != null && v < minValue) continue;
if (list.length == count) {
if (lastValue != null && lastValue >= v) continue;
list[count - 1] = i;
} else {
list.add(i);
}
list.sort((a, b) => -_values[a].compareTo(_values[b]));
lastValue = _values[list.last];
if (v < minValue) continue;
builder.add(i);
}
return Map.fromEntries(list.map((i) => MapEntry(_keys[i], _values[i])));
return Map.fromEntries(
builder.getTopK().map((i) => MapEntry(_keys[i], _values[i])));
}

Map<K, double> toMap() {
Expand Down
44 changes: 44 additions & 0 deletions app/lib/shared/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,50 @@ class DurationTracker extends LastNTracker<Duration> {
};
}

/// Builds a sorted list of the top-k items using the provided comparator.
///
/// The algorithm uses a binary tree insertion, resulting in O(N * log(K)) comparison.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a binary tree insertion, but insertion in a sorted list, complexity O(n*k) (the insertion step is linear in k)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good point, I was ignoring the List insertion step, as in the comparison was dominating the sorting times much more. However, the reduced gain on high-k values may be due to the List insertion. I was certainly lazy in the insertion part, will take a look how to improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use an actual binary tree (though that comes with its own overhead)

If k is small enough, simple list insertion might be better

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if we stick with the list, consider using https://pub.dev/documentation/collection/latest/collection/binarySearch.html . Binary search is known as being notoriously hard to implement correctly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case the implementation is very close to the one in package:collection, but I will certainly benchmark it, and if it about the same, we shall use it:
https://github.com/dart-lang/core/blob/main/pkgs/collection/lib/src/algorithms.dart#L47-L62

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

binarySearch in this case is not usable, as it returns -1 when the searched item is not in the collection, while this algorithm needs the index where the next item should be inserted into. Furthermore, in most of the cases there is no equality, it won't be in the collection. However, with close review, I don't think the algorithm is conceptually the same, except for this specific goal.

I've checked List.insert + List.removeLast() (if the list is over k) and overall did perform worse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we know both k and N upfront, we could probably decide which sorting is more worthwhile: this top-k list building for low k or a full list + single sort upfront otherwise. (We could also limit k, e.g. the number of text search results to 1000 or some reasonably high number).

class TopKSortedListBuilder<T> {
final int _k;
final Comparator<T> _compare;
final _list = <T>[];

TopKSortedListBuilder(this._k, this._compare);

void addAll(Iterable<T> items) {
for (final item in items) {
add(item);
}
}

void add(T item) {
if (_list.length >= _k && _compare(_list.last, item) <= 0) {
return;
}
var start = 0, end = _list.length;
while (start < end) {
final mid = (start + end) >> 1;
if (_compare(_list[mid], item) <= 0) {
start = mid + 1;
} else {
end = mid;
}
}
if (_list.length < _k) {
_list.insert(start, item);
return;
}
for (var i = _list.length - 1; i > start; i--) {
_list[i] = _list[i - 1];
}
_list[start] = item;
}

Iterable<T> getTopK() {
return _list;
}
}

/// Returns the MIME content type based on the name of the file.
String contentType(String name) {
final ext = p.extension(name).replaceAll('.', '');
Expand Down
33 changes: 33 additions & 0 deletions app/test/shared/utils_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,37 @@ void main() {
expect(tracker.getLatency().inMilliseconds, greaterThan(15000));
});
});

group('top-k sorted list', () {
int compare(int a, int b) => -a.compareTo(b);

test('no items', () {
final builder = TopKSortedListBuilder(5, compare);
expect(builder.getTopK().toList(), []);
});

test('single item', () {
final builder = TopKSortedListBuilder(5, compare);
builder.add(1);
expect(builder.getTopK().toList(), [1]);
});

test('three items ascending', () {
final builder = TopKSortedListBuilder(5, compare);
builder.addAll([1, 2, 3]);
expect(builder.getTopK().toList(), [3, 2, 1]);
});

test('three items descending', () {
final builder = TopKSortedListBuilder(5, compare);
builder.addAll([3, 2, 1]);
expect(builder.getTopK().toList(), [3, 2, 1]);
});

test('10 items + repeated', () {
final builder = TopKSortedListBuilder(5, compare);
builder.addAll([1, 10, 2, 9, 3, 8, 4, 7, 6, 5, 9]);
expect(builder.getTopK().toList(), [10, 9, 9, 8, 7]);
});
});
}