lib: replace xbps_file_hash_check_dictionary with transaction_file lookup #359

Duncaen · 2020-12-12T22:10:02Z

xbps_file_hash_check_dictionary was called for every file that is
getting unpacked, because the files list is an array it iterated over
the whole files array to find the matching file.
With a lot of files this is really slow and a lot of time was spend
in locking the proplib array and iterating over it.

Time from my Ryzen 3700X system with nvme disk updating
texlive-fontsextra with files 86375:

time xbps-install -y texlive-fontsextra
    6m34.61s real     6m27.71s user     0m03.95s system

And with this patch:

time xbps-install -y texlive-fontsextra
    0m08.40s real     0m07.34s user     0m00.98s system

…okup xbps_file_hash_check_dictionary was called for every file that is getting unpacked, because the files list is an array it iterated over the whole files array to find the matching file. With a lot of files this is really slow and a lot of time was spend in locking the proplib array and iterating over it. Time from my Ryzen 3700X system with nvme disk updating texlive-fontsextra with files 86375: time xbps-install -y texlive-fontsextra 6m34.61s real 6m27.71s user 0m03.95s system And with this patch: time xbps-install -y texlive-fontsextra 0m08.40s real 0m07.34s user 0m00.98s system

ericonr · 2020-12-14T15:24:55Z

lib/package_unpack.c

+					xbps_dbg_printf(xhp, "%s: %s\n", entry_pname, file->sha256);
+					assert(file->sha256);


Might be nice to make it clear that a (null) there is an error? Not sure.

file->sha256 ? file->sha256 : "error..."

include/xbps_api_impl.h

ericonr · 2021-02-10T04:24:56Z

For the record, a good test case here seems to be updating papirus-icon-theme. It took 2m37s going from 20201001_1 to 20210201_1, all of it inside xbps-install, not any hooks.

I will build this locally and see how better it is.

EDIT: I was testing on a loaded machine, so it isn't a clean test, but this PR still took 52s. Without load, 31s.

Duncaen · 2021-02-10T14:10:07Z

This should greatly reduce the time needed, not sure why this example with more files is faster, maybe its just a lot more symlinks or smaller files that would need to be checksumed.

For the implementation, I think I would rename a few things, xbps_transaction_file_new does not really represent what the function actually does and maybe it would make sense to call the structure xbps_file instead og xbps_transaction_file as the struct could be reused for other things in the future.
Might also make sense to move some of the fields like "bool update" back into the transaction_files item structure as some of those are not important outside of the context of finding conflicts in the transaction.

ericonr · 2021-02-10T15:18:08Z

include/xbps_api_impl.h

+	TYPE_DIR,
+	TYPE_FILE,
+	TYPE_CONFFILE,
+} transaction_file_type_t;


Technically speaking *_t is reserved for libc (https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html), but everyone uses it anyway.

ericonr · 2021-02-10T15:27:46Z

include/xbps_api_impl.h

+	const char *pkgname;
+	const char *pkgver;
+	char *sha256;
+	const char *target;
+	uint64_t size;
+	transaction_file_type_t type;


Would these first 6 be the xbps_file struct? If so, I guess transaction_file_type would be switched over to file_type.

Maybe we need two struct a xbps_transaction_file and a xbps_file which is part of the former.
The idea would be to start using the xbps_file struct in places where we currently use the xbps_dictionary references to files.plist so that we gradually switch from dynamic plist types to actual structs which should help to avoid some issues at compile time through the type system.
I think xbps_file would be a simple way to start, doing this for packages would be the end goal, there are so many places that use xbps_dictionarys for packages and there are a lot of places that can be improved on the way to switching to c structs.

ericonr · 2021-02-10T16:58:44Z

lib/transaction_files.c

+		if (sha256)
+			item->new.sha256 = strdup(sha256);


Is item->new always guaranteed to be zero initialized? Might be reasonable to add an else item->new.sha256 = NULL;

Generally everything is zero initialized, IMHO not doing that by default when initializing the memory should only be done if you know you overwrite it immediately.

ericonr · 2021-02-10T17:05:03Z

include/xbps_api_impl.h

+void xbps_transaction_files_free(void);
 int HIDDEN xbps_transaction_fetch(struct xbps_handle *,
 		xbps_object_iterator_t);
 int HIDDEN xbps_transaction_pkg_deps(struct xbps_handle *, xbps_array_t, xbps_dictionary_t);

+struct transaction_file *xbps_transaction_file_new(struct xbps_handle *, const char *);
+


The naming is a bit confusing, in that xbps_transaction_file_new was added, but no xbps_transaction_file_free, and xbps_transaction_files_free was added, without a corresponding _new, and doesn't seem to be called at all.

Yes the _new does not actually create anything, it gives you a reference to a file struct that already exists from collecting all files in the transaction, "_new" because it returns the new file as opposed to the old file.

Not sure how this should be done exactly, something like xbps_transaction_get_file with either _new or _old or a parameter to choose which one you want.

xbps_transaction_files_free deletes the whole hashtable/tree, this should be called from somewhere after the transaction is done, maybe from xbps_transaction_commit.

Not sure how this should be done exactly, something like xbps_transaction_get_file with either _new or _old or a parameter to choose which one you want.

I think the parameter helps avoid duplicated logic, so I'm in favor of that.

ericonr reviewed Dec 14, 2020

View reviewed changes

Duncaen added 2 commits December 17, 2020 22:51

lib/transaction_files.c: also record new files from reinstall/downgrade

62c82bc

include/xbps_api_impl.h: remove attributes from prototype

c4c5114

ericonr reviewed Feb 10, 2021

View reviewed changes

Duncaen mentioned this pull request Jan 10, 2024

Replace xbps_file_hash_check_dictionary #582

Open

		xbps_dbg_printf(xhp, "%s: %s\n", entry_pname, file->sha256);
		assert(file->sha256);

lib: replace xbps_file_hash_check_dictionary with transaction_file lookup #359

Are you sure you want to change the base?

lib: replace xbps_file_hash_check_dictionary with transaction_file lookup #359

Uh oh!

Conversation

Duncaen commented Dec 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericonr commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Duncaen commented Feb 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Duncaen commented Dec 12, 2020 •

edited

Loading

ericonr commented Feb 10, 2021 •

edited

Loading