lib: replace xbps_file_hash_check_dictionary with transaction_file lookup by Duncaen · Pull Request #359 · void-linux/xbps

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: replace xbps_file_hash_check_dictionary with transaction_file lookup#359

lib: replace xbps_file_hash_check_dictionary with transaction_file lookup#359
Duncaen wants to merge 3 commits intovoid-linux:masterfrom
Duncaen:unpack-speedup

Duncaen commented Dec 12, 2020 •

edited

Loading

Uh oh!

ericonr Dec 14, 2020

Uh oh!

Uh oh!

ericonr commented Feb 10, 2021 •

edited

Loading

Uh oh!

Duncaen commented Feb 10, 2021

Uh oh!

ericonr Feb 10, 2021

Uh oh!

ericonr Feb 10, 2021

Uh oh!

Duncaen Feb 10, 2021

Uh oh!

ericonr Feb 10, 2021

Uh oh!

Duncaen Feb 10, 2021

Uh oh!

ericonr Feb 10, 2021

Uh oh!

Duncaen Feb 10, 2021

Uh oh!

Duncaen Feb 10, 2021

Uh oh!

ericonr Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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