[pacman-dev] Eliminate extra loop over dbs_sync

Message ID 20200130052710.201469-1-anatol.pomozov@gmail.com
State Accepted, archived
Headers show
Series [pacman-dev] Eliminate extra loop over dbs_sync | expand

Commit Message

Anatol Pomozov Jan. 30, 2020, 5:27 a.m. UTC
Current flow looks like
  loop dbs_sync {
    loop pkgs {
      if pkg.db == db then process(pkg, db)
    }
  }

Package sync transaction always has a counterpart in the dbs_sync list
(I cannot come up with a use-case when it is not true). So the loop can
be simplified to:

  loop pkgs {
    process(pkg, pkg.db)
  }

Tested: 'ninja test' & manually by using pacman with this patch for a
week
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 lib/libalpm/sync.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Allan McRae Jan. 30, 2020, 2:37 p.m. UTC | #1
On 30/1/20 3:27 pm, Anatol Pomozov wrote:
> Current flow looks like
>   loop dbs_sync {
>     loop pkgs {
>       if pkg.db == db then process(pkg, db)
>     }
>   }
> 
> Package sync transaction always has a counterpart in the dbs_sync list
> (I cannot come up with a use-case when it is not true). So the loop can
> be simplified to:
> 
>   loop pkgs {
>     process(pkg, pkg.db)
>   }
> 
> Tested: 'ninja test' & manually by using pacman with this patch for a
> week
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>

I have been trying to figure out if there was an historical reason for
this...  The loops was added in:

d37ad0487 (Aaron Griffin   2006-10-15 19:31:03 +0000  805)

which is a big merge from Frugalware.

So in short, this looks fine to me.

Allan
Allan McRae Jan. 31, 2020, 3:04 a.m. UTC | #2
On 31/1/20 12:37 am, Allan McRae wrote:
> On 30/1/20 3:27 pm, Anatol Pomozov wrote:
>> Current flow looks like
>>   loop dbs_sync {
>>     loop pkgs {
>>       if pkg.db == db then process(pkg, db)
>>     }
>>   }
>>
>> Package sync transaction always has a counterpart in the dbs_sync list
>> (I cannot come up with a use-case when it is not true). So the loop can
>> be simplified to:
>>
>>   loop pkgs {
>>     process(pkg, pkg.db)
>>   }
>>
>> Tested: 'ninja test' & manually by using pacman with this patch for a
>> week
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> 
> I have been trying to figure out if there was an historical reason for
> this...  The loops was added in:
> 
> d37ad0487 (Aaron Griffin   2006-10-15 19:31:03 +0000  805)
> 
> which is a big merge from Frugalware.
> 
> So in short, this looks fine to me.

I haven't confirmed...  but it likely effects download order.  Currently
downloads occur roughly in repo order.  I don't think it matters, but
thought I would flag it anyway.

Allan

Patch

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index d02a435f..4d93f41f 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -705,16 +705,14 @@  static struct dload_payload *build_payload(alpm_handle_t *handle,
 		return payload;
 }
 
-static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files)
+static int find_dl_candidates(alpm_handle_t *handle, alpm_list_t **files)
 {
-	alpm_list_t *i;
-	alpm_handle_t *handle = repo->handle;
-
-	for(i = handle->trans->add; i; i = i->next) {
+	for(alpm_list_t *i = handle->trans->add; i; i = i->next) {
 		alpm_pkg_t *spkg = i->data;
 
-		if(spkg->origin != ALPM_PKG_FROM_FILE && repo == spkg->origin_data.db) {
+		if(spkg->origin != ALPM_PKG_FROM_FILE) {
 			char *fpath = NULL;
+			alpm_db_t *repo = spkg->origin_data.db;
 
 			if(!repo->servers) {
 				handle->pm_errno = ALPM_ERR_SERVER_NONE;
@@ -802,9 +800,7 @@  static int download_files(alpm_handle_t *handle)
 		handle->totaldlcb(total_size);
 	}
 
-	for(i = handle->dbs_sync; i; i = i->next) {
-		errors += find_dl_candidates(i->data, &files);
-	}
+	errors += find_dl_candidates(handle, &files);
 
 	if(files) {
 		/* check for necessary disk space for download */