From c419919252f9076761900fd4e7575407a554255e Mon Sep 17 00:00:00 2001 From: Andrew Lindesay Date: Tue, 19 May 2020 22:53:10 +1200 Subject: [PATCH] HaikuDepot: Performance This change removes two mistakes I made a long time ago that caused unnecessarily copying of lists of data. This fix speeds up the UI alot. This change also clears data in UI list elements when a bulk load is requested. It stops clearing otherwise and instead uses "add" and "remove" operations in the lists which is OK now because the UI list elements are much faster than they have been in the past. This removes the strange clean-and-reload that was visible in the UI previously. A threaded package loading system was put in place a long time ago, but with these performance improvements this mechanism is no longer necessary; it has been removed to simplify the code. Fixes #16012 Change-Id: I393cee929695726539602b51630ae285fb8384f1 Reviewed-on: https://review.haiku-os.org/c/haiku/+/2748 Reviewed-by: Adrien Destugues --- src/apps/haikudepot/HaikuDepotConstants.h | 2 - src/apps/haikudepot/model/Model.cpp | 33 +- src/apps/haikudepot/model/Model.h | 4 +- .../server/ServerIconExportUpdateProcess.cpp | 2 +- .../server/ServerPkgDataUpdateProcess.cpp | 4 +- .../haikudepot/ui/FeaturedPackagesView.cpp | 15 +- src/apps/haikudepot/ui/MainWindow.cpp | 283 ++++-------------- src/apps/haikudepot/ui/MainWindow.h | 12 +- 8 files changed, 92 insertions(+), 263 deletions(-) diff --git a/src/apps/haikudepot/HaikuDepotConstants.h b/src/apps/haikudepot/HaikuDepotConstants.h index 4b5be6139f..bb72497766 100644 --- a/src/apps/haikudepot/HaikuDepotConstants.h +++ b/src/apps/haikudepot/HaikuDepotConstants.h @@ -11,8 +11,6 @@ enum { MSG_PACKAGE_SELECTED = 'pkgs', MSG_PACKAGE_WORKER_BUSY = 'pkwb', MSG_PACKAGE_WORKER_IDLE = 'pkwi', - MSG_ADD_VISIBLE_PACKAGES = 'avpk', - MSG_UPDATE_SELECTED_PACKAGE = 'uspk', MSG_CLIENT_TOO_OLD = 'oldc', MSG_NETWORK_TRANSPORT_ERROR = 'nett', MSG_SERVER_ERROR = 'svre', diff --git a/src/apps/haikudepot/model/Model.cpp b/src/apps/haikudepot/model/Model.cpp index e2352ebc76..de427d082a 100644 --- a/src/apps/haikudepot/model/Model.cpp +++ b/src/apps/haikudepot/model/Model.cpp @@ -353,29 +353,19 @@ Model::AddListener(const ModelListenerRef& listener) } -PackageList -Model::CreatePackageList() const +// TODO; part of a wider change; cope with the package being in more than one +// depot +PackageInfoRef +Model::PackageForName(const BString& name) { - // Iterate all packages from all depots. - // If configured, restrict depot, filter by search terms, status, name ... - PackageList resultList; - - for (int32 i = 0; i < fDepots.CountItems(); i++) { - const DepotInfo& depot = fDepots.ItemAtFast(i); - - if (fDepotFilter.Length() > 0 && fDepotFilter != depot.Name()) - continue; - - const PackageList& packages = depot.Packages(); - - for (int32 j = 0; j < packages.CountItems(); j++) { - const PackageInfoRef& package = packages.ItemAtFast(j); - if (MatchesFilter(package)) - resultList.Add(package); - } + DepotList depots = Depots(); + for (int32 d = 0; d < depots.CountItems(); d++) { + const DepotInfo& depot = depots.ItemAtFast(d); + int32 packageIndex = depot.PackageIndexByName(name); + if (packageIndex >= 0) + return depot.Packages().ItemAtFast(packageIndex); } - - return resultList; + return PackageInfoRef(); } @@ -384,6 +374,7 @@ Model::MatchesFilter(const PackageInfoRef& package) const { return fCategoryFilter->AcceptsPackage(package) && fSearchTermsFilter->AcceptsPackage(package) + && (fDepotFilter.IsEmpty() || fDepotFilter == package->DepotName()) && (fShowAvailablePackages || package->State() != NONE) && (fShowInstalledPackages || package->State() != ACTIVATED) && (fShowSourcePackages || !is_source_package(package)) diff --git a/src/apps/haikudepot/model/Model.h b/src/apps/haikudepot/model/Model.h index 7fd161cce2..1aa57cca83 100644 --- a/src/apps/haikudepot/model/Model.h +++ b/src/apps/haikudepot/model/Model.h @@ -77,9 +77,7 @@ public: bool AddListener(const ModelListenerRef& listener); - // !Returns new PackageInfoList from current parameters - PackageList CreatePackageList() const; - + PackageInfoRef PackageForName(const BString& name); bool MatchesFilter( const PackageInfoRef& package) const; diff --git a/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp b/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp index a8d14bae05..d1d59d1e6a 100644 --- a/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp +++ b/src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp @@ -147,7 +147,7 @@ ServerIconExportUpdateProcess::PopulateForDepot(const DepotInfo& depot) printf("[%s] will populate icons for depot [%s]\n", Name(), depot.Name().String()); status_t result = B_OK; - PackageList packages = depot.Packages(); + const PackageList& packages = depot.Packages(); for(int32 j = 0; (j < packages.CountItems()) && !WasStopped() && (result == B_OK); j++) { diff --git a/src/apps/haikudepot/server/ServerPkgDataUpdateProcess.cpp b/src/apps/haikudepot/server/ServerPkgDataUpdateProcess.cpp index 6e4695a998..fc6cc9c11c 100644 --- a/src/apps/haikudepot/server/ServerPkgDataUpdateProcess.cpp +++ b/src/apps/haikudepot/server/ServerPkgDataUpdateProcess.cpp @@ -200,11 +200,11 @@ PackageFillingPkgListener::Handle(DumpExportPkg* pkg) const DepotInfo* depotInfo = fModel->DepotForName(fDepotName); if (depotInfo != NULL) { - BString packageName = *(pkg->Name()); + const BString packageName = *(pkg->Name()); int32 packageIndex = depotInfo->PackageIndexByName(packageName); if (-1 != packageIndex) { - PackageList packages = depotInfo->Packages(); + const PackageList& packages = depotInfo->Packages(); const PackageInfoRef& packageInfoRef = packages.ItemAtFast(packageIndex); diff --git a/src/apps/haikudepot/ui/FeaturedPackagesView.cpp b/src/apps/haikudepot/ui/FeaturedPackagesView.cpp index 10427ccc54..1dd9b0f042 100644 --- a/src/apps/haikudepot/ui/FeaturedPackagesView.cpp +++ b/src/apps/haikudepot/ui/FeaturedPackagesView.cpp @@ -199,6 +199,10 @@ public: void Clear() { + for (std::vector::iterator it = fPackages.begin(); + it != fPackages.end(); it++) { + (*it)->RemoveListener(fPackageListener); + } fPackages.clear(); fSelectedIndex = -1; Invalidate(); @@ -254,6 +258,7 @@ public: fPackages.insert(itInsertionPt, package); Invalidate(_RectOfIndex(insertionIndex) | _RectOfIndex(fPackages.size() - 1)); + package->AddListener(fPackageListener); } } @@ -266,6 +271,7 @@ public: fSelectedIndex = -1; if (fSelectedIndex > index) fSelectedIndex--; + fPackages[index]->RemoveListener(fPackageListener); fPackages.erase(fPackages.begin() + index); if (fPackages.empty()) Invalidate(); @@ -302,9 +308,12 @@ public: int32 _IndexOfPackage(PackageInfoRef package) const { - if (package.Get() != NULL) - return _IndexOfName(package->Name()); - return -1; + std::vector::const_iterator it + = std::lower_bound(fPackages.begin(), fPackages.end(), package, + &_IsPackageBefore); + + return (it == fPackages.end() || (*it)->Name() != package->Name()) + ? -1 : it - fPackages.begin(); } diff --git a/src/apps/haikudepot/ui/MainWindow.cpp b/src/apps/haikudepot/ui/MainWindow.cpp index d9e4890e06..28764a14df 100644 --- a/src/apps/haikudepot/ui/MainWindow.cpp +++ b/src/apps/haikudepot/ui/MainWindow.cpp @@ -205,8 +205,6 @@ MainWindow::MainWindow(const BMessage& settings) _UpdateAuthorization(); _RestoreWindowFrame(settings); - atomic_set(&fPackagesToShowListID, 0); - // start worker threads BPackageRoster().StartWatching(this, B_WATCH_PACKAGE_INSTALLATION_LOCATIONS); @@ -271,15 +269,6 @@ MainWindow::~MainWindow() if (fPendingActionsWorker >= 0) wait_for_thread(fPendingActionsWorker, NULL); - delete_sem(fPackageToPopulateSem); - if (fPopulatePackageWorker >= 0) - wait_for_thread(fPopulatePackageWorker, NULL); - - delete_sem(fNewPackagesToShowSem); - delete_sem(fShowPackagesAcknowledgeSem); - if (fShowPackagesWorker >= 0) - wait_for_thread(fShowPackagesWorker, NULL); - if (fScreenshotWindow != NULL && fScreenshotWindow->Lock()) fScreenshotWindow->Quit(); } @@ -436,17 +425,15 @@ MainWindow::MessageReceived(BMessage* message) { BString name; if (message->FindString("name", &name) == B_OK) { - BAutolock locker(fModel.Lock()); - int count = fVisiblePackages.CountItems(); - for (int i = 0; i < count; i++) { - const PackageInfoRef& package - = fVisiblePackages.ItemAtFast(i); - if (package.Get() != NULL && package->Name() == name) { - locker.Unlock(); - _AdoptPackage(package); - break; - } + PackageInfoRef package; + { + BAutolock locker(fModel.Lock()); + package = fModel.PackageForName(name); } + if (package.Get() == NULL) + debugger("unable to find the named package"); + else + _AdoptPackage(package); } else { _ClearPackage(); } @@ -506,43 +493,7 @@ MainWindow::MessageReceived(BMessage* message) BAutolock locker(fModel.Lock()); fModel.SetPackageState(ref, ref->State()); } - - // Asynchronous updates to the package information - // can mean that the package needs to be added or - // removed to/from the visible packages when the current - // filter parameters are re-evaluated on this package. - bool wasVisible = fVisiblePackages.Contains(ref); - bool isVisible; - { - BAutolock locker(fModel.Lock()); - // The package didn't get a chance yet to be in the - // visible package list - isVisible = fModel.MatchesFilter(ref); - - // Transfer this single package, otherwise we miss - // other packages if they appear or disappear along - // with this one before receive a notification for - // them. - if (isVisible) { - fVisiblePackages.Add(ref); - } else if (wasVisible) - fVisiblePackages.Remove(ref); - } - - if (wasVisible != isVisible) { - if (!isVisible) { - if (fPackageListView != NULL) - fPackageListView->RemovePackage(ref); - if (fFeaturedPackagesView != NULL) - fFeaturedPackagesView->RemovePackage(ref); - } else { - if (fPackageListView != NULL) - fPackageListView->AddPackage(ref); - if (fFeaturedPackagesView != NULL && ref->IsProminent()) - fFeaturedPackagesView->AddPackage(ref); - } - } - + _AddRemovePackageFromLists(ref); if (!fSinglePackageMode && (changes & PKG_CHANGED_STATE) != 0 && fCoordinator == NULL) { fWorkStatusView->PackageStatusChanged(ref); @@ -575,69 +526,6 @@ MainWindow::MessageReceived(BMessage* message) fWorkStatusView->SetIdle(); break; - case MSG_ADD_VISIBLE_PACKAGES: - { - struct SemaphoreReleaser { - SemaphoreReleaser(sem_id semaphore) - : - fSemaphore(semaphore) - { } - - ~SemaphoreReleaser() { release_sem(fSemaphore); } - - sem_id fSemaphore; - }; - - // Make sure acknowledge semaphore is released even on error, - // so the worker thread won't be blocked - SemaphoreReleaser acknowledger(fShowPackagesAcknowledgeSem); - - int32 numPackages = 0; - type_code unused; - status_t status = message->GetInfo("package_ref", &unused, - &numPackages); - if (status != B_OK || numPackages == 0) - break; - - int32 listID = 0; - status = message->FindInt32("list_id", &listID); - if (status != B_OK) - break; - if (listID != atomic_get(&fPackagesToShowListID)) { - // list is outdated, ignore - break; - } - - for (int i = 0; i < numPackages; i++) { - PackageInfo* packageRaw = NULL; - status = message->FindPointer("package_ref", i, - (void**)&packageRaw); - if (status != B_OK) - break; - PackageInfoRef package(packageRaw, true); - - if (fPackageListView != NULL) - fPackageListView->AddPackage(package); - if (fFeaturedPackagesView != NULL && package->IsProminent()) - fFeaturedPackagesView->AddPackage(package); - } - break; - } - - case MSG_UPDATE_SELECTED_PACKAGE: - { - const PackageInfoRef& selectedPackage = fPackageInfoView->Package(); - if (fFeaturedPackagesView != NULL) - fFeaturedPackagesView->SelectPackage(selectedPackage, true); - if (fPackageListView != NULL) - fPackageListView->SelectPackage(selectedPackage); - - AutoLocker modelLocker(fModel.Lock()); - if (!fVisiblePackages.Contains(fPackageInfoView->Package())) - fPackageInfoView->Clear(); - break; - } - case MSG_USER_USAGE_CONDITIONS_NOT_LATEST: { BMessage userDetailMsg; @@ -909,43 +797,15 @@ MainWindow::_InitWorkerThreads() resume_thread(fPopulatePackageWorker); } else fPopulatePackageWorker = -1; - - fNewPackagesToShowSem = create_sem(0, "ShowPackages"); - fShowPackagesAcknowledgeSem = create_sem(0, "ShowPackagesAck"); - if (fNewPackagesToShowSem >= 0 && fShowPackagesAcknowledgeSem >= 0) { - fShowPackagesWorker = spawn_thread(&_PackagesToShowWorker, - "Good news everyone", B_NORMAL_PRIORITY, this); - if (fShowPackagesWorker >= 0) - resume_thread(fShowPackagesWorker); - } else - fShowPackagesWorker = -1; } void -MainWindow::_AdoptModel() +MainWindow::_AdoptModelControls() { - if (Logger::IsTraceEnabled()) - printf("adopting model to main window ui"); - if (fSinglePackageMode) return; - { - AutoLocker modelLocker(fModel.Lock()); - fVisiblePackages = fModel.CreatePackageList(); - AutoLocker listLocker(fPackagesToShowListLock); - fPackagesToShowList = fVisiblePackages; - atomic_add(&fPackagesToShowListID, 1); - } - - if (fFeaturedPackagesView != NULL) - fFeaturedPackagesView->Clear(); - if (fPackageListView != NULL) - fPackageListView->Clear(); - - release_sem(fNewPackagesToShowSem); - BAutolock locker(fModel.Lock()); fShowAvailablePackagesItem->SetMarked(fModel.ShowAvailablePackages()); fShowInstalledPackagesItem->SetMarked(fModel.ShowInstalledPackages()); @@ -961,6 +821,51 @@ MainWindow::_AdoptModel() } +void +MainWindow::_AdoptModel() +{ + if (Logger::IsTraceEnabled()) + printf("adopting model to main window ui\n"); + + if (fSinglePackageMode) + return; + + fModel.Lock()->Lock(); + const DepotList& depots = fModel.Depots(); + fModel.Lock()->Unlock(); + + for (int32 d = 0; d < depots.CountItems(); d++) { + const DepotInfo& depot = depots.ItemAtFast(d); + const PackageList& packages = depot.Packages(); + for (int32 p = 0; p < packages.CountItems(); p++) + _AddRemovePackageFromLists(packages.ItemAtFast(p)); + } + + _AdoptModelControls(); +} + + +void +MainWindow::_AddRemovePackageFromLists(const PackageInfoRef& package) +{ + bool matches; + + { + AutoLocker modelLocker(fModel.Lock()); + matches = fModel.MatchesFilter(package); + } + + if (matches) { + if (package->IsProminent()) + fFeaturedPackagesView->AddPackage(package); + fPackageListView->AddPackage(package); + } else { + fFeaturedPackagesView->RemovePackage(package); + fPackageListView->RemovePackage(package); + } +} + + void MainWindow::_AdoptPackage(const PackageInfoRef& package) { @@ -988,6 +893,12 @@ MainWindow::_ClearPackage() void MainWindow::_StartBulkLoad(bool force) { + if (fFeaturedPackagesView != NULL) + fFeaturedPackagesView->Clear(); + if (fPackageListView != NULL) + fPackageListView->Clear(); + fPackageInfoView->Clear(); + fRefreshRepositoriesItem->SetEnabled(false); ProcessCoordinator* bulkLoadCoordinator = ProcessCoordinatorFactory::CreateBulkLoadCoordinator( @@ -1176,78 +1087,6 @@ MainWindow::_PopulatePackageWorker(void* arg) } -/* static */ status_t -MainWindow::_PackagesToShowWorker(void* arg) -{ - MainWindow* window = reinterpret_cast(arg); - - while (acquire_sem(window->fNewPackagesToShowSem) == B_OK) { - PackageList packageList; - int32 listID = 0; - { - AutoLocker lock(&window->fPackagesToShowListLock); - packageList = window->fPackagesToShowList; - listID = atomic_get(&window->fPackagesToShowListID); - window->fPackagesToShowList.Clear(); - } - - // Add packages to list views in batches of kPackagesPerUpdate so we - // don't block the window thread for long with each iteration - enum { - kPackagesPerUpdate = 20 - }; - uint32 packagesInMessage = 0; - BMessage message(MSG_ADD_VISIBLE_PACKAGES); - BMessenger messenger(window); - bool listIsOutdated = false; - - for (int i = 0; i < packageList.CountItems(); i++) { - const PackageInfoRef& package = packageList.ItemAtFast(i); - - if (packagesInMessage >= kPackagesPerUpdate) { - if (listID != atomic_get(&window->fPackagesToShowListID)) { - // The model was changed again in the meantime, and thus - // our package list isn't current anymore. Send no further - // messags based on this list and go back to start. - listIsOutdated = true; - break; - } - - message.AddInt32("list_id", listID); - messenger.SendMessage(&message); - message.MakeEmpty(); - packagesInMessage = 0; - - // Don't spam the window's message queue, which would make it - // unresponsive (i.e. allows UI messages to get in between our - // messages). When it has processed the message we just sent, - // it will let us know by releasing the semaphore. - acquire_sem(window->fShowPackagesAcknowledgeSem); - } - package->AcquireReference(); - message.AddPointer("package_ref", package.Get()); - packagesInMessage++; - } - - if (listIsOutdated) - continue; - - // Send remaining package infos, if any, which didn't make it into - // the last message (count < kPackagesPerUpdate) - if (packagesInMessage > 0) { - message.AddInt32("list_id", listID); - messenger.SendMessage(&message); - acquire_sem(window->fShowPackagesAcknowledgeSem); - } - - // Update selected package in list views - messenger.SendMessage(MSG_UPDATE_SELECTED_PACKAGE); - } - - return 0; -} - - void MainWindow::_OpenLoginWindow(const BMessage& onSuccessMessage) { diff --git a/src/apps/haikudepot/ui/MainWindow.h b/src/apps/haikudepot/ui/MainWindow.h index 0dddeae636..32a3717d9c 100644 --- a/src/apps/haikudepot/ui/MainWindow.h +++ b/src/apps/haikudepot/ui/MainWindow.h @@ -87,7 +87,10 @@ private: void _RestoreModelSettings(const BMessage& settings); void _InitWorkerThreads(); + void _AdoptModelControls(); void _AdoptModel(); + void _AddRemovePackageFromLists( + const PackageInfoRef& package); void _AdoptPackage(const PackageInfoRef& package); void _ClearPackage(); @@ -151,7 +154,6 @@ private: Model fModel; ModelListenerRef fModelListener; - PackageList fVisiblePackages; std::queue fCoordinatorQueue; @@ -171,14 +173,6 @@ private: bool fForcePopulatePackage; BLocker fPackageToPopulateLock; sem_id fPackageToPopulateSem; - - thread_id fShowPackagesWorker; - PackageList fPackagesToShowList; - int32 fPackagesToShowListID; - // atomic, counted up whenever fPackagesToShowList is refilled - BLocker fPackagesToShowListLock; - sem_id fNewPackagesToShowSem; - sem_id fShowPackagesAcknowledgeSem; }; #endif // MAIN_WINDOW_H