Skip to content

Minor cleanup to search in-memory index #8148

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

Closed
wants to merge 3 commits into from
Closed

Minor cleanup to search in-memory index #8148

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d2575d
Minor cleanup to search in-memory index
parlough Oct 14, 2024
534548f
Merge branch 'master' into misc/minor-memory-index-cleanup
parlough Oct 14, 2024
1267216
Merge branch 'master' into misc/minor-memory-index-cleanup
parlough Oct 16, 2024
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
85 changes: 41 additions & 44 deletions app/lib/search/mem_index.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:math' as math;

import 'package:_pub_shared/search/search_form.dart';
import 'package:clock/clock.dart';
import 'package:collection/collection.dart';
import 'package:logging/logging.dart';
import 'package:meta/meta.dart';

Expand All @@ -19,15 +20,15 @@ final _logger = Logger('search.mem_index');
final _textSearchTimeout = Duration(milliseconds: 500);

class InMemoryPackageIndex {
final Map<String, PackageDocument> _packages = <String, PackageDocument>{};
final _packageNameIndex = PackageNameIndex();
final Map<String, PackageDocument> _packages = {};
final PackageNameIndex _packageNameIndex = PackageNameIndex();
Copy link
Collaborator

@isoos isoos Oct 15, 2024

Choose a reason for hiding this comment

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

Not sure: why do we need to specify the type here?

final TokenIndex _descrIndex = TokenIndex();
final TokenIndex _readmeIndex = TokenIndex();
final TokenIndex _apiSymbolIndex = TokenIndex();

/// Adjusted score takes the overall score and transforms
/// it linearly into the [0.4-1.0] range.
final _adjustedOverallScores = <String, double>{};
final Map<String, double> _adjustedOverallScores = {};
Copy link
Collaborator

@isoos isoos Oct 15, 2024

Choose a reason for hiding this comment

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

Same here: type inference seems to be enough, why the extra specification?

Copy link
Member Author

@parlough parlough Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The inference is enough, but this change allows it to be consistent with the other nearby field entries.

Also, since these are fields and can be used outside of the direct vicinity, I generally think it makes sense to specify (and as a result, explicitly think about) the type.

Happy to revert these changes though :)

Copy link
Collaborator

@isoos isoos Oct 16, 2024

Choose a reason for hiding this comment

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

Oh, now I understand the intent here. The other nearby field entries are older code that preceded the inference capabilities in the language, so if we wanted to make them consistent, I'd rather move them towards using the more modern features. wdyt?

late final List<PackageHit> _overallOrderedHits;
late final List<PackageHit> _createdOrderedHits;
late final List<PackageHit> _updatedOrderedHits;
Expand All @@ -49,36 +50,34 @@ class InMemoryPackageIndex {
}
_updateOverallScores();
_lastUpdated = clock.now().toUtc();
_overallOrderedHits = _rankWithComparator(_compareOverall,
_overallOrderedHits = _rankWithComparator(_packages, _compareOverall,
Copy link
Collaborator

@isoos isoos Oct 15, 2024

Choose a reason for hiding this comment

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

Not sure: is there any benefit of extracting these methods outside of the class? It is not that we will reuse them in other places.

Copy link
Member Author

@parlough parlough Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was more of a transitionary change to a world where a lot of the mutable state doesn't have to escape construction. But that proved too large, so I thought this initial change would still be an improvement for now while getting a bit closer.

I do think making it static helps make it clear (and enforces) that it's not impacting any state of the class.

score: (doc) => doc.overallScore ?? 0.0);
_createdOrderedHits = _rankWithComparator(_compareCreated);
_updatedOrderedHits = _rankWithComparator(_compareUpdated);
_popularityOrderedHits = _rankWithComparator(_comparePopularity,
_createdOrderedHits = _rankWithComparator(_packages, _compareCreated);
_updatedOrderedHits = _rankWithComparator(_packages, _compareUpdated);
_popularityOrderedHits = _rankWithComparator(_packages, _comparePopularity,
score: (doc) => doc.popularityScore ?? 0);
_likesOrderedHits = _rankWithComparator(_compareLikes,
_likesOrderedHits = _rankWithComparator(_packages, _compareLikes,
score: (doc) => doc.likeCount.toDouble());
_pointsOrderedHits = _rankWithComparator(_comparePoints,
_pointsOrderedHits = _rankWithComparator(_packages, _comparePoints,
score: (doc) => doc.grantedPoints.toDouble());
}

IndexInfo indexInfo() {
return IndexInfo(
isReady: true,
packageCount: _packages.length,
lastUpdated: _lastUpdated,
);
}
IndexInfo indexInfo() => IndexInfo(
isReady: true,
packageCount: _packages.length,
lastUpdated: _lastUpdated,
);

void _addPackage(PackageDocument doc) {
_packages[doc.package] = doc;
_packageNameIndex.add(doc.package);
_descrIndex.add(doc.package, doc.description);
_readmeIndex.add(doc.package, doc.readme);

for (final ApiDocPage page in doc.apiDocPages ?? const []) {
for (final page in doc.apiDocPages ?? const <ApiDocPage>[]) {
final pageId = _apiDocPageId(doc.package, page);
if (page.symbols != null && page.symbols!.isNotEmpty) {
_apiSymbolIndex.add(pageId, page.symbols!.join(' '));
if (page.symbols case final pageSymbols? when pageSymbols.isNotEmpty) {
_apiSymbolIndex.add(pageId, pageSymbols.join(' '));
}
}
}
Expand Down Expand Up @@ -149,8 +148,8 @@ class InMemoryPackageIndex {

List<String>? nameMatches;
List<PackageHit> packageHits;
switch (query.effectiveOrder ?? SearchOrder.top) {
case SearchOrder.top:
switch (query.effectiveOrder) {
case null || SearchOrder.top:
if (textResults == null) {
packageHits = _overallOrderedHits.whereInSet(packages);
break;
Expand Down Expand Up @@ -346,59 +345,57 @@ class InMemoryPackageIndex {
return list;
}

List<PackageHit> _rankWithComparator(
static List<PackageHit> _rankWithComparator(
Map<String, PackageDocument> packages,
int Function(PackageDocument a, PackageDocument b) compare, {
double Function(PackageDocument doc)? score,
}) {
final list = _packages.values
.map((doc) => PackageHit(
package: doc.package, score: score == null ? null : score(doc)))
.toList();
list.sort((a, b) => compare(_packages[a.package]!, _packages[b.package]!));
return list;
}
}) =>
packages.entries
.sorted((entryA, entryB) => compare(entryA.value, entryB.value))
.map((e) => PackageHit(package: e.key, score: score?.call(e.value)))
.toList(growable: false);

int _compareCreated(PackageDocument a, PackageDocument b) {
static int _compareCreated(PackageDocument a, PackageDocument b) {
return -a.created.compareTo(b.created);
}

int _compareUpdated(PackageDocument a, PackageDocument b) {
static int _compareUpdated(PackageDocument a, PackageDocument b) {
return -a.updated.compareTo(b.updated);
}

int _compareOverall(PackageDocument a, PackageDocument b) {
static int _compareOverall(PackageDocument a, PackageDocument b) {
final x = -(a.overallScore ?? 0.0).compareTo(b.overallScore ?? 0.0);
if (x != 0) return x;
return _compareUpdated(a, b);
}

int _comparePopularity(PackageDocument a, PackageDocument b) {
static int _comparePopularity(PackageDocument a, PackageDocument b) {
final x = -(a.popularityScore ?? 0.0).compareTo(b.popularityScore ?? 0.0);
if (x != 0) return x;
return _compareUpdated(a, b);
}

int _compareLikes(PackageDocument a, PackageDocument b) {
static int _compareLikes(PackageDocument a, PackageDocument b) {
final x = -a.likeCount.compareTo(b.likeCount);
if (x != 0) return x;
return _compareUpdated(a, b);
}

int _comparePoints(PackageDocument a, PackageDocument b) {
static int _comparePoints(PackageDocument a, PackageDocument b) {
final x = -a.grantedPoints.compareTo(b.grantedPoints);
if (x != 0) return x;
return _compareUpdated(a, b);
}

String _apiDocPageId(String package, ApiDocPage page) {
static String _apiDocPageId(String package, ApiDocPage page) {
return '$package::${page.relativePath}';
}

String _apiDocPkg(String id) {
static String _apiDocPkg(String id) {
return id.split('::').first;
}

String _apiDocPath(String id) {
static String _apiDocPath(String id) {
return id.split('::').last;
}
}
Expand All @@ -415,11 +412,6 @@ class _TextResults {
class PackageNameIndex {
final _data = <String, _PkgNameData>{};

/// Maps package name to a reduced form of the name:
/// the same character parts, but without `-`.
String _collapseName(String package) =>
package.replaceAll('_', '').toLowerCase();

void addAll(Iterable<String> packages) {
for (final package in packages) {
add(package);
Expand Down Expand Up @@ -470,6 +462,11 @@ class PackageNameIndex {
}
return Score(values).removeLowValues(fraction: 0.5, minValue: 0.5);
}

/// Maps package name to a reduced form of the name:
/// the same character parts, but without `-`.
static String _collapseName(String package) =>
package.replaceAll('_', '').toLowerCase();
}

class _PkgNameData {
Expand Down
Loading