Package Kit: Refactor BRepositoryCache to have GetPackageInfos(callback)...

... instead of GetIterator(). This allows us to avoid loading the
cache entirely in SetTo(), and allows consumers to read the cache
much more efficiently than loading it all into memory and then
freeing it again afterwards.

This technically breaks API/ABI, however the Package Kit APIs
are not considered stable (I don't think.)

All consumers adjusted. As we build the host tools with modern GCC
only, I made update_package_requires use a C++11 lambda function.
All others use out-of-line static methods.

"time pkgman list-repos -v" (which has to read the whole cache)
is ~0.5s before this change, and ~0.25s after this change,
on my test VM.

Change-Id: I6976b4cf5eb846fc925ed199dc00eb227fc81344
Reviewed-on: https://review.haiku-os.org/c/haiku/+/10247
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
Reviewed-by: Andrew Lindesay <[email protected]>

headers/os/package/RepositoryCache.h

@@ -18,7 +18,7 @@ namespace BPackageKit {
 
 class BRepositoryCache {
 public:
-			typedef BPackageInfoSet::Iterator Iterator;
+			typedef bool (*GetPackageInfosCallback)(void* /* context */, const BPackageInfo& info);
 
 public:
 								BRepositoryCache();
@@ -30,20 +30,19 @@ public:
 			const BEntry&		Entry() const;
 			bool				IsUserSpecific() const;
 
-			void				SetIsUserSpecific(bool isUserSpecific);
+			status_t			GetPackageInfos(GetPackageInfosCallback callback, void* context) const;
-
-			uint32				CountPackages() const;
-			Iterator			GetIterator() const;
 
 private:
 			struct RepositoryContentHandler;
 
+			status_t			_ReadCache(const BPath& repositoryCachePath,
+									BRepositoryInfo& repositoryInfo,
+									GetPackageInfosCallback callback, void* context) const;
+
 private:
 			BEntry				fEntry;
 			BRepositoryInfo		fInfo;
 			bool				fIsUserSpecific;
-
-			BPackageInfoSet		fPackages;
 };
 
 

src/bin/pkgman/command_list_repos.cpp

@@ -46,6 +46,14 @@ DEFINE_COMMAND(ListReposCommand, "list-repos", kShortUsage, kLongUsage,
 	COMMAND_CATEGORY_REPOSITORIES)
 
 
+static bool
+CountPackagesCallback(void* context, const BPackageInfo&)
+{
+	*((int32*)context) += 1;
+	return true;
+}
+
+
 int
 ListReposCommand::Execute(int argc, const char* const* argv)
 {
@@ -111,6 +119,9 @@ ListReposCommand::Execute(int argc, const char* const* argv)
 			BRepositoryCache repoCache;
 			result = roster.GetRepositoryCache(repoName, &repoCache);
 			if (result == B_OK) {
+				int32 count = 0;
+				repoCache.GetPackageInfos(CountPackagesCallback, &count);
+
 				printf("\t\tvendor:    %s\n",
 					repoCache.Info().Vendor().String());
 				printf("\t\tsummary:   %s\n",
@@ -118,7 +129,7 @@ ListReposCommand::Execute(int argc, const char* const* argv)
 				printf("\t\tarch:      %s\n", BPackageInfo::kArchitectureNames[
 						repoCache.Info().Architecture()]);
 				printf("\t\tpkg-count: %" B_PRIu32 "\n",
-					repoCache.CountPackages());
+					count);
 				printf("\t\tbase-url:  %s\n",
 					repoCache.Info().BaseURL().String());
 				printf("\t\tidentifier:  %s\n",

src/kits/package/PackageRoster.cpp

@@ -264,7 +264,7 @@ BPackageRoster::IsPackageActive(BPackageInstallationLocation location,
 	if (error != B_OK)
 		return error;
 
-	BRepositoryCache::Iterator it = packageInfos.GetIterator();
+	BPackageInfoSet::Iterator it = packageInfos.GetIterator();
 	while (const BPackageInfo* packageInfo = it.Next()) {
 		if (info.Name() == packageInfo->Name() &&
 			info.Version().Compare(packageInfo->Version()) == 0) {

src/kits/package/RepositoryCache.cpp

@@ -40,12 +40,13 @@ using namespace BHPKG;
 
 
 struct BRepositoryCache::RepositoryContentHandler : BRepositoryContentHandler {
-	RepositoryContentHandler(BRepositoryInfo& repositoryInfo,
+	RepositoryContentHandler(BErrorOutput* errorOutput, BRepositoryInfo& repositoryInfo,
-		BPackageInfoSet& packages, BErrorOutput* errorOutput)
+		GetPackageInfosCallback callback, void* callbackContext)
 		:
 		fRepositoryInfo(repositoryInfo),
 		fPackageInfo(),
-		fPackages(packages),
+		fCallback(callback),
+		fCallbackContext(callbackContext),
 		fPackageInfoContentHandler(fPackageInfo, errorOutput)
 	{
 	}
@@ -56,7 +57,6 @@ struct BRepositoryCache::RepositoryContentHandler : BRepositoryContentHandler {
 		return B_OK;
 	}
 
-
 	virtual status_t HandlePackageAttribute(
 		const BPackageInfoAttributeValue& value)
 	{
@@ -69,9 +69,11 @@ struct BRepositoryCache::RepositoryContentHandler : BRepositoryContentHandler {
 		if (result != B_OK)
 			return result;
 
-		result = fPackages.AddInfo(fPackageInfo);
+		if (fCallback == NULL)
-		if (result != B_OK)
+			return B_CANCELED;
-			return result;
+
+		if (!fCallback(fCallbackContext, fPackageInfo))
+			return B_CANCELED;
 
 		return B_OK;
 	}
@@ -79,7 +81,6 @@ struct BRepositoryCache::RepositoryContentHandler : BRepositoryContentHandler {
 	virtual status_t HandleRepositoryInfo(const BRepositoryInfo& repositoryInfo)
 	{
 		fRepositoryInfo = repositoryInfo;
-
 		return B_OK;
 	}
 
@@ -90,7 +91,9 @@ struct BRepositoryCache::RepositoryContentHandler : BRepositoryContentHandler {
 private:
 	BRepositoryInfo&			fRepositoryInfo;
 	BPackageInfo				fPackageInfo;
-	BPackageInfoSet&			fPackages;
+	GetPackageInfosCallback		fCallback;
+	void*						fCallbackContext;
+
 	BPackageInfoContentHandler	fPackageInfoContentHandler;
 };
 
@@ -100,8 +103,7 @@ private:
 
 BRepositoryCache::BRepositoryCache()
 	:
-	fIsUserSpecific(false),
+	fIsUserSpecific(false)
-	fPackages()
 {
 }
 
@@ -132,21 +134,9 @@ BRepositoryCache::IsUserSpecific() const
 }
 
 
-void
-BRepositoryCache::SetIsUserSpecific(bool isUserSpecific)
-{
-	fIsUserSpecific = isUserSpecific;
-}
-
-
 status_t
 BRepositoryCache::SetTo(const BEntry& entry)
 {
-	// unset
-	fPackages.MakeEmpty();
-	fEntry.Unset();
-
-	// get cache file path
 	fEntry = entry;
 
 	BPath repositoryCachePath;
@@ -154,14 +144,8 @@ BRepositoryCache::SetTo(const BEntry& entry)
 	if ((result = entry.GetPath(&repositoryCachePath)) != B_OK)
 		return result;
 
-	// read repository cache
+	// read only info from repository cache
-	BStandardErrorOutput errorOutput;
+	if ((result = _ReadCache(repositoryCachePath, fInfo, NULL, NULL) != B_OK))
-	BRepositoryReader repositoryReader(&errorOutput);
-	if ((result = repositoryReader.Init(repositoryCachePath.Path())) != B_OK)
-		return result;
-
-	RepositoryContentHandler handler(fInfo, fPackages, &errorOutput);
-	if ((result = repositoryReader.ParseContent(&handler)) != B_OK)
 		return result;
 
 	BPath userSettingsPath;
@@ -175,17 +159,35 @@ BRepositoryCache::SetTo(const BEntry& entry)
 }
 
 
-uint32
+status_t
-BRepositoryCache::CountPackages() const
+BRepositoryCache::GetPackageInfos(GetPackageInfosCallback callback, void* context) const
 {
-	return fPackages.CountInfos();
+	BPath repositoryCachePath;
+	status_t result;
+	if ((result = fEntry.GetPath(&repositoryCachePath)) != B_OK)
+		return result;
+
+	BRepositoryInfo dummy;
+	return _ReadCache(repositoryCachePath, dummy, callback, context);
 }
 
 
-BRepositoryCache::Iterator
+status_t
-BRepositoryCache::GetIterator() const
+BRepositoryCache::_ReadCache(const BPath& repositoryCachePath,
+	BRepositoryInfo& repositoryInfo, GetPackageInfosCallback callback, void* context) const
 {
-	return fPackages.GetIterator();
+	status_t result;
+
+	BStandardErrorOutput errorOutput;
+	BRepositoryReader repositoryReader(&errorOutput);
+	if ((result = repositoryReader.Init(repositoryCachePath.Path())) != B_OK)
+		return result;
+
+	RepositoryContentHandler handler(&errorOutput, repositoryInfo, callback, context);
+	if ((result = repositoryReader.ParseContent(&handler)) != B_OK && result != B_CANCELED)
+		return result;
+
+	return B_OK;
 }
 
 

src/kits/package/solver/SolverRepository.cpp

@@ -131,6 +131,20 @@ BSolverRepository::SetTo(BAllInstallationLocations)
 }
 
 
+static bool
+SolverRepositoryAddPackageCallback(void* context, const BPackageInfo& packageInfo)
+{
+	BSolverRepository* repo = (BSolverRepository*)context;
+
+	status_t error = repo->AddPackage(packageInfo);
+	if (error != B_OK) {
+		repo->Unset();
+		return false;
+	}
+	return true;
+}
+
+
 status_t
 BSolverRepository::SetTo(const BRepositoryConfig& config)
 {
@@ -150,14 +164,8 @@ BSolverRepository::SetTo(const BRepositoryConfig& config)
 		return error;
 	}
 
-	BRepositoryCache::Iterator it = cache.GetIterator();
+	if ((error = cache.GetPackageInfos(SolverRepositoryAddPackageCallback, this)) != B_OK)
-	while (const BPackageInfo* packageInfo = it.Next()) {
-		error = AddPackage(*packageInfo);
-		if (error != B_OK) {
-			Unset();
 		return error;
-		}
-	}
 
 	return B_OK;
 }
@@ -175,14 +183,9 @@ BSolverRepository::SetTo(const BRepositoryCache& cache)
 	fName = info.Name();
 	fPriority = info.Priority();
 
-	BRepositoryCache::Iterator it = cache.GetIterator();
+	status_t error;
-	while (const BPackageInfo* packageInfo = it.Next()) {
+	if ((error = cache.GetPackageInfos(SolverRepositoryAddPackageCallback, this)) != B_OK)
-		status_t error = AddPackage(*packageInfo);
-		if (error != B_OK) {
-			Unset();
 		return error;
-		}
-	}
 
 	return B_OK;
 }
@@ -292,7 +295,7 @@ BSolverRepository::AddPackages(BPackageInstallationLocation location)
 	if (error != B_OK)
 		return error;
 
-	BRepositoryCache::Iterator it = packageInfos.GetIterator();
+	BPackageInfoSet::Iterator it = packageInfos.GetIterator();
 	while (const BPackageInfo* packageInfo = it.Next()) {
 		error = AddPackage(*packageInfo);
 		if (error != B_OK)

src/libs/libsolv/ext/repo_haiku.cpp

@@ -164,6 +164,7 @@ static Id add_package_info_to_repo(Repo *repo, Repodata *repoData,
   return solvableId;
 }
 
+#if 0
 static void add_installed_packages(Repo *repo, Repodata *repoData,
   BPackageInstallationLocation location)
 {
@@ -191,6 +192,7 @@ int repo_add_haiku_installed_packages(Repo *repo, const char *rootdir,
 
   return 0;
 }
+#endif
 
 Id repo_add_haiku_package(Repo *repo, const char *hpkgPath, int flags)
 {
@@ -201,6 +203,7 @@ Id repo_add_haiku_package(Repo *repo, const char *hpkgPath, int flags)
   return repo_add_haiku_package_info(repo, packageInfo, flags);
 }
 
+#if 0
 int repo_add_haiku_packages(Repo *repo, const char *repoName, int flags)
 {
   BPackageRoster roster;
@@ -219,6 +222,7 @@ int repo_add_haiku_packages(Repo *repo, const char *repoName, int flags)
 
   return 0;
 }
+#endif
 
 Id repo_add_haiku_package_info(Repo *repo,
   const BPackageKit::BPackageInfo &packageInfo, int flags)

src/tools/update_package_requires/update_package_requires.cpp

@@ -21,7 +21,7 @@
 using namespace BPackageKit;
 
 
-typedef std::list<BPackageResolvable*> ProvidesList;
+typedef std::list<BPackageResolvable> ProvidesList;
 
 
 static const char* sProgramName = "update_package_requires";
@@ -53,10 +53,10 @@ update_requires_expression(BPackageResolvableExpression& expression,
 	const ProvidesList& providesList)
 {
 	// find the best-matching provides
-	BPackageResolvable* bestProvides = NULL;
+	const BPackageResolvable* bestProvides = NULL;
 	for (ProvidesList::const_iterator it = providesList.begin();
 		it != providesList.end(); ++it) {
-		BPackageResolvable* provides = *it;
+		const BPackageResolvable* provides = &*it;
 		if (!expression.Matches(*provides))
 			continue;
 
@@ -116,16 +116,17 @@ main(int argc, const char* const* argv)
 
 	ProvidesMap providesMap;
 
-	for (BRepositoryCache::Iterator it = repositoryCache.GetIterator();
+	repositoryCache.GetPackageInfos([](void* context, const BPackageInfo& info) -> bool {
-		const BPackageInfo* info = it.Next();) {
+		ProvidesMap* providesMap = (ProvidesMap*)context;
-		const BObjectList<BPackageResolvable, true>& provides = info->ProvidesList();
+		const BObjectList<BPackageResolvable, true>& provides = info.ProvidesList();
 		int32 count = provides.CountItems();
 		for (int32 i = 0; i < count; i++) {
 			BPackageResolvable* resolvable = provides.ItemAt(i);
-			ProvidesList& providesList = providesMap[resolvable->Name()];
+			ProvidesList& providesList = (*providesMap)[resolvable->Name()];
-			providesList.push_back(resolvable);
+			providesList.push_back(*resolvable);
-		}
 		}
+		return true;
+	}, &providesMap);
 
 	// load the package info
 	BPackageInfo packageInfo;