[pacman-dev] Convert payload structs from heap allocated to stack allocated

Message ID 20200209235525.197390-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev] Convert payload structs from heap allocated to stack allocated | expand

Commit Message

Anatol Pomozov Feb. 9, 2020, 11:55 p.m. UTC
download_files() dynamically allocates a payload object for each
package. Then iterates over these payloads and calls
download_single_file() for it.

This code can be simplified to iterate over package list itself.
payload struct can be stack allocated in this case.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 lib/libalpm/sync.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

Comments

Allan McRae Feb. 12, 2020, 8:36 a.m. UTC | #1
On 10/2/20 9:55 am, Anatol Pomozov wrote:
> download_files() dynamically allocates a payload object for each
> package. Then iterates over these payloads and calls
> download_single_file() for it.
> 
> This code can be simplified to iterate over package list itself.
> payload struct can be stack allocated in this case.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---

This patch looks fine (minor change below).

However, the change from heap to stack allocated is rather unimportant.
 Your commit message should highlight what the patch achieves.  E.g.


Simplify construction of payloads in download_files

Currently, download_files() creates payloads for all packages then
iterates over them, calling download_single_file.  This can be
simplified by looping over packages and constructing the payload as needed.


Saying that...  it is really a limited simplification.  I assume this
makes implementing parallel downloads easier later on.  If so, state
that too.


<snip>

> +	if(files)
> +		alpm_list_free(files);

Note we keep the surrounding brackets even for single line statements.

Allan
Anatol Pomozov Feb. 20, 2020, 1:46 a.m. UTC | #2
Hello

On Wed, Feb 12, 2020 at 12:36 AM Allan McRae <allan@archlinux.org> wrote:
>
> On 10/2/20 9:55 am, Anatol Pomozov wrote:
> > download_files() dynamically allocates a payload object for each
> > package. Then iterates over these payloads and calls
> > download_single_file() for it.
> >
> > This code can be simplified to iterate over package list itself.
> > payload struct can be stack allocated in this case.
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> > ---
>
> This patch looks fine (minor change below).
>
> However, the change from heap to stack allocated is rather unimportant.
>  Your commit message should highlight what the patch achieves.  E.g.
>
>
> Simplify construction of payloads in download_files
>
> Currently, download_files() creates payloads for all packages then
> iterates over them, calling download_single_file.  This can be
> simplified by looping over packages and constructing the payload as needed.
>

>
> <snip>
>
> > +     if(files)
> > +             alpm_list_free(files);

Fixed it and modified the description. An updated patch is sent to the maillist.

> Saying that...  it is really a limited simplification.  I assume this
> makes implementing parallel downloads easier later on.  If so, state
> that too.

Correct. It is a small part of the "parallel download" work. I've
decided to split it into separate patch and send to the list for a
review.

Patch

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 4d93f41f..cd9db641 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -693,18 +693,6 @@  static int prompt_to_delete(alpm_handle_t *handle, const char *filepath,
 	return question.remove;
 }
 
-static struct dload_payload *build_payload(alpm_handle_t *handle,
-		const char *filename, size_t size, alpm_list_t *servers)
-{
-		struct dload_payload *payload;
-
-		CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
-		STRDUP(payload->remote_name, filename, FREE(payload); RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
-		payload->max_size = size;
-		payload->servers = servers;
-		return payload;
-}
-
 static int find_dl_candidates(alpm_handle_t *handle, alpm_list_t **files)
 {
 	for(alpm_list_t *i = handle->trans->add; i; i = i->next) {
@@ -729,10 +717,7 @@  static int find_dl_candidates(alpm_handle_t *handle, alpm_list_t **files)
 			}
 
 			if(spkg->download_size != 0 || !fpath) {
-				struct dload_payload *payload;
-				payload = build_payload(handle, spkg->filename, spkg->size, repo->servers);
-				ASSERT(payload, return -1);
-				*files = alpm_list_add(*files, payload);
+				*files = alpm_list_add(*files, spkg);
 			}
 
 			FREE(fpath);
@@ -815,8 +800,8 @@  static int download_files(alpm_handle_t *handle)
 			CALLOC(file_sizes, num_files, sizeof(off_t), goto finish);
 
 			for(i = files, idx = 0; i; i = i->next, idx++) {
-				const struct dload_payload *payload = i->data;
-				file_sizes[idx] = payload->max_size;
+				const alpm_pkg_t *pkg = i->data;
+				file_sizes[idx] = pkg->size;
 			}
 
 			ret = _alpm_check_downloadspace(handle, cachedir, num_files, file_sizes);
@@ -832,20 +817,26 @@  static int download_files(alpm_handle_t *handle)
 		EVENT(handle, &event);
 		event.type = ALPM_EVENT_RETRIEVE_DONE;
 		for(i = files; i; i = i->next) {
-			if(download_single_file(handle, i->data, cachedir) == -1) {
+			const alpm_pkg_t *pkg = i->data;
+			struct dload_payload payload = {0};
+
+			STRDUP(payload.remote_name, pkg->filename, handle->pm_errno = ALPM_ERR_MEMORY; goto finish);
+			payload.servers = pkg->origin_data.db->servers;
+			payload.max_size = pkg->size;
+
+			if(download_single_file(handle, &payload, cachedir) == -1) {
 				errors++;
 				event.type = ALPM_EVENT_RETRIEVE_FAILED;
 				_alpm_log(handle, ALPM_LOG_WARNING, _("failed to retrieve some files\n"));
 			}
+			_alpm_dload_payload_reset(&payload);
 		}
 		EVENT(handle, &event);
 	}
 
 finish:
-	if(files) {
-		alpm_list_free_inner(files, (alpm_list_fn_free)_alpm_dload_payload_reset);
-		FREELIST(files);
-	}
+	if(files)
+		alpm_list_free(files);
 
 	for(i = handle->trans->add; i; i = i->next) {
 		alpm_pkg_t *pkg = i->data;